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

Spike: Abstract wallet-dependant components/functionality #6446

Closed
gomesalexandre opened this issue Mar 13, 2024 · 2 comments
Closed

Spike: Abstract wallet-dependant components/functionality #6446

gomesalexandre opened this issue Mar 13, 2024 · 2 comments
Labels
needs architecture Needs architecture input before engineering

Comments

@gomesalexandre
Copy link
Contributor

gomesalexandre commented Mar 13, 2024

Overview

The high-level idea behind this issue is we have a set of common functionality implemented across the app WRT buttons that interact with wallet. This screenshot from @woodenfurniture sums it best:

image

We always have to:

  • Think about the "Connect Wallet" flow (spoiler alert: we never do and I can think of at least 5 "Connect Wallet" issues where we had to handle the no wallet connected flow)
  • Think of MetaMask/Ledger specifically
  • In the case of Ledger, we always have to warn users to open the app on their device first, or do some weird UX workarounds Ledger

What if we instead we had some dynamic component/s (e.g a button, but not limited to it) which would detect if/which wallet is connected, an do some checks before, returning a fallback in the case of no wallet connected, or in case an action is needed

What if we didn't have to think of those edge cases, but would have a UI library of sorts that handles this, and returns a different copy, also handling things like approvals needed and accepting functions that trigger whichever action is needed on click (e.g onApprove) or sane defaults for actions which are always the same (onConnectWalletClick).

This spike is an engineering spike on how to avoid the repetitiveness, disparity of duplicated code, and eventually make it so we don't have to repeat ourselves and miss edge cases, because it is streamlined.

Here is an overview of states we're interested in:

  • Wallet isn't connected
  • Wallet is connected, but chain isn't supported
  • Wallet is Ledger and requires action (can we open the app on-device programatically as an easy enough lift?)
  • "Quote" error (gas estimation error, pool halted etc)
  • Loading state

References and additional details

TODO

Acceptance Criteria

  • A plan of action is redacted on how to avoid Ledger regressions, but most importantly streamline wallet-dependant components and actions
  • Stretch: How hard is it to get Ledger "open app" functionality working? If we can do so, then this would be the perfect contender for it, as having the hdwallet functionality for it is one thing and a beast of its own, but then we would still have to write a bunch of repeated logic everywhere to detect and fire it, versus "this component" (if this ends up being a component) handling it.

Need By Date

No response

Screenshots/Mockups

No response

Estimated effort

No response

@twblack88 twblack88 moved this from Backlog to Spike in ShapeShift Dashboard Mar 13, 2024
@twblack88 twblack88 added the needs architecture Needs architecture input before engineering label Mar 13, 2024
@twblack88
Copy link
Contributor

Some notes from spike @purelycrickets Common component doesn't have the right estimate baked in. We want to do a holistic treating for is.crypto is.fiat simply the way we do in swapper. Generalizing this abstraction out plugs it into everywhere else (lending, LP etc.)

@0xean
Copy link
Contributor

0xean commented Jan 6, 2025

closing and referencing in #8471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs architecture Needs architecture input before engineering
Projects
Status: Done
Development

No branches or pull requests

4 participants