feat: wire ClaimsService and ClaimsController initialization - #9588
feat: wire ClaimsService and ClaimsController initialization#9588tuna1207 wants to merge 29 commits into
Conversation
Add default wallet initialization units for ClaimsService and ClaimsController with tight messenger delegation, required claimsService instance options, wallet-cli defaults, ownership, and dependency graph updates. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Colocate ClaimsService with ClaimsController so a single CODEOWNERS initializationPath covers both units, and revert multi-path ownership. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the SubscriptionEnv naming pattern so wallet consumers can set claimsService.env without colliding with other Env enums. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
tuna1207
changed the title
| storage: new InMemoryStorageAdapter(), | ||
| }, | ||
| remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, | ||
| claimsService: { |
There was a problem hiding this comment.
Why? TransactionController doesn't interact with claims though.
There was a problem hiding this comment.
Is this really required for the test file only, since no change has been made to the corresponding controller initialization file? If not, please remove
There was a problem hiding this comment.
It's strange that this didn't require changes to packages/wallet/src/initialization/instances/gas-fee-controller/gas-fee-controller.test.ts as well 🤔 That file also uses the wallet InstanceSpecificOptions type, which has the new required property that necessitated this change.
There was a problem hiding this comment.
The fact that we're embedding the options for the entire wallet library in these test suites is a bit odd. It might be better to test initialization in the wallet test suite, rather than in the test suites for the initialization options for each individual controller. Or at least we should be sharing a "default options" fixture.
| export type InstanceSpecificOptions = { | ||
| approvalController?: ApprovalControllerInstanceOptions; | ||
| claimsService: ClaimsServiceInstanceOptions; | ||
| claimsController?: ClaimsControllerInstanceOptions; |
There was a problem hiding this comment.
May I know why is claimsController optional but claimsService is required?
There was a problem hiding this comment.
The controller only needs state and messenger, which initialization already provides. There's nothing for the consumer to configure, so the slot is optional.
Claim Service need required fetchFunction option
|
One more thing I noticed is that shield/subscriptions/claims controllers have the different constructor interfaces. Some accept |
pedronfigueiredo
left a comment
pedronfigueiredo
left a comment
There was a problem hiding this comment.
I see there's a bunch of comments and merge conflicts, can you address them and then re-request my review here on github once it's ready for another look?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Nice work, I just have one comment.
| init: ({ messenger, options }) => | ||
| new ClaimsService({ | ||
| messenger, | ||
| env: options.env ?? Env.PRD, |
There was a problem hiding this comment.
Is this safe, do we want this to be development by default instead? I don't know what convention we've been using for other services, but it seems like we wouldn't want to hit production unnecessarily.
Alternatively should we make this option required and have clients always pass this?
There was a problem hiding this comment.
This one follow other controllers like profile sync authentication controller, money account api service all default to prd, since for all services with auth we need to follow authentication controller i think we can keep this for now and refactor all controllers to default to dev in the future if needed
There was a problem hiding this comment.
The antipattern here is that env exists as an option in the first place. We should be passing in config, rather than hard-coding it into the library and using env to choose which to use at runtime. This puts it under the control of the clients, which gives us more confidence that the clients are correctly configured, and more flexibility in updating config on short notice if we need to.
Agreed that it can wait for another PR, but please make a ticket to track getting rid of this variable, this is very much contrary to our application architecture.
|
@mcmire @pedronfigueiredo i have refactored claim service to its own separate data service and resolve all the comments, please help me review again, thank you |
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 01d318f. Configure here.
|
@metamaskbot publish-previews |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
| const bearerToken = await this.messenger.call( | ||
| 'AuthenticationController:getBearerToken', | ||
| ); | ||
| const profileKey = bytesToHex(await sha256(stringToBytes(bearerToken))); |
There was a problem hiding this comment.
can u just not use the sub inside the token + service name + method name as query key?
|
|
||
| return await this.fetchQuery({ | ||
| queryKey: [`${this.name}:getClaimById`, id, profileKey], | ||
| // TODO: Restore default staleTime once claim reads are invalidated |
… for enhanced caching
…stead of canonicalProfileId
sirtimid
left a comment
sirtimid
left a comment
There was a problem hiding this comment.
Approved for the wallet-cli package changes.
7 participants
Explanation
Adds
ClaimsServiceandClaimsControlleras default initialized units in the@metamask/walletpackage, following the existingInitializationConfigurationpattern used byTransactionController,StorageService, and other wallet instances.@metamask/walletis the shared controller-integration layer for extension, mobile, and other clients, so build-specific and runtime values are injected viainstanceOptionsrather than hardcoded.ClaimsService — selects the Claims API environment and platform
fetchimplementation.instanceOptions.claimsService.envandinstanceOptions.claimsService.fetchFunctionare required. The wallet init delegatesAuthenticationController:getBearerTokento the service messenger; hosts must registerAuthenticationControlleron the supplied root messenger before authenticated Claims API calls succeed.ClaimsController — stateful controller with no additional constructor options beyond optional persisted
state. The wallet init delegates only the ClaimsService actions the controller calls (fetchClaimsConfigurations,getRequestHeaders,getClaimsApiUrl,generateMessageForClaimSignature,getClaims) plusKeyringController:signPersonalMessage.Wallet release work is intentionally deferred; this PR lands the integration and documents consumer options only.
References
Per-environment options
claimsService.envEnv.DEV/Env.UAT/Env.PRD)Env.DEV/Env.UAT/Env.PRD)Env.PRDin wallet-cli;Env.DEVin wallet unit testsclaimsService.fetchFunctionfetchfetchglobalThis.fetchclaimsControllerstatestateblob when suppliedstateblob when suppliedAuthenticationController:getBearerTokenAuthenticationControlleron the wallet root messenger (not aninstanceOptionsslot)AuthenticationControlleron the wallet root messenger (not aninstanceOptionsslot)Verification
yarn workspace @metamask/wallet run testyarn workspace @metamask/wallet run jest --no-coverage src/initialization/instances/claims-service/claims-service.test.ts src/initialization/instances/claims-controller/claims-controller.test.tsyarn workspace @metamask/wallet run messenger-action-types:checkyarn workspace @metamask/wallet run lint:tsconfigsyarn workspace @metamask/wallet-cli run lint:tsconfigsyarn readme-content:checkyarn codeowners:checkyarn lint:teamsChecklist
Made with Cursor