Skip to content
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

feat: custom names for snap accounts (Flask only) #12198

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Nov 6, 2024

Description

This PR allows enables the following...

  1. Keyring snaps to suggest account names
  2. Users to set custom account names during the snap account creation flow
  3. deny adding an account

This will be used to create Bitcoin accounts in the future.

Equivalent extension PR: MetaMask/metamask-extension#25191

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/604

Manual testing steps

  1. open .js.env in your editor and set export METAMASK_BUILD_TYPE="flask"
  2. run the app and build the create a wallet
  3. in the in app browser open this url: https://metamask.github.io/snap-simple-keyring/latest/
  4. click connect and approve the permissions and install confirmation
  5. now that you are connected, click the create account button in the dapp
  6. EXPECT: a popup with the suggested account name should appear. Assuming this is your first snap account the suggested name should be SSK Account
  7. click Add account
  8. EXPECT: SSK Account should be added to the account list in the wallet view and be the currently selected account.
  9. navigate back to the browser and click the create account button again.
  10. EXPECT: a new popup should appear with the suggested name being SSK Account 1
  11. If you remove the 1 from SSK Account 1 you should see a an error popup on the screen saying This account name already exists AND the Add account button should be disabled.
  12. Change the text input value to something unique and click Add account
  13. again this account should be added to the wallet and set as the currently selected account.

Testing Add account denial

  1. open .js.env in your editor and set export METAMASK_BUILD_TYPE="flask"
  2. run the app and build the create a wallet
  3. in the in app browser open this url: https://metamask.github.io/snap-simple-keyring/latest/
  4. click connect and approve the permissions and install confirmation
  5. now that you are connected, click the create account button in the dapp
  6. EXPECT: a popup with the suggested account name should appear
  7. click cancel
  8. EXPECT: the an error should appear in the SSK dapp indicating that the user denied the account creation and the account should not have been added to metamask.

Screenshots/Recordings

Extension version

Screenshot 2024-11-06 at 3 47 12 PM

Mobile:

Screenshot 2024-11-06 at 3 47 12 PM

Before

Untitled.mov

After

Screen.Recording.2024-11-08.at.6.00.03.PM.mov

Pre-merge author checklist

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.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

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.

@owencraston owencraston force-pushed the feat/custom-snap-account-names branch 5 times, most recently from 1e6000f to d4172c9 Compare November 9, 2024 02:16
@owencraston owencraston marked this pull request as ready for review November 9, 2024 02:17
@owencraston owencraston requested review from a team as code owners November 9, 2024 02:17
@owencraston owencraston added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 9, 2024
Copy link
Contributor

github-actions bot commented Nov 9, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: d4172c9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2d84d488-5fba-4d0b-a0fc-4e00fb651e62

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@owencraston owencraston changed the title feat: custom names for snap accounts feat: custom names for snap accounts (Flask only) Nov 9, 2024
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 12, 2024
Copy link
Contributor

github-actions bot commented Nov 12, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 1954f47
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a3307ed7-40d0-4bde-93b0-86bbbee8644a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 12, 2024
Copy link
Contributor

github-actions bot commented Nov 12, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 202684f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d137c941-3fb7-49d4-86ef-d6badf075687

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

Copy link
Contributor

@Daniel-Cross Daniel-Cross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments around the usage of some optimisation hooks and an intense useEffect. Looking good though!

@owencraston owencraston force-pushed the feat/custom-snap-account-names branch 2 times, most recently from e292014 to dce728b Compare November 14, 2024 23:00
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 14, 2024
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left some minor comments

app/core/SnapKeyring/SnapKeyring.ts Outdated Show resolved Hide resolved
app/core/SnapKeyring/SnapKeyring.ts Outdated Show resolved Hide resolved
app/core/SnapKeyring/SnapKeyring.ts Show resolved Hide resolved
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 15, 2024
Copy link
Contributor

github-actions bot commented Nov 15, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 4722411
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4b788d88-0c09-4ce7-b738-037072dd14e7

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 15, 2024
Copy link
Contributor

github-actions bot commented Nov 15, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 0e3c1c7
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ece47cd3-1794-4bb1-99af-953a7be36d55

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 15, 2024
Copy link
Contributor

github-actions bot commented Nov 15, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: dd8c2fa
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b5d3a8b3-0ce8-4ad4-ae0c-3b3092e73346

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented Nov 15, 2024

@owencraston
Copy link
Contributor Author

confirmed it is working on the latest commit.

Screen.Recording.2024-11-15.at.3.40.24.PM.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants