-
Notifications
You must be signed in to change notification settings - Fork 5.4k
fix: multichain support on dapp connected site popover cp-13.5.0 #36881
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. |
✨ Files requiring CODEOWNER review ✨👨🔧 @MetaMask/core-extension-ux (3 files, +64 -32)
|
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [3abb653]
UI Startup Metrics (1238 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
3abb653
to
1255491
Compare
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [3f10837]
UI Startup Metrics (1247 ± 68 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [98c125e]
UI Startup Metrics (1260 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Just checked and on main requesting via the solana wallet standard only gets a solana scope permission Screen.Recording.2025-10-15.at.3.23.54.PM.movBut on your PR we're getting all EVM chains as well: Screen.Recording.2025-10-15.at.3.28.19.PM.movso there is a regression. It seems the regression in the other direction (request via EVM provider and get EVM plus Solana) is already on main |
@adonesky1 Thank you for testing it carefully! 🙏 I just tested it again and it is working correctly, I'm pretty sure the reason it was not working correctly for you is because of the E2E Test Dapp using the same domain as solana test dapp. I saw on your video when you switched tab just for a moment (very helpful mistake! haha) you were already connected to the EVM network on E2E Test Dapp, so this is the reason why EVM networks were included in the Solana Dapp request. I was able to reproduce the same behaviour on main |
Description
This PR fixes issue with the DaPP permissions icon showing wrong network for Solana only DaPP.
Changelog
CHANGELOG entry: Fixed a bug with the DaPP permissions icon showing wrong network for Solana only DaPP
Related issues
Fixes: #36825
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Refactors active network detection to be multichain-aware (EVM and Solana) via a selector, updates the connected site popover to use it, adds safeguards, and adjusts/extends tests and fixtures.
getDappActiveNetwork
: Derives active network from ordered connected accounts; supports EVM and non‑EVM (Solana) via scopes; returns{ ...network, isEvm }
; usesgetMultichainNetworkConfigurationsByChainId
.getOrderedConnectedAccountsForActiveTab
; updates tests for new logic.getMetaMaskAccountsOrdered
: Safeguards spreading whenaddress
missing.ConnectedSitePopover
: Replaces manual domain-based lookup withgetDappActiveNetwork
; removesuseMemo
and unused selectors.ui/selectors/dapp.test.ts
) covering EVM, Solana, and null paths.domains
,selectedNetworkClientId
,keyrings.metadata
,internalAccounts
,permissionHistory
, multichain configs/accounts).mock-send-state.json
and onboarding data.Written by Cursor Bugbot for commit 98c125e. This will update automatically on new commits. Configure here.