Skip to content

Commit 6bac04c

Browse files
authored
chore: log all bridge-api validation errors (#5913)
## Explanation Improves response validation traces by logging all invalid fields. Currently the logs only include the first detected failure. View test snapshot updates to see how this changes the logged data <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- THIS SECTION IS NO LONGER NEEDED. The process for updating changelogs has changed. Please consult the "Updating changelogs" section of the Contributing doc for more. --> ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent 787c256 commit 6bac04c

File tree

7 files changed

+96
-8
lines changed

7 files changed

+96
-8
lines changed

packages/bridge-controller/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Include all invalid quote properties in sentry logs ([#5913](https://github.com/MetaMask/core/pull/5913))
13+
1014
## [32.0.1]
1115

1216
### Fixed
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Jest Snapshot v1, https://goo.gl/fbAQLP
2+
3+
exports[`fetch fetchBridgeQuotes should filter out malformed bridge quotes 1`] = `
4+
Array [
5+
Array [
6+
"Quote validation failed",
7+
Object {
8+
"socket": Set {
9+
"quote.requestId",
10+
"quote.srcChainId",
11+
"quote.srcAsset.decimals",
12+
"quote.srcTokenAmount",
13+
"quote.destChainId",
14+
"quote.destAsset",
15+
"quote.destTokenAmount",
16+
"quote.feeData",
17+
"quote.bridges",
18+
"quote.steps",
19+
"quote.srcAsset",
20+
"quote.destAsset.address",
21+
},
22+
"undefined": Set {
23+
"quote",
24+
},
25+
},
26+
],
27+
]
28+
`;

packages/bridge-controller/src/utils/fetch.test.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,9 @@ describe('fetch', () => {
225225
});
226226

227227
it('should filter out malformed bridge quotes', async () => {
228+
const mockConsoleError = jest
229+
.spyOn(console, 'error')
230+
.mockImplementation(jest.fn());
228231
mockFetchFn.mockResolvedValue([
229232
...mockBridgeQuotesErc20Erc20,
230233
...mockBridgeQuotesErc20Erc20.map(
@@ -233,6 +236,7 @@ describe('fetch', () => {
233236
{
234237
...mockBridgeQuotesErc20Erc20[0],
235238
quote: {
239+
bridgeId: 'socket',
236240
srcAsset: {
237241
...mockBridgeQuotesErc20Erc20[0].quote.srcAsset,
238242
decimals: undefined,
@@ -242,7 +246,8 @@ describe('fetch', () => {
242246
{
243247
...mockBridgeQuotesErc20Erc20[1],
244248
quote: {
245-
srcAsset: {
249+
bridgeId: 'socket',
250+
destAsset: {
246251
...mockBridgeQuotesErc20Erc20[1].quote.destAsset,
247252
address: undefined,
248253
},
@@ -280,6 +285,8 @@ describe('fetch', () => {
280285
);
281286

282287
expect(result).toStrictEqual(mockBridgeQuotesErc20Erc20);
288+
// eslint-disable-next-line jest/no-restricted-matchers
289+
expect(mockConsoleError.mock.calls).toMatchSnapshot();
283290
});
284291
});
285292

packages/bridge-controller/src/utils/fetch.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { StructError } from '@metamask/superstruct';
12
import type { CaipAssetType, CaipChainId, Hex } from '@metamask/utils';
23
import { Duration } from '@metamask/utils';
34

@@ -103,14 +104,30 @@ export async function fetchBridgeQuotes(
103104
functionName: 'fetchBridgeQuotes',
104105
});
105106

107+
const validationFailuresByAggregator: {
108+
[aggregator: string]: Set<string>;
109+
} = {};
106110
const filteredQuotes = quotes.filter((quoteResponse: unknown) => {
107111
try {
108112
return validateQuoteResponse(quoteResponse);
109113
} catch (error) {
110-
console.error(error);
114+
if (error instanceof StructError) {
115+
error.failures().forEach(({ branch, path }) => {
116+
const aggregatorId = branch?.[0]?.quote?.bridgeId;
117+
if (!validationFailuresByAggregator[aggregatorId]) {
118+
validationFailuresByAggregator[aggregatorId] = new Set([]);
119+
}
120+
const pathString = path?.join('.') || 'unknown';
121+
validationFailuresByAggregator[aggregatorId].add(pathString);
122+
});
123+
}
111124
return false;
112125
}
113126
});
127+
128+
if (Object.keys(validationFailuresByAggregator).length > 0) {
129+
console.error('Quote validation failed', validationFailuresByAggregator);
130+
}
114131
return filteredQuotes as QuoteResponse[];
115132
}
116133

packages/bridge-status-controller/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Include all invalid status properties in sentry logs ([#5913](https://github.com/MetaMask/core/pull/5913))
13+
1014
## [29.0.0]
1115

1216
### Changed

packages/bridge-status-controller/src/utils/__snapshots__/validators.test.ts.snap

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,39 +3,55 @@
33
exports[`validators bridgeStatusValidator should throw for invalid response for complete bridge status with missing fields 1`] = `
44
Array [
55
Array [
6-
[StructError: At path: srcChain -- Expected an object, but received: undefined],
6+
"Bridge status validation failed",
7+
Object {
8+
"srcChain": "[across] Expected an object, but received: undefined",
9+
},
710
],
811
]
912
`;
1013

1114
exports[`validators bridgeStatusValidator should throw for invalid response for empty object 1`] = `
1215
Array [
1316
Array [
14-
[StructError: At path: status -- Expected one of \`"UNKNOWN","FAILED","PENDING","COMPLETE"\`, but received: undefined],
17+
"Bridge status validation failed",
18+
Object {
19+
"srcChain": "[unknown] Expected an object, but received: undefined",
20+
"status": "[unknown] Expected one of \`\\"UNKNOWN\\",\\"FAILED\\",\\"PENDING\\",\\"COMPLETE\\"\`, but received: undefined",
21+
},
1522
],
1623
]
1724
`;
1825

1926
exports[`validators bridgeStatusValidator should throw for invalid response for null 1`] = `
2027
Array [
2128
Array [
22-
[StructError: Expected an object, but received: null],
29+
"Bridge status validation failed",
30+
Object {
31+
"unknown": "[unknown] Expected an object, but received: null",
32+
},
2333
],
2434
]
2535
`;
2636

2737
exports[`validators bridgeStatusValidator should throw for invalid response for pending bridge status with missing fields 1`] = `
2838
Array [
2939
Array [
30-
[StructError: At path: destChain.chainId -- Expected the value to satisfy a union of \`number | string\`, but received: undefined],
40+
"Bridge status validation failed",
41+
Object {
42+
"destChain.chainId": "[across] Expected a string, but received: undefined",
43+
},
3144
],
3245
]
3346
`;
3447

3548
exports[`validators bridgeStatusValidator should throw for invalid response for undefined 1`] = `
3649
Array [
3750
Array [
38-
[StructError: Expected an object, but received: undefined],
51+
"Bridge status validation failed",
52+
Object {
53+
"unknown": "[unknown] Expected an object, but received: undefined",
54+
},
3955
],
4056
]
4157
`;

packages/bridge-status-controller/src/utils/validators.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
type,
1111
nullable,
1212
assert,
13+
StructError,
1314
} from '@metamask/superstruct';
1415

1516
export const validateBridgeStatusResponse = (data: unknown) => {
@@ -52,10 +53,21 @@ export const validateBridgeStatusResponse = (data: unknown) => {
5253
refuel: optional(RefuelStatusResponseSchema),
5354
});
5455

56+
const validationFailures: { [path: string]: string } = {};
5557
try {
5658
assert(data, StatusResponseSchema);
5759
} catch (error) {
58-
console.error(error);
60+
if (error instanceof StructError) {
61+
error.failures().forEach(({ branch, path, message }) => {
62+
const pathString = path?.join('.') || 'unknown';
63+
validationFailures[pathString] =
64+
`[${branch?.[0]?.bridge || 'unknown'}] ${message}`;
65+
});
66+
}
5967
throw error;
68+
} finally {
69+
if (Object.keys(validationFailures).length > 0) {
70+
console.error(`Bridge status validation failed`, validationFailures);
71+
}
6072
}
6173
};

0 commit comments

Comments
 (0)