feat: eth_accounts return all permitted accounts

[jiexi]

Development & PR Process

1. Follow MetaMask Mobile Coding Standards
2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
3. Add needs-dev-review label when work is completed
4. Add needs-qa label when dev review is completed
5. Add QA Passed label when QA has signed off

Description

• Return all permitted accounts from eth_accounts with most recently used first for browser and SDK

This change was made to align with the change suggested in extension. The goal is start returning all accounts on each eth_accounts call rather than only returning an array with a single account. We achieve this by updating the restrictReturnedAccounts decorator

Screenshots/Recordings

Screen.Recording.2023-06-07.at.4.12.55.PM.mov

NOTE: test dapp page changes are not deployed yet, but have been merged into the main branch and be run locally to reproduce the recording above.

Screen.Recording.2023-06-07.at.4.30.05.PM.mov

Issue

Ticket: mme-18515
Documentation PR

Checklist

• There is a related GitHub issue
• Tests are included if applicable
• Any added code is fully documented

[github-actions]

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.

[jiexi]

I have read the CLA Document and I hereby sign the CLA

[jiexi]

I'm unsure if my assumptions are correct for eth_accounts behavior:

• Wallet Connect does not support multiple permitted accounts, and thus can only return the selectedAddress
• SDK does support multiple permitted accounts, and thus behaves the same as browser

[jiexi]

Reverted sdk and wc behavior based on discussion with @andreahaku

[Cal-L]

left a comment

[Cal-L]

LGTM

[cortisiko]

hey @jiexi I have a couple of questions:

Did you test removing accounts that were connected to a dapp?
Did you test importing accounts while one of your previous accounts was connected to a dapp?
Did you test connecting and disconnecting hardware wallets while connected to a dapp?
Was this tested against the portfolio dapp?
WC v2 was merged a couple of days ago, was this tested on the most recent main commit?

[jiexi]

@cortisiko

Did you test removing accounts that were connected to a dapp?
https://github.com/MetaMask/metamask-mobile/assets/918701/8a9add6f-8f57-46e0-b5af-be6b855d3893

Did you test importing accounts while one of your previous accounts was connected to a dapp?
https://github.com/MetaMask/metamask-mobile/assets/918701/ed4b203c-08c8-4769-bccb-7d9b2e808e43

Did you test connecting and disconnecting hardware wallets while connected to a dapp?
No. Don't have access to one I can use currently

Was this tested against the portfolio dapp?
Yes, I'm not sure what the correct behavior is. Seems like revoking account access doesn't remove them from the tracked accounts. But adding multiple accounts does get all the accounts to show up in the dapp on initial connect. Adding multiple accounts after the fact only gets one account added. To get the other account to show up, you have to switch and make them the active dapp. I believe this is a problem with the portfolio dapp itself specifically pulling only the active account when permissions change.

EDIT:

This issue doesn't occur in extension partly because it's not possible to permit multiple accounts via wallet ui after initial account connection. Bug filed in slack. See conversation for video proof as well

WC v2 was merged a couple of days ago, was this tested on the most recent main commit?
Yes. Just getting the error from not having WALLET_CONNECT_PROJECT_ID set

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[cortisiko]

Approving! I went through some manual verifications and I did not find any issues.