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.

Sign up for GitHub

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

Jump to bottom

feat: new wallet flow first-class mipdish wallets #8610

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

feat: new wallet flow first-class mipdish wallets #8610

wants to merge 16 commits into from
+380 −161

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 17, 2025 •
edited
Loading

Description

Adds support for the following "first-class" wallets:

  • Keplr: detected as mipd since it supports both EVM chains and Cosmos SDK, but we only support Cosmos SDK for it. Regardless of whether or not we support its EVM capabilities (we don't), it still allows us to do installed detections for it, and then go as usual with connecting it as a Cosmos SDK wallet
  • Phantom: same as above. Detected as installed thanks to its EVM capabilities. We do support more than just EVM chains for it (i.e, ETH and BTC), but mipd allows us to do intalled checks
  • Coinbase: Added as first-class implementation just because that's how it currently is implemented, though we may want to change that with the new wallet flow. See https://discord.com/channels/554694662431178782/1329789642157064232/1329789646066155532

⚠️ Note: there is a slight change of heuristics as part of this PR (though, only with the new wallet flow flag on): only installed (detected) MIPD wallets are displayed, whether first-class or not.

Also, added a few cleanup/styling issues spotted by @NeOMakinG in #8594

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

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

Medium. As this is eventually going to replace the current wallet flow, test this as if it was hitting prod. Phantom, Coinbase and Keplr could be broken with the flag on.

Testing

Engineering

  • Coinbase, Keplr, Phantom and MM wallets are happy with the REACT_APP_FEATURE_NEW_WALLET_FLOW flag on
  • Coinbase, Keplr, Phantom and MM wallets are happy with the REACT_APP_FEATURE_NEW_WALLET_FLOW flag off

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • Regression test that Coinbase, Keplr and Phantom wallets are still happy (i.e with the flag off, which is what you're testing with as part of release)

Screenshots (if applicable)

  • Flag on

https://jam.dev/c/9e16855b-c50a-42f3-82ba-466c0543e2e7

  • Flag off

https://jam.dev/c/3abe0f97-2373-4088-9e8b-ca6de794bb02

@gomesalexandre gomesalexandre changed the title [skip ci] wip: first-class mipdish wallets [skip ci] wip: new wallet flow first-class mipdish wallets Jan 17, 2025
@gomesalexandre gomesalexandre force-pushed the feat_new_wallet_first_class branch from 0d45a8b to 4ccfe9f Compare January 17, 2025 13:29
Base automatically changed from feat_new_wallet_flow_native to develop January 17, 2025 13:59
@gomesalexandre gomesalexandre force-pushed the feat_new_wallet_first_class branch from 0f6d46f to c562ee4 Compare January 17, 2025 13:59
@gomesalexandre gomesalexandre changed the title [skip ci] wip: new wallet flow first-class mipdish wallets feat: new wallet flow first-class mipdish wallets Jan 17, 2025
gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/routes/NativeRoutes.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved around

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/routes/MipdRoutes.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved around, and added handling of first-class routes

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/types.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved around

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/sections/SavedWalletsSection.tsx
@@ -127,8 +127,6 @@ export const SavedWalletsSection = ({
history.push(NativeWalletRoutes.Connect)
}, [history])

if (!nativeVaultsQuery.data?.length) return null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty state was rugged

image

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/WalletProvider.tsx
Comment on lines +51 to +53
| ComponentWithAs<'svg', IconProps>
// RFC-2397
| `data:image/${string}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really leverage the latter anywhere hence the null ternaries, but noticed we were setting the wallet to MM icon (as in, actual MM icon) in MetaMask connect, vs. the actual mipd wallet (MM or another), which was obviously wrong.
Setting the correct icon for sanity's sake required updating this type.

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/routes/MipdRoutes.tsx
<Route path='/metamask/snap/install'>
<SnapInstall />
</Route>
<Route path='/metamask/snap/update'>
Copy link
Contributor Author

@gomesalexandre gomesalexandre Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, there is another change in addition to the first-class handling: /metamask/failure handling is now unhandled, as we don't push to it anymore in <FirstClassBody /> / <MipdBody />.

The rationale is there is no need for an explicit pairing failure step, errors are handled within the body itself

https://jam.dev/c/f6c73f5b-d68e-491a-a741-d7463cfe2870

gomesalexandre
gomesalexandre commented Jan 17, 2025
View reviewed changes
src/context/WalletProvider/NewWalletViews/routes/NativeRoutes.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved around

@gomesalexandre gomesalexandre marked this pull request as ready for review January 17, 2025 14:34
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 17, 2025 14:34
@gomesalexandre gomesalexandre mentioned this pull request Jan 17, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New wallet flow - first class EVM/ish wallets
1 participant
@gomesalexandre