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: sort wallets [OTE-825] #1063

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

yogurtandjam
Copy link
Contributor

@yogurtandjam yogurtandjam commented Sep 25, 2024

sorts wallets according to spec https://linear.app/dydx/issue/OTE-825/sort-for-wallets-show-default-wallets-and

  • metamask

  • phantom

  • keplr
    ^ these three will go first and default to a download link if the extension isn't already installed

  • walletconnect

  • coinbase

  • email or social (privy)

  • coinbase

  • okx wallet

    • if the okx wallet is not installed, we'll still show the logo and just route it through the walletconnect option. if it is installed, we'll display as part of injected wallets. this isn't exactly how it was outlined in the spec so will want to get confirmation that this is acceptable behavior
image

https://www.loom.com/share/01711f85a43045fc9e252f91ccf3b352

Copy link

linear bot commented Sep 25, 2024

Copy link

vercel bot commented Sep 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
v4-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 7:43pm
v4-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 7:43pm

@yogurtandjam yogurtandjam force-pushed the jerms+tina/OTE-825_sort-wallets-and-show-default branch from 933ca3b to 1e73dcf Compare September 25, 2024 19:23
Comment on lines 63 to 75
const metamaskWallet = injectedMetaMask
? {
connectorType: ConnectorType.Injected,
icon: injectedMetaMask.detail.info.icon,
name: injectedMetaMask.detail.info.name,
rdns: injectedMetaMask.detail.info.rdns,
}
: {
connectorType: ConnectorType.DownloadWallet,
name: WalletType.MetaMask,
downloadLink: METAMASK_DOWNLOAD_LINK,
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const metamaskWallet = injectedMetaMask
? {
connectorType: ConnectorType.Injected,
icon: injectedMetaMask.detail.info.icon,
name: injectedMetaMask.detail.info.name,
rdns: injectedMetaMask.detail.info.rdns,
}
: {
connectorType: ConnectorType.DownloadWallet,
name: WalletType.MetaMask,
downloadLink: METAMASK_DOWNLOAD_LINK,
};
const metamaskWallet = injectedMetaMask ?? {
connectorType: ConnectorType.DownloadWallet,
name: WalletType.MetaMask,
downloadLink: METAMASK_DOWNLOAD_LINK,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

injectedMetaMask isn't the right format D: it needs to go through the same transformation that we have in the .map function

const enabledInjectedWallets = injectedWallets
.filter(
(wallet) =>
// Remove Metamask. We will show it no matter what at the front of
Copy link
Contributor

Choose a reason for hiding this comment

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

fix grammar? 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lmao, this comment got mangled 😆 what happened to the second line?!

@yogurtandjam yogurtandjam force-pushed the jerms+tina/OTE-825_sort-wallets-and-show-default branch from bab5d61 to 32aace9 Compare September 25, 2024 19:37
@yogurtandjam yogurtandjam marked this pull request as ready for review September 25, 2024 19:37
@yogurtandjam yogurtandjam requested a review from a team as a code owner September 25, 2024 19:37
@yogurtandjam yogurtandjam enabled auto-merge (squash) September 25, 2024 19:38
@yogurtandjam yogurtandjam merged commit 93ad572 into main Sep 25, 2024
8 checks passed
@yogurtandjam yogurtandjam deleted the jerms+tina/OTE-825_sort-wallets-and-show-default branch September 25, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants