web icon indicating copy to clipboard operation
web copied to clipboard

feat: improve useSendDetails loading, early bail and debounce

Open gomesalexandre opened this issue 9 months ago • 2 comments

Description

Started as a simple fix for early bailing out of estimating fees when funds are insufficient (see this sentry issue, but noticed our debouncing in useSendFees is borked out of sanity i.e:

  • Have a balance of 0.6 ETH
  • Type 0.5 which is happy
  • Now either clear the 0. part, or add a 9 at the end which would result in insufficient balance. The insufficient balance will be displayed for a second or so, only for the previously debounced promise to settle, its side effects run, and the send modal being wrongly in a valid balance state.

The root cause is that while async functions may look like good ol' functions from a syntactic sugar point of view, they are promises, and promises are not cancellable. Us calling .cancel() on the debounced "function" doesn't really do anything as far as canceling promises goes. This could be solved by using bluebird or other fancy APIs like AbortController which both come with their set of issues (package and API overhead for the former, boilerplaty and limited to the fetch API for the latter).

Fixed by going on a proper refactor of useSendFees which:

  • decouples the network logic and the input change logic. handleInputChange's sole responsibility is now setting values in the form, and the newly introduced query handles fetching fees and error-handling
  • things are still debounced, though in a more simple fashion, by debouncing the amountCryptoPrecision input

And obviously fixes the issue this initially went for, which is early bailing out of estimating fees when there is insufficient balance.

Pull Request Type

  • [ ] :bug: Bug fix (Non-breaking Change: Fixes an issue)
  • [ ] :hammer_and_wrench: Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • [x] :nail_care: New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #6877

Risk

High Risk PRs Require 2 approvals

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

Testing

  • Loading states are still happy i.e is still reflected immediately on user input
  • Insufficient funds checks are still happy:
    • Insufficient funds (either for token or native asset sends) are still surfaced with a "Insufficient funds" error
    • Token sends without enough funds for gas are still surfaced with a "Not enough native token for gas" error
  • Send max is still happy:
    • Token sends set the absolute max
    • Native asset still do fees deduction to set the actual max
  • Debouncing is actually working, and previous requests completing do not end up in a wrong state i.e
    • Having an initial input which settles to a happy state (enough funds), but a subsequent one that completes to a fail (not enough funds) correctly settles to a fail state visually, without a flash of wrong state
    • Having an initial input which settles to a sad state (not enough funds), but a subsequent one that completes to a happy (enough funds) state correctly settles to a happy state visually, without a flash of wrong state

Engineering

  • Input when not enough funds does not trigger an estimate request
  • ☝🏽

Operations

Screenshots (if applicable)

  • Having an initial input which settles to a happy state (enough funds), but a subsequent one that completes to a fail (not enough funds) correctly settles to a fail state visually, without a flash of wrong state

https://github.com/shapeshift/web/assets/17035424/863a9398-7db4-4642-9aaa-b796951b6624

  • Having an initial input which settles to a sad state (not enough funds), but a subsequent one that completes to a happy (enough funds) state correctly settles to a happy state visually, without a flash of wrong state

https://github.com/shapeshift/web/assets/17035424/a946cea0-ef36-493d-9311-6984b773e441

  • Input when not enough funds does not trigger an estimate request

https://github.com/shapeshift/web/assets/17035424/d49a057b-c312-4d52-acf4-99a3db4f3ffe

  • Insufficient funds (either for token or native asset sends) are still reflected as "Insufficient funds"
image image
  • Token sends without enough funds for gas are still surfaced with a "Not enough native token for gas" error
image
  • Token sends set the absolute max
image
  • Native asset still do fees deduction to set the actual max
image image image

gomesalexandre avatar May 10 '24 12:05 gomesalexandre

Testing notes

✅ Send non-native max ✅ Send not max


🚫 NaN appearing when attempting to enter fiat amount (confirmed not in prod):

https://github.com/shapeshift/web/assets/125113430/72f0a8c9-07e1-40fe-b9df-f7cd7954a45a


🚫 Insufficient Funds appears when clearing the fiat amount:

image


🚫 Allows user to send 0 amount:

https://github.com/shapeshift/web/assets/125113430/93a5688e-f337-4fa1-8e3e-53528aa6899c


🚫 Allows user to enter negative amount, displays Insufficient Funds:

image


🚫 Unable to send max on native token:

image

woodenfurniture avatar May 16 '24 00:05 woodenfurniture

Stellar testing @woodenfurniture, ty!

🚫 NaN appearing when attempting to enter fiat amount (confirmed not in prod) 🚫 Insufficient Funds appears when clearing the fiat amount

Both fixed in 271e1adb9e - https://jam.dev/c/d54a8ec9-29b1-46fc-808c-2adcc72d220c

🚫 Allows user to send 0 amount 🚫 Allows user to enter negative amount, displays Insufficient Funds

Both are also in develop/prod - quite wary about introducing more issues here so stacked out on top of this instead: https://github.com/shapeshift/web/pull/6930. See this Jam for the fix this PR introduces: https://jam.dev/c/542c2419-7c78-4339-a27e-77f63666e490 though I'll still need to test the send max negative amounts there. My main concern is that negative amounts can sometimes be present when clicking max, fee deduction is expected (i.e native asset) but fees are greater than amount, this is valid and expected and we could break the error messaging here).

🚫 Unable to send max on native token

That one is specific to Arb fees which are always derp and very volatile by nature and isn't specific to this PR, see jam against prod (note, I monkey patched the staleTime of fees estimation to 2s for ease of testing), though this PR made the behavior more obvious (and actually correct, vs. the infinite loading state on prod):

https://jam.dev/c/b01da49b-485d-4f43-b6b5-59982a609d92

If you test with another native asset, things should be consistently happy:

https://jam.dev/c/a8121673-3cf4-43df-8fa0-a9987b09a256

gomesalexandre avatar May 16 '24 09:05 gomesalexandre