feat: shield controller wire in wallet package - #9616
Conversation
![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 0fc9607. Configure here.
| transactionController?: TransactionControllerInstanceOptions; | ||
| passkeyController?: PasskeyControllerInstanceOptions; | ||
| seedlessOnboardingController?: SeedlessOnboardingControllerInstanceOptions; | ||
| shieldController: ShieldControllerInstanceOptions; |
There was a problem hiding this comment.
Why shieldController is required type here?
There was a problem hiding this comment.
Most controllers/services are optional here.
There was a problem hiding this comment.
shieldController is required because it follows the same rule as the other required slots.
Needs baseUrl + fetchFunction, or an injected backend.
initialize() always creates ShieldController (it's in defaultConfigurations). Making the slot required forces every Wallet consumer to explicitly provide Shield connectivity config, similar to gasFeeController.clientId.
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?
…pdate dependencies
…on and tsconfig files
…ove signature_controller link from wallet
…on and update backend options
| storage: new InMemoryStorageAdapter(), | ||
| }, | ||
| remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, | ||
| shieldController: { |
There was a problem hiding this comment.
this should no longer be needed
There was a problem hiding this comment.
shieldController option is still required because of required fetchFunction: globalThis.fetch, do you think we should default this option ?
There was a problem hiding this comment.
Not quite true: fetchFunction is not required when a backend is supplied. The type is a union: either provide backend, or provide fetchFunction so a backend can be created. The test currently provides both, but fetchFunction is ignored because initialization uses:
options.backend ?? createShieldRemoteBackend(...)So this would be sufficient:
shieldController: {
backend: {
checkCoverage: jest.fn(),
checkSignatureCoverage: jest.fn(),
logSignature: jest.fn(),
logTransaction: jest.fn(),
},
},
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Nice, just had one immediate comment and one future comment.
| env = Env.PRD, | ||
| ...options | ||
| }: CreateShieldRemoteBackendOptions): ShieldRemoteBackend { | ||
| return new ShieldRemoteBackend({ |
There was a problem hiding this comment.
Hmm, why does this package need to export a convenience function to export this class? If we are initializing this class in @metamask/wallet then do we need this?
| state, | ||
| backend: | ||
| options.backend ?? | ||
| createShieldRemoteBackend({ |
There was a problem hiding this comment.
Similar to the comment I posted about SubscriptionController, it would be better if ShieldRemoteBackend were a data service class, so that it could access AuthenticationController:getBearerToken itself through the messenger (see ChompApiService for an example.
Also it looks like ShieldController takes ShieldRemoteBackend as an argument. If the service had its own messenger, then it would be better if the controller could use it to access the service itself (see ClaimsController and ClaimsService.
Again this could be a future change, but I wanted to point this out.
| }, | ||
| remoteFeatureFlagController: REMOTE_FEATURE_FLAG_OPTIONS, | ||
| }, | ||
| instanceOptions: getInstanceOptions(), |
There was a problem hiding this comment.
Instead of extracting getInstanceOptions, what do you think about having setupWallet take an option that allows for overriding options? Then the tests that use new Wallet could use setupWallet instead, and pass whatever is relevant to them (e.g. initializationConfigurations)?
5 participants
Explanation
Adds ShieldController as a default initialized unit in @metamask/wallet.
@metamask/wallet is the shared controller-integration layer for extension, mobile, and other clients. Build-specific and runtime values are injected via instanceOptions rather than hardcoded.
Breaking changes
References
Checklist
Note
High Risk
Breaking wallet API plus shield-controller backend options; hosts must supply shield options, register Authentication/Signature controllers, and start Shield explicitly—misconfiguration can break transaction/signature protection flows.
Overview
ShieldControlleris now a default@metamask/walletinstance, with a requiredinstanceOptions.shieldControllerslot (fetchFunctionplus optionalenv, backend override, and tuning). Initialization builds a default remote backend viacreateShieldRemoteBackend, delegatesAuthenticationController:getBearerTokenand transaction/signature:stateChangeevents, and does not auto-callShieldController:start.In
@metamask/shield-controller,ShieldRemoteBackend/ the factory takeenv(DEV/UAT/PRD) instead ofbaseUrl; URLs come fromgetShieldApiBaseUrl. The factory defaults auth toAuthenticationController:getBearerTokenon the shield messenger (new allowed action).profile-sync-controlleris added for bearer-token types.wallet-clipassesfetchFunctiononly (production env default). Tests and CODEOWNERS/dependency graphs are updated for the new wiring.Reviewed by Cursor Bugbot for commit 903aae9. Bugbot is set up for automated code reviews on this repo. Configure here.