Conversation
Jwhiles
force-pushed
the
add-chomp-api
branch
4 times, most recently
from
8b1785b to
be04334
Compare
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
This looks pretty good so far! I just had a few comments.
| return this.#mutationPolicy.execute(async () => { | ||
| const response = await fetch(new URL(path, this.#baseUrl), { | ||
| method: 'POST', | ||
| headers, | ||
| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| if (!response.ok && !acceptedStatuses.includes(response.status)) { | ||
| throw new HttpError( | ||
| response.status, | ||
| `POST ${path} failed with status '${response.status}'`, | ||
| ); | ||
| } | ||
|
|
||
| return response; | ||
| }); |
There was a problem hiding this comment.
After I gave you the suggest to use the service policy manually like this, I came across another PR that someone else posted here: https://github.com/MetaMask/core/pull/8260/changes#diff-df87c072da30a25c3d5d0291f962f6b7c52da863a4d1e79a11752d89bc471c4cR204.
Basically it seems that we can use fetchQuery, but pass staleTime: 0 to effectively turn off caching. Can you try that and see how that works? So this would look something like:
| return this.#mutationPolicy.execute(async () => { | |
| const response = await fetch(new URL(path, this.#baseUrl), { | |
| method: 'POST', | |
| headers, | |
| body: JSON.stringify(body), | |
| }); | |
| if (!response.ok && !acceptedStatuses.includes(response.status)) { | |
| throw new HttpError( | |
| response.status, | |
| `POST ${path} failed with status '${response.status}'`, | |
| ); | |
| } | |
| return response; | |
| }); | |
| return this.fetchQuery({ | |
| queryKey: [ | |
| // ... you might want to pass this in ... | |
| ], | |
| queryFn: async () => { | |
| const response = await fetch(new URL(path, this.#baseUrl), { | |
| method: 'POST', | |
| headers, | |
| body: JSON.stringify(body), | |
| }); | |
| if (!response.ok && !acceptedStatuses.includes(response.status)) { | |
| throw new HttpError( | |
| response.status, | |
| `POST ${path} failed with status '${response.status}'`, | |
| ); | |
| } | |
| return response; | |
| }), | |
| staleTime: 0, | |
| }); |
| }); | ||
|
|
||
| this.#baseUrl = baseUrl; | ||
| this.#getAccessToken = getAccessToken; |
There was a problem hiding this comment.
Where will this access token come from in the clients? Will this come from another controller or service? If so, then it would be better to use the messenger to retrieve this value instead of taking a callback.
| @@ -0,0 +1,15 @@ | |||
| # `@metamask/chom-api-service` | |||
There was a problem hiding this comment.
Typo?
| # `@metamask/chom-api-service` | |
| # `@metamask/chomp-api-service` |
There was a problem hiding this comment.
In addition to modifying these files you should also add chomp-api-service to the root tsconfig.json and tsconfig.build.json files to ensure that TypeScript knows about it (and its relationship to other packages) in development.
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
|
@metamaskbot publish-preview |
Jwhiles
force-pushed
the
add-chomp-api
branch
2 times, most recently
from
0e8c506 to
08b3d87
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Jwhiles
force-pushed
the
add-chomp-api
branch
5 times, most recently
from
039faf6 to
2f413ac
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a820fad. Configure here.
|
|
||
| const HexStringStruct = define<string>('Hex string', (value) => | ||
| isStrictHexString(value), | ||
| ); |
There was a problem hiding this comment.
Redundant HexStringStruct duplicates existing StrictHexStruct
Low Severity
The locally defined HexStringStruct using define and isStrictHexString duplicates StrictHexStruct already exported from @metamask/utils, which this package already depends on. Multiple other packages in the monorepo (eip-5792-middleware, eip-7702-internal-rpc-middleware, eth-json-rpc-middleware) import and use StrictHexStruct directly. Replacing the custom definition with the existing export avoids maintaining a redundant copy.
Reviewed by Cursor Bugbot for commit a820fad. Configure here.
3 participants
Explanation
References
Checklist
Note
Medium Risk
Adds new network-facing service code that fetches with bearer-token authentication and caches results, so incorrect error handling or schema validation could impact clients integrating the new package.
Overview
Introduces a new package,
@metamask/chomp-api-service, implementingChompApiServiceon top ofBaseDataServiceto make authenticated (JWT bearer) requests to the CHOMP API with response validation and cached-query semantics.The service exposes messenger actions for key endpoints (associate address, account upgrade create/get, delegation verification, intent create/list, withdrawal create, and service details), defines the request/response types and generated action-type unions, and adds comprehensive nock-based tests for success/error/malformed-response cases.
Repo wiring is updated to include the new package in TypeScript project references, yarn workspace lockfile, docs/graph (
README.md), and ownership/team mappings (CODEOWNERS,teams.json).Reviewed by Cursor Bugbot for commit a820fad. Bugbot is set up for automated code reviews on this repo. Configure here.