This repository has been archived by the owner on Oct 7, 2024. It is now read-only.
feat: handle approval when adding/removing account #99
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
owencraston
force-pushed
the
feature/account-dialog
branch
from
e1c00d6 to
452c039
Compare
8 tasks
danroc
changed the title
owencraston
force-pushed
the
feature/account-dialog
branch
from
158fe71 to
018df68
Compare
owencraston
force-pushed
the
feature/account-dialog
branch
5 times, most recently
from
df65443 to
151bb41
Compare
owencraston
force-pushed
the
feature/account-dialog
branch
2 times, most recently
from
2e53a89 to
186ceda
Compare
owencraston
force-pushed
the
feature/account-dialog
branch
from
9a9054b to
b36ebaf
Compare
gantunesr
approved these changes
Sep 26, 2023
plasmacorral
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Sep 29, 2023
## Explanation Progresses MetaMask/accounts-planning#14 Designs: https://www.figma.com/file/XzaR4r04pMXIE8ft1T2hmJ/Add%2Fremove-snap-accounts%2Fkeyring-snaps-%2313-and-%2314?type=design&node-id=604-2586&mode=design - this PR works in conjunction with changes in the [eth-snap-keyring](MetaMask/eth-snap-keyring#99) to intercept account additions from a snap and get approval from the user. - This PR heavily leverages existing patterns with the approval controller/confirmations flow and for that reason, differs slightly from the original designs. - Since we are waiting on the [accounts controller PR](#20344) to support custom names, I have removed the TextField from the creation screen. - I have also opted to use the out of the box approval success/error screens with custom messages instead of the designs that @raulmono provided above. These standardized success/error screens are almost the same as the designs but lack the description and translations from the design. It is not currently possible to have custom success/error screens without a massive overhaul to the confirmations flow which would effect many teams. If changes are needed we should work with @matthewwalsh0 to make those changes upstream at a later date. ## Screenshots/Screencaps <!-- If you're making a change to the UI, make sure to capture a screenshot or a short video showing off your work! --> <img width="384" alt="Screenshot 2023-09-15 at 2 46 30 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7d119dc5-6a7c-4e83-ae9d-52390217a52d"> <img width="363" alt="Screenshot 2023-09-15 at 4 57 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/f20a63a6-9e16-40f7-b269-d697424454f0"> <img width="371" alt="Screenshot 2023-09-15 at 4 57 17 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/c00c2baa-8aa4-403f-aec1-7164a103792c"> <img width="370" alt="Screenshot 2023-09-20 at 3 54 27 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/8adc01ed-6138-43e7-aa93-7514d35f8a5e"> <img width="367" alt="Screenshot 2023-09-20 at 3 54 34 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/b246f7a6-05b1-4f5c-a2e5-d90b99310588"> ### Before https://github.com/MetaMask/metamask-extension/assets/22918444/465e7833-6ed1-4c38-b618-81723ed54928 ### After #### Happy Path https://github.com/MetaMask/metamask-extension/assets/22918444/049566e1-a121-4803-9166-1f26de2eba6f #### Error case https://github.com/MetaMask/metamask-extension/assets/22918444/fb628db4-4781-4bab-9efd-b755e011ff26 ## Manual Testing Steps This change requires the manual installing of the snaps rpc methods handlers and building metamask flask. Here are the steps. #### Setup 1. checkout this branch and run `yarn start --build-type flask` 2. in your browser, load the extension by clicking `Load unpacked` in the extension settings and using the `dist/chrome/` (or whatever browser you are using) as the source 3. create your account and login #### Testing the snap account creation 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 4. connect the dapp 5. click create account. Custom account names are not supported at this time <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 6. click create 7. a new account should be created on the snap 8. open metamask and click the accounts dropdown, a new snap account should be added #### Testing the denial flow 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 2. connect the dapp 3. click create account <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 4. A modal should open on extension 5. click cancel 9. the modal should close and the dapp should throw an error `User denied account creation` <img width="1158" alt="Screenshot 2023-09-20 at 4 14 25 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7eee4396-a762-4c97-a680-2e03b358596d"> #### Testing the error case - luckily for us, the snap simple keyring had a bug in version 0.2.1 that we can use to test this flow. To test the error flow, use the following steps 1. open this test snap: https://metamask.github.io/snap-simple-keyring/0.2.1/ 2. connect dapp (uninstall and reinstall if you were using a different version before) 3. click create account 4. the account creation modal should pop up 5. click create, an error should be shown with the message `Method not supported: event:accountCreated` 6. create another account 10. click cancel, the modal should close and the dapp should throw an error `User denied account addition` #### Testing account removal 1. follow the previous mentioned steps to create 2 or more snap accounts with this [snap](https://metamask.github.io/snap-simple-keyring/latest/) 2. copy the account `id` from the side bar and paste it into the remove account section of the snap 3. click remove account <img width="1184" alt="Screenshot 2023-09-20 at 4 17 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e8fb4891-a59d-436f-995c-02d145e1b3e9"> 4. A modal should show up indicating what you are trying to do 5. click the `share` icon beside the public address and it should open that account on etherscan. It should work with different networks as well. 6. click `remove` 7. a success screen should be shown and the snap should update with the state and the account no longer visible in the side bar 8. open the extension and verify that the account is no longer in the account list 9. repeat steps 1 and 2 10. this time click cancel 11. the extension should close and an error on the snap should appear with the text `User denied account removal` <img width="1168" alt="Screenshot 2023-09-20 at 4 20 03 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e85e8ab1-aec0-4ac7-8a56-5253c82af53b"> 12. verify that the account was not removed from the extension or the snap ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required. --------- Co-authored-by: Monte Lai <[email protected]> Co-authored-by: Matthew Walsh <[email protected]> Co-authored-by: Daniel Rocha <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>
k-g-j
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Nov 1, 2023
Progresses https://github.com/MetaMask/accounts-planning/issues/14 Designs: https://www.figma.com/file/XzaR4r04pMXIE8ft1T2hmJ/Add%2Fremove-snap-accounts%2Fkeyring-snaps-%2313-and-%2314?type=design&node-id=604-2586&mode=design - this PR works in conjunction with changes in the [eth-snap-keyring](MetaMask/eth-snap-keyring#99) to intercept account additions from a snap and get approval from the user. - This PR heavily leverages existing patterns with the approval controller/confirmations flow and for that reason, differs slightly from the original designs. - Since we are waiting on the [accounts controller PR](#20344) to support custom names, I have removed the TextField from the creation screen. - I have also opted to use the out of the box approval success/error screens with custom messages instead of the designs that @raulmono provided above. These standardized success/error screens are almost the same as the designs but lack the description and translations from the design. It is not currently possible to have custom success/error screens without a massive overhaul to the confirmations flow which would effect many teams. If changes are needed we should work with @matthewwalsh0 to make those changes upstream at a later date. <!-- If you're making a change to the UI, make sure to capture a screenshot or a short video showing off your work! --> <img width="384" alt="Screenshot 2023-09-15 at 2 46 30 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7d119dc5-6a7c-4e83-ae9d-52390217a52d"> <img width="363" alt="Screenshot 2023-09-15 at 4 57 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/f20a63a6-9e16-40f7-b269-d697424454f0"> <img width="371" alt="Screenshot 2023-09-15 at 4 57 17 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/c00c2baa-8aa4-403f-aec1-7164a103792c"> <img width="370" alt="Screenshot 2023-09-20 at 3 54 27 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/8adc01ed-6138-43e7-aa93-7514d35f8a5e"> <img width="367" alt="Screenshot 2023-09-20 at 3 54 34 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/b246f7a6-05b1-4f5c-a2e5-d90b99310588"> https://github.com/MetaMask/metamask-extension/assets/22918444/465e7833-6ed1-4c38-b618-81723ed54928 https://github.com/MetaMask/metamask-extension/assets/22918444/049566e1-a121-4803-9166-1f26de2eba6f https://github.com/MetaMask/metamask-extension/assets/22918444/fb628db4-4781-4bab-9efd-b755e011ff26 This change requires the manual installing of the snaps rpc methods handlers and building metamask flask. Here are the steps. 1. checkout this branch and run `yarn start --build-type flask` 2. in your browser, load the extension by clicking `Load unpacked` in the extension settings and using the `dist/chrome/` (or whatever browser you are using) as the source 3. create your account and login 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 4. connect the dapp 5. click create account. Custom account names are not supported at this time <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 6. click create 7. a new account should be created on the snap 8. open metamask and click the accounts dropdown, a new snap account should be added 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 2. connect the dapp 3. click create account <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 4. A modal should open on extension 5. click cancel 9. the modal should close and the dapp should throw an error `User denied account creation` <img width="1158" alt="Screenshot 2023-09-20 at 4 14 25 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7eee4396-a762-4c97-a680-2e03b358596d"> - luckily for us, the snap simple keyring had a bug in version 0.2.1 that we can use to test this flow. To test the error flow, use the following steps 1. open this test snap: https://metamask.github.io/snap-simple-keyring/0.2.1/ 2. connect dapp (uninstall and reinstall if you were using a different version before) 3. click create account 4. the account creation modal should pop up 5. click create, an error should be shown with the message `Method not supported: event:accountCreated` 6. create another account 10. click cancel, the modal should close and the dapp should throw an error `User denied account addition` 1. follow the previous mentioned steps to create 2 or more snap accounts with this [snap](https://metamask.github.io/snap-simple-keyring/latest/) 2. copy the account `id` from the side bar and paste it into the remove account section of the snap 3. click remove account <img width="1184" alt="Screenshot 2023-09-20 at 4 17 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e8fb4891-a59d-436f-995c-02d145e1b3e9"> 4. A modal should show up indicating what you are trying to do 5. click the `share` icon beside the public address and it should open that account on etherscan. It should work with different networks as well. 6. click `remove` 7. a success screen should be shown and the snap should update with the state and the account no longer visible in the side bar 8. open the extension and verify that the account is no longer in the account list 9. repeat steps 1 and 2 10. this time click cancel 11. the extension should close and an error on the snap should appear with the text `User denied account removal` <img width="1168" alt="Screenshot 2023-09-20 at 4 20 03 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e85e8ab1-aec0-4ac7-8a56-5253c82af53b"> 12. verify that the account was not removed from the extension or the snap - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required. --------- Co-authored-by: Monte Lai <[email protected]> Co-authored-by: Matthew Walsh <[email protected]> Co-authored-by: Daniel Rocha <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
works alongside this PR in the extension: MetaMask/metamask-extension#20684
follow steps outlined in the linked extension PR to test
Examples