metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard
verified publisher icon MetaMask

feat: sdk permissions system integration

Open abretonc7s opened this issue 2 years ago • 4 comments

Description

Changes to include permissions system within sdk workflows (js + android). WalletConnect will require a separate PR and migrate away from persisting connection in DefaultPreferences.

  • new connection modal flow (allow multiple account selection)
  • new sessions management ui

Design: https://www.figma.com/file/piME5CaTYUz07Gc7Ye7c22/Deeplinking--SDK?type=design&node-id=1633-2285&mode=design&t=egikpsEVgt3xcMNg-0

https://app.zenhub.com/workspaces/metamask-mobile-5f984938ddc0e4001d4b79cb/issues/gh/metamask/mobile-planning/868

  • added online indication in connection list

Related issues

Fixes:

Manual testing steps

  • See QA comments

Screenshots/Recordings

Before

After

image image

Pre-merge author checklist

  • [x] I’ve followed MetaMask Coding Standards.
  • [x] I've clearly explained what problem this PR is solving and how it is solved.
  • [x] I've linked related issues
  • [x] I've included manual testing steps
  • [x] I've included screenshots/recordings if applicable
  • [x] I’ve included tests if applicable
  • [x] I’ve documented my code using JSDoc format if applicable
  • [x] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • [x] I’ve properly set the pull request status:
    • [x] In case it's not yet "ready for review", I've set it to "draft".
    • [x] In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

abretonc7s avatar Feb 06 '24 03:02 abretonc7s

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.

github-actions[bot] avatar Feb 06 '24 03:02 github-actions[bot]

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b989d065-bb5f-4484-810e-522bce54a6cf You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

github-actions[bot] avatar Feb 08 '24 14:02 github-actions[bot]

Codecov Report

Attention: Patch coverage is 21.07280% with 206 lines in your changes are missing coverage. Please review.

Project coverage is 41.35%. Comparing base (e9a1442) to head (05cf76c). Report is 1 commits behind head on main.

Files Patch % Lines
...mponents/Views/SDKSessionModal/SDKSessionModal.tsx 5.40% 35 Missing :warning:
...ts/Views/SDKDisconnectModal/SDKDisconnectModal.tsx 5.88% 32 Missing :warning:
app/core/BackgroundBridge/BackgroundBridge.js 0.00% 24 Missing :warning:
...s/SDKSessionsManager/SDKSessionAccountListItem.tsx 0.00% 23 Missing :warning:
app/core/RPCMethods/RPCMethodMiddleware.ts 22.72% 15 Missing and 2 partials :warning:
...onents/Views/SDKSessionsManager/SDKSessionItem.tsx 0.00% 14 Missing :warning:
app/core/SDKConnect/handlers/handleSendMessage.ts 67.56% 10 Missing and 2 partials :warning:
...ts/Views/SDKSessionsManager/SDKSessionsManager.tsx 8.33% 11 Missing :warning:
app/core/SDKConnect/SDKConnect.ts 27.27% 8 Missing :warning:
...components/Views/AccountConnect/AccountConnect.tsx 0.00% 6 Missing :warning:
... and 10 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8531      +/-   ##
==========================================
- Coverage   41.54%   41.35%   -0.19%     
==========================================
  Files        1268     1271       +3     
  Lines       30664    30804     +140     
  Branches     3011     3028      +17     
==========================================
+ Hits        12738    12740       +2     
- Misses      17172    17304     +132     
- Partials      754      760       +6

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 09 '24 07:02 codecov-commenter

Important notes:

  • Permission system on WalletConnect has been left aside for a second stage so for the scope of this PR it was verified that the way a user connects to a WalletConnet dapp remains the same
  • A bug was found on the WalletConnect integration related to the accountsSync on metamask_accountsChanged after a rebase with main. Before merging it was verified that the issue is present on main and the root cause was not this PR (see video 1 below)
  • For the SDK context, developers usually refer to ethereum.selectedAddress so both the funcionality of wallet_getPermissions and the normal functionality of updating the ethereum.selectedAddress were checked
  • On this first stage, the "Manage Connections" screen in MetaMask (security & privacy) only contemplates the removal of: A single dapp connection; Removal of one account from a connection|; Removal of all connections from all dapps

Video 1 (a new issue will be created with it) https://github.com/MetaMask/metamask-mobile/issues/8706:

https://github.com/MetaMask/metamask-mobile/assets/104831203/4a16a109-7700-4fc6-8a74-06f27f83f4d5

Error:

ERROR  {"context": "client"} No matching key. session: 45e54d0244a9670d8d918f2f523bf2ec7bd7d468175cb841fb26c5b07d46fc92
 WARN  WC2::updateSession can't update session topic=45e54d0244a9670d8d918f2f523bf2ec7bd7d468175cb841fb26c5b07d46fc92 [Error: No matching key. session: 45e54d0244a9670d8d918f2f523bf2ec7bd7d468175cb841fb26c5b07d46fc92]

Dapps and platforms used for testing:

  • Android SDK native dapp
  • WalletConnect
  • iOS SDK
  • JS SDK: mix between node, reactNative and webdapp

Test Cases Connect one account

  1. Open dapp
  2. connect 1 account
  3. dapp gets 1 account
  4. Switching to an account without permissions does not sync the account to the dapp

Connect multiple accounts

  1. open dapp
  2. connect multiple accounts
  3. dapp gets the list of approved accounts
  4. switching to an account with permissions
    1. selectedAddress is changed to the selected one
    2. signing from dapp promps a sign to the selected address that should be the one selected on the wallet side

Explore permissions

  1. Open dapp
  2. connect multiple accounts
  3. dapp gets the list of approved accounts
  4. switching to an account without persmissions
    1. selected address remains the same
    2. signing prompts to sign with the selected address (not the one selected in the wallet) -> ethereum.selectedAddress

Update permissions

  1. open dapp
  2. connect 2 accounts
  3. call requestPermissions
  4. remove the permission to one of the accounts
    1. Dapp gets the updated accounts. The account that was removed is not showing

WalletConnect

  1. Open a WC2 dapp
  2. connect one or more accounts
  3. selectedAddress becomes accounts[0]
  4. The behaviour of the WC Dapp remains the same as it was

Manage Connections screen

https://github.com/MetaMask/metamask-mobile/assets/104831203/acec7460-60be-46d1-a293-c4e9361e0643

JS Overview (QR Code)

https://github.com/MetaMask/metamask-mobile/assets/104831203/62302032-9a94-4a31-9096-daf190d6f475

JS Connection (deeplink)

https://github.com/MetaMask/metamask-mobile/assets/104831203/df177c93-2b78-4ac8-af0d-7cc3acb6a39b

WalletConnect

https://github.com/MetaMask/metamask-mobile/assets/104831203/1b586258-d945-4700-8fa6-cd65f0af0e34

christopherferreira9 avatar Feb 23 '24 14:02 christopherferreira9

Quality Gate Passed Quality Gate passed

Issues
4 New issues

Measures
0 Security Hotspots
14.5% Coverage on New Code
2.9% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Feb 27 '24 00:02 sonarqubecloud[bot]