-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: add Solana accountsChanged event support #30949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
7ec7603 to
a3ed385
Compare
48cbf3a to
f246120
Compare
f286ac9 to
a92a66e
Compare
|
❌ Multichain API Spec Test Failed. View the report here. |
a92a66e to
f8e23ae
Compare
|
❌ Multichain API Spec Test Failed. View the report here. |
444a28b to
86f636f
Compare
|
❌ Multichain API Spec Test Failed. View the report here. |
Builds ready [27cc99c]
Page Load Metrics (3345 ± 1591 ms)
|
|
❌ Multichain API Spec Test Failed. View the report here. |
Builds ready [10b079d]
Page Load Metrics (6128 ± 3591 ms)
|
Builds ready [10b9cc5]
Page Load Metrics (4146 ± 1668 ms)
|
| @@ -0,0 +1,11 @@ | |||
| export enum KnownSessionProperties { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to export this from the @metamask/chain-agnostic-permission package, can do as a fast follow: MetaMask/core#5522
Builds ready [6317531]
Page Load Metrics (3735 ± 2074 ms)
|
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if ( | ||
| parsedSolanaAddresses.includes(account.address) && | ||
| originsWithSolanaAccountChangedNotifications[origin] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor performance optimization if we move this check to the top of this loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 we need the parsedSolanaAddresses to be parsed first though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if originsWithSolanaAccountChangedNotifications[origin] is false, then we don't need to do parsedSolanaAddresses and its includes checks in the first place
app/scripts/migrations/148.ts
Outdated
| if (!hasProperty(caveat.value, 'sessionProperties')) { | ||
| caveat.value.sessionProperties = {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay if we add a break in here? or use find instead of the for loop? definitely a nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
tested manually. looks great |
Builds ready [bbcfb23]
Page Load Metrics (3297 ± 852 ms)
|
Description
Adds support for
metamask_accountChangednotification viawallet_notify(CAIP-319) notification to enable our wallet-standard implementation to forward account changed events via the wallet standard change eventThis is not a pattern we want to continue since the Multichain API should not require account changing. This is a one off bespoke implementation to support a Solana wallet-standard interface.
E2E testing for this feature should be handled in the Wallet Standard powered test dapp one we have it cc @EdouardBougon @baptiste-marchand
Demo video
Screen.Recording.2025-03-25.at.1.38.28.PM.mov
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4193
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist