-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add support for accountChanged for Tron network #23639
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. |
45dffc0 to
33e253f
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
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.
Bug: Missing unsubscribe calls for Tron event handlers
The onDisconnect method unsubscribes the Solana event handlers but does not unsubscribe the newly added Tron event handlers (handleTronAccountChangedFromScopeChanges, handleTronAccountChangedFromSelectedAccountChanges, handleTronAccountChangedFromSelectedAccountGroupChanges). These handlers are subscribed in setupCaipEventSubscriptions() but never cleaned up, causing a resource leak where the handlers continue responding to events after the bridge is disconnected.
app/core/BackgroundBridge/BackgroundBridge.js#L503-L521
metamask-mobile/app/core/BackgroundBridge/BackgroundBridge.js
Lines 503 to 521 in 33e253f
| // Enable multichain functionality for all connections except for WalletConnect and MMSDK v1. | |
| if (!(this.isMMSDK && this.sdkVersion === 'v1') && !this.isWalletConnect) { | |
| controllerMessenger.unsubscribe( | |
| `${PermissionController.name}:stateChange`, | |
| this.handleCaipSessionScopeChanges, | |
| ); | |
| controllerMessenger.unsubscribe( | |
| `${PermissionController.name}:stateChange`, | |
| this.handleSolanaAccountChangedFromScopeChanges, | |
| ); | |
| controllerMessenger.unsubscribe( | |
| `${AccountsController.name}:selectedAccountChange`, | |
| this.handleSolanaAccountChangedFromSelectedAccountChanges, | |
| ); | |
| controllerMessenger.unsubscribe( | |
| `${AccountTreeController.name}:selectedAccountGroupChange`, | |
| this.handleSolanaAccountChangedFromSelectedAccountGroupChanges, | |
| ); | |
| } |
app/core/BackgroundBridge/BackgroundBridge.js#L961-L978
metamask-mobile/app/core/BackgroundBridge/BackgroundBridge.js
Lines 961 to 978 in 33e253f
| // wallet_notify for tron accountChanged when permission changes | |
| controllerMessenger.subscribe( | |
| `${PermissionController.name}:stateChange`, | |
| this.handleTronAccountChangedFromScopeChanges, | |
| getAuthorizedScopes(this.channelIdOrOrigin), | |
| ); | |
| // wallet_notify for tron accountChanged when selected account changes | |
| controllerMessenger.subscribe( | |
| `${AccountsController.name}:selectedAccountChange`, | |
| this.handleTronAccountChangedFromSelectedAccountChanges, | |
| ); | |
| controllerMessenger.subscribe( | |
| `${AccountTreeController.name}:selectedAccountGroupChange`, | |
| this.handleTronAccountChangedFromSelectedAccountGroupChanges, | |
| ); |
33e253f to
695bc50
Compare
6c6fb83 to
2911f7a
Compare
|
@SocketSecurity ignore npm/@metamask/multichain-api-middleware@1.2.5 |
| // have listeners ready for this Solana accountChanged event below. | ||
| this.notifySolanaAccountChangedForCurrentAccount(); | ||
| ///: BEGIN:ONLY_INCLUDE_IF(tron) | ||
| this.notifyTronAccountChangedForCurrentAccount(); |
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 could include the same comment as above, for tron as well. Or even remove the above comment altogether since the method name is pretty self explanatory. This is not a blocking comment
| ); | ||
|
|
||
| ///: BEGIN:ONLY_INCLUDE_IF(tron) | ||
| // wallet_notify for tron accountChanged when permission changes |
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.
| // wallet_notify for tron accountChanged when permission changes | |
| // wallet_notify for Tron accountChanged when permission changes |
| getAuthorizedScopes(this.channelIdOrOrigin), | ||
| ); | ||
|
|
||
| // wallet_notify for tron accountChanged when selected account changes |
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.
| // wallet_notify for tron accountChanged when selected account changes | |
| // wallet_notify for Tron accountChanged when selected account changes |
|
Hey @EdouardBougon, been trying to test this but I'm unable to select unable-to-select-mainnet.mov |
|
Can't seem to get the app to register the account change, as the address never changes. Again, not sure if I'm missing something here. Clip below: accountChanged-tron.mov |
adonesky1
left a comment
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 and works, also tested Solana and EVM account changed events still working. Code looks good. We'll want to refactor later but not blocking for now
346ca3a to
f9f2ce7
Compare
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR introduces Tron blockchain support in the BackgroundBridge, which is a critical core component that handles communication between the app and dApps (browser, SDK, WalletConnect). The changes include:
The BackgroundBridge is imported by 14 files including BrowserTab, SDKConnect, WalletConnect, and DeeplinkProtocol - all critical paths for dApp interactions. Selected tags rationale:
The Tron code is feature-flagged (ONLY_INCLUDE_IF(tron)), but the package updates and refactoring of _notifySolanaAccountChange to _notifyMultichainAccountChange affect existing functionality. |
|



Description
After Solana, add the support for Tron account Change.
In parallel, a new PR is in progress to factorize this code and make it generic for all non-EVM chains: Solana, Tron, Bitcoin, etc.
Changelog
CHANGELOG entry: Add Tron Account Change detection support for multichain Api (#23639)
Related issues
Fixes:
Manual testing steps
You could try with our Tron test app here: https://metamask.github.io/test-dapp-tron/latest/
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds Tron accountChanged notifications to the Multichain provider, reusing a generalized notification helper, with new event subscriptions, tests, and dependency bumps.
app/core/BackgroundBridge/BackgroundBridge.js)handleTronAccountChangedFromScopeChanges,handleTronAccountChangedFromSelectedAccountChanges,handleTronAccountChangedFromSelectedAccountGroupChanges, andgetTronAccountFromSelectedAccountGroup.lastSelectedTronAccountAddressand emit initialnotifyTronAccountChangedForCurrentAccount()on provider setup._notifySolanaAccountChangewith_notifyMultichainAccountChange(scope, value)and use for Solana and Tron.getSolanaAccountFromSelectedAccountGroup()and update Solana flows accordingly.app/core/BackgroundBridge/BackgroundBridge.test.js)package.json,yarn.lock)@metamask/chain-agnostic-permissionto^1.3.0.@metamask/multichain-api-clientto^0.10.1and@metamask/multichain-api-middlewareto1.2.5.Written by Cursor Bugbot for commit 3d95aa3. This will update automatically on new commits. Configure here.