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: manual receive address empty confirm step #5947

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 4, 2024

Description

Fixes the manual receive address screen being borked currently.

tl;dr of why that is:

  • <ManualAddressEntry /> validation method initially resets the manual receive address before parsing, validating, and setting it again

dispatch(swappers.actions.setManualReceiveAddress(undefined))

  • useGetTradeQuotes reacts to this and this condition fails

if (wallet && sellAccountId && sellAccountMetadata && receiveAddress && !isVotingPowerLoading) {

we then end up in the following block, where we reset the active quote index:

if (tradeQuoteInput !== skipToken) {
setTradeQuoteInput(skipToken)
dispatch(tradeQuoteSlice.actions.resetConfirmedQuote())
dispatch(tradeQuoteSlice.actions.resetActiveQuoteIndex())

  • Since we don't have an active quote index anymore, selectActiveSwapperName returns undefined
  • Since we are not in the context of <TradeInput /> anymore after clicking the preview button, we never set the active quote index again

The core issue of this is validation being re-ran while <ManualAddressEntry /> despite the input not changing, instead of it immediately unmounting (which it does only a few render cycles later, after re-validation occurred), and is most likely a combination of react-hook-form v7 not automagically unregistering inputs after unmount (see https://github.com/orgs/react-hook-form/discussions/3714), however, none of manual unregister() calls, shouldUnregister, nor using a show state variable that's set with ReactDOM.flushSync() to avoid batching helped here.
if you ever run into a similar validation running right before unmount kind of bug, the above could be explored.

While this doesn't fix the core issue of why the validation gets re-ran after clicking the confirm button, this fixes the current issue at hand, and this bug is very much isolated to our useGetTradeQuotes hook reactivity, so this is unlikely to produce other bugs in the future.

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #5945

Risk

Low, see testing steps. Theoretically, not resetting the manual receive address in store beforehand could bring bugs, however, we do gracefully handle it already, and a catch block has been added for extra safety.

Testing

  • Ensure manual receive address is working
  • Ensure validation of manual receive address is working (i.e inputting an invalid address doesn't allow you to continue)
  • Ensure using a wallet-supported pair (e.g ETH -> FOX on MM or ETH -> DOGE on native) is still happy

Engineering

  • ☝🏽

Operations

  • ☝🏽

Screenshots (if applicable)

Invalid manual receive address

image

Valid receive address

image image

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@gomesalexandre gomesalexandre marked this pull request as ready for review January 4, 2024 23:52
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 4, 2024 23:52
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.

Trades with manual receive address are working as expected, and quotes for ETH <> BTC are great again.

@0xApotheosis 0xApotheosis merged commit 0c45aaf into develop Jan 8, 2024
4 checks passed
@0xApotheosis 0xApotheosis deleted the fix_manual_receive_address_reactivity branch January 8, 2024 00:46
0xApotheosis added a commit that referenced this pull request Jan 8, 2024
* fix: manual receive address empty confirm step

* feat: ocd terminology

---------

Co-authored-by: Apotheosis <0xapotheosis@gmail.com>
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.

Swapper flow fails for ETH <> BTC transaction
2 participants