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: new trade flow loading states #8527

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

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 9, 2025

Description

Two birds, one stone:

  1. Removes hasClickedButton heuristics in profit of poor man's form control submission state. Since we're dealing with async flows in mutations, we are actually capable of properly detecting submission states, without needing to resort to button click hacks.
  2. Adds some logic to allowance/reset loading states so that we consider Tx complete a loading state, avoiding a weird transitive state where the approval Tx is complete, but the allowance hasn't been refetched and we haven't transitioned to the next step yet, meaning "Approve" button would be shown and enabled in that interim, vs. disabled and loading.

In effect, this allows for Txs to be retried as a direct side-effect of having a form submission of sorts.

Issue (if applicable)

closes #8523

Risk

High Risk PRs Require 2 approvals

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

Low - new swapper flow only, and changeset pretty much aligns the logic to that of old swapper flow.

Testing

  • Test with 0x as it is the one with the most stages (permit2 contract approval, permit2 EIP712 signing, sign message)
  • Ensure all of the three steps, when cancelled in the wallet, can be triggered again
  • Ensure loading/transition states are happy
  • Additionally, test me with another swapper (say CoW) and USDT mainnet allowance reset to ensure it is also handled gracefully

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

ZRX - all 3 steps are retry-able and loading states are happy

https://jam.dev/c/c5bc81e4-a931-4cc8-be0f-7b406d417abf

CoW USDT allowance/reset steps are happy

https://jam.dev/c/f94c1e9f-acde-43c2-a93a-4f782d18c7b4

@gomesalexandre gomesalexandre marked this pull request as ready for review January 9, 2025 14:19
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 9, 2025 14:19
@0xApotheosis 0xApotheosis self-assigned this Jan 10, 2025
Copy link
Member

@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.

One blocking comment, otherwise functionally excellent!

Can happily continue after an error at any stage:

Screenshot 2025-01-10 at 15 52 32 Screenshot 2025-01-10 at 15 53 07 Screenshot 2025-01-10 at 15 53 26

setIsSubmitting(true)
await tradeButtonProps?.onSubmit()
} finally {
setIsSubmitting(false)
Copy link
Member

Choose a reason for hiding this comment

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

🧠

Comment on lines -55 to -56
hasClickedButton,
setHasClickedButton,
Copy link
Member

Choose a reason for hiding this comment

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

Blocking:

Since we aren't using these anymore we'll want to remove them from the TradeFooterButtonProps and all remove all related parent logic:

diff --git a/src/components/MultiHopTrade/components/TradeConfirm/TradeConfirmFooter.tsx b/src/components/MultiHopTrade/components/TradeConfirm/TradeConfirmFooter.tsx
index 280ed52e20..1a4d655de6 100644
--- a/src/components/MultiHopTrade/components/TradeConfirm/TradeConfirmFooter.tsx
+++ b/src/components/MultiHopTrade/components/TradeConfirm/TradeConfirmFooter.tsx
@@ -1,7 +1,7 @@
 import { HStack, Skeleton, Stack, Switch } from '@chakra-ui/react'
 import type { TradeQuoteStep } from '@shapeshiftoss/swapper'
 import type { FC } from 'react'
-import { useMemo, useState } from 'react'
+import { useMemo } from 'react'
 import { Amount } from 'components/Amount/Amount'
 import { Row } from 'components/Row/Row'
 import { Text } from 'components/Text/Text'
@@ -33,7 +33,6 @@ export const TradeConfirmFooter: FC<TradeConfirmFooterProps> = ({
   activeTradeId,
 }) => {
   const [isExactAllowance, toggleIsExactAllowance] = useToggle(true)
-  const [hasClickedButton, setHasClickedButton] = useState(false)
   const { currentTradeStep } = useStepperSteps()
   const currentHopIndex = useCurrentHopIndex()
   const quoteNetworkFeeCryptoBaseUnit = tradeQuoteStep.feeData.networkFeeCryptoBaseUnit
@@ -140,8 +139,8 @@ export const TradeConfirmFooter: FC<TradeConfirmFooterProps> = ({
   }, [tradeQuoteStep])

   const isApprovalButtonDisabled = useMemo(() => {
-    return isAllowanceApprovalLoading || hasClickedButton
-  }, [isAllowanceApprovalLoading, hasClickedButton])
+    return isAllowanceApprovalLoading
+  }, [isAllowanceApprovalLoading])

   const tradeAllowanceStepSummary = useMemo(() => {
     return (
@@ -276,8 +275,6 @@ export const TradeConfirmFooter: FC<TradeConfirmFooterProps> = ({
         currentHopIndex={currentHopIndex}
         activeTradeId={activeTradeId}
         isExactAllowance={isExactAllowance}
-        hasClickedButton={hasClickedButton}
-        setHasClickedButton={setHasClickedButton}
         isLoading={isNetworkFeeCryptoBaseUnitLoading || isNetworkFeeCryptoBaseUnitRefetching}
       />
     )
@@ -286,7 +283,6 @@ export const TradeConfirmFooter: FC<TradeConfirmFooterProps> = ({
     currentHopIndex,
     activeTradeId,
     isExactAllowance,
-    hasClickedButton,
     isNetworkFeeCryptoBaseUnitLoading,
     isNetworkFeeCryptoBaseUnitRefetching,
   ])
diff --git a/src/components/MultiHopTrade/components/TradeConfirm/TradeFooterButton.tsx b/src/components/MultiHopTrade/components/TradeConfirm/TradeFooterButton.tsx
index c00a694dda..0e2e616dd6 100644
--- a/src/components/MultiHopTrade/components/TradeConfirm/TradeFooterButton.tsx
+++ b/src/components/MultiHopTrade/components/TradeConfirm/TradeFooterButton.tsx
@@ -42,8 +42,6 @@ type TradeFooterButtonProps = {
   currentHopIndex: SupportedTradeQuoteStepIndex
   activeTradeId: string
   isExactAllowance: boolean
-  hasClickedButton: boolean
-  setHasClickedButton: (hasClickedButton: boolean) => void
   isLoading?: boolean
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Allow for retry on swapper failure
2 participants