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

fix: appcontext isSnapInstalled checks #7808

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Sep 25, 2024

Description

Does what it says on the box - we were casting an object into a Boolean which means checks would always return true, and "snap just installed" checks would never evaluate to true, meaning unless you refresh the app, snap installation would never trigger non-EVM accounts derivation/fetch.

Issue (if applicable)

closes #7376

Risk

High Risk PRs Require 2 approvals

None, this was blatantly wrong

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Going from uninstalled snap to installed snap derives non-EVM accounts without needing a refresh

  • Going from installed snap to uninstalled snap still clears accounts and trigger a refetch of EVM accounts only

  • In Add Accounts modal, managing non-EVM chains when having the multichain snap installed is happy

  • Managing accounts when not having the multichain snap installed is still disabled

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

https://jam.dev/c/54a6c088-a295-4111-9fe6-5d17e0efc895

@gomesalexandre gomesalexandre requested a review from a team as a code owner September 25, 2024 13:24
@woodenfurniture
Copy link
Member

noted by @kaladinlight

Accounts/AddAccountModal.tsx: const isSnapInstalled = Boolean(useIsSnapInstalled())

@gomesalexandre
Copy link
Contributor Author

noted by @kaladinlight

Accounts/AddAccountModal.tsx: const isSnapInstalled = Boolean(useIsSnapInstalled())

Here she is! 9c1cfcb

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Works flawlessly.

✅ Going from uninstalled snap to installed snap derives non-EVM accounts without needing a refresh

✅ Going from installed snap to uninstalled snap still clears accounts and trigger a refetch of EVM accounts only

✅ In Add Accounts modal, managing non-EVM chains when having the multichain snap installed is happy

Matching prod behaviour in that account 0 can be "managed". but new accounts can't be added.

Screenshot 2024-09-26 at 08 47 02

✅ Managing accounts when not having the multichain snap installed is still disabled

@0xApotheosis 0xApotheosis merged commit b1bbb76 into develop Sep 25, 2024
6 checks passed
@0xApotheosis 0xApotheosis deleted the fix_snap_appcontext_checks branch September 25, 2024 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MM snaps says installed but only EVMs are showing in wallet
3 participants