feat(TokenEnterAmount): add new component to Swap flow #6247

sviderock · 2024-11-19T12:44:28Z

Final PR for new Enter Amount component. This page implements a 2nd version of SwapScreen that uses new Enter Amount component. It is hidden behind the feature flag to mitigate the risks of such a big rework.

SwapScreenV2.test.tsx file is an exact copy of the existing SwapScreen.test.tsx. It seems like a huge addition (2k lines) BUT it is a 99% copy-paste just because the new flow is implemented as a new component. The only changes in that file are testID props and some translations excerpts. To make life A LOT easier in reviewing this specific file, I suggest doing the following in VSCode to only see the changed lines:

Right click SwapScreen.test.tsx and click "Select for Compare"
Right click SwapScreenV2.test.tsx and click "Compare with Selected"

Details of the implementation for clarity:

The correct functioning of the new component has been ensured by implementing it to pass all the existing tests from SwapScreen.test.tsx.
Removed local Redux slice. As a new Enter Amount component is using useState and it would be a mess to keep multiple useStates and Redux slice in sync - all the other fields from the slice were moved to the corresponding useStates.
The business logic of the component is intact. Only the state implementation (useState instead of Redux slice) has changed to follow the existing logic therefore some functions look different but they do identical things in terms of business logic.
Code has been order in the following order:
1. variables
2. derived variables (useMemo)
3. useEffects
4. functions

Test plan

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-12-19.at.14.56.55.mp4

Related issues

Relates to RET-1208

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

…ter-amount-hook

bakoushin · 2024-12-23T15:16:45Z

src/swap/SwapScreenV2.tsx

+              />
+            </View>
+
+            <View>


do we need this <View> wrapper?

@bakoushin I think it's indeed redundant, will remove!

bakoushin · 2024-12-23T15:27:09Z

src/swap/SwapScreenV2.tsx

+                  if (
+                    !quote ||
+                    quote.preparedTransactions.type !== 'need-decrease-spend-amount-for-gas'
+                  )
+                    return


why we need this condition? it seems to lead to a confusing ux.

if i correctly understand its behavior:

the warning is displayed

but the CTA press does nothing

why it does nothing? maybe we shoudn't display the warning in the first place?

@bakoushin As I understand - it's purely for type narrowing. It shouldn't ever evaluate to true as warnings.showDecreaseSpendForGasWarning directly contradicts it. But with this condition quote.preparedTransactions is evaluated to PreparedTransactionsNeedDecreaseSpendAmountForGas and doesn't need type casting with as (even thought the actual warnings.showDecreaseSpendForGasWarning check already ensures the type is right but typescript cannot determine it as it considers it a "different variable" 🤷 )

ah, yes! it makes sense, thanks! maybe we can add the // Should never happen comment here -- up to your judjement.
i noticed they are often used in our code base to mark those type guards and allow to just skip them when reading without thinking too much. at least, that's how it works for me 😅

bakoushin · 2024-12-23T15:31:53Z

src/swap/SwapScreenV2.tsx

+                  feeCurrencies,
+                })}
+                style={styles.warning}
+                onPressCta={onPressLearnMoreFees}


either the cta label is missing or this handler is redundant

@bakoushin you're right! It was like that initially so I assume that it was accidentally left out from clean up.

bakoushin · 2024-12-23T15:41:47Z

src/swap/SwapScreenV2.tsx

+          </View>
+        </View>
+
+        <View>


do we need this <View> wrapper?

@bakoushin Yes. Per design, UK disclaimer has to be right above the button and this View is a direct child of ScrollView component with styles.contentContainer which has justifyContent: 'space-between'. Without this view disclaimer and button will be evenly separated from other blocks (inputs and warnings) across the whole height of the screen instead of sticking together.

bakoushin · 2024-12-23T15:55:51Z

src/swap/SwapScreenV2.tsx

+        contentContainerStyle={styles.contentContainer}
+        keyboardShouldPersistTaps="handled"
+      >
+        <View style={{ display: 'flex', justifyContent: 'space-between', flexGrow: 1 }}>


what is the reason of inlining styles vs including them in the StyleSheet?

and do we need that View at all?

@bakoushin

It's just me not doing my due diligence with the styles cleanup after making layout changes. Will move this to a separate class!

We do need it as it separates block with inputs and warnings from the block with the disclaimer and button which ensures the first block to stay at the very top and the second one - at the very bottom.

bakoushin · 2024-12-23T16:09:07Z

src/swap/SwapScreenV2.tsx

+        keyboardShouldPersistTaps="handled"
+      >
+        <View style={{ display: 'flex', justifyContent: 'space-between', flexGrow: 1 }}>
+          <View style={{ flexShrink: 0 }}>


do we need this style?

@bakoushin it's indeed redundant, will remove!

bakoushin · 2024-12-23T16:21:42Z

src/swap/SwapScreenV2.tsx

+    }
+
+    if (fromToken?.tokenId === selectedToken.tokenId) {
+      setFromToken(toToken)


do we need to set "from" token in this handler?

please ignore, got it 😅

bakoushin · 2024-12-23T16:22:08Z

src/swap/SwapScreenV2.tsx

+    // if in "from" we select the same token as in "to" then just swap
+    if (toToken?.tokenId === selectedToken.tokenId) {
+      setFromToken(toToken)
+      setToToken(fromToken)


do we need to set "to" token in this handler?

please ignore, got it 😅

bakoushin · 2024-12-23T16:58:55Z

src/swap/SwapScreenV2.tsx

+    AppAnalytics.track(SwapEvents.swap_screen_select_token, { fieldType: Field.FROM })
+    // use requestAnimationFrame so that the bottom sheet open animation is done
+    // after the selectingField value is updated, so that the title of the
+    // bottom sheet (which depends on selectingField) does not change on the
+    // screen
+    requestAnimationFrame(() => tokenBottomSheetFromRef.current?.snapToIndex(0))


shall we consistenly use the hadndle prefix?

perhaps we can extract this code to a separate function e.g. handleOpenTokenPicker(fieldType: Field) to emphasize that hanlding is mostly identical

@bakoushin hard agree on both!

bakoushin · 2024-12-23T17:02:30Z

src/swap/SwapScreenV2.tsx

+
+        if (!fromToken || !toToken) {
+          // Should never happen
+          Logger.error(TAG, 'fromToken or toToken not found')


if that is a type guard (as "should never happen" comment suggests), do we need to log anything?

@bakoushin this log was present there from the start. I would keep it just in case somehow in the future we actually hit it for any miraculous reason (even though it should be impossible but who knows 🤷 )

bakoushin · 2024-12-23T17:04:03Z

src/swap/SwapScreenV2.tsx

+      // clearQuote,
+      // replaceAmountTo,


shall we remove it?

or uncomment? 😅

@bakoushin I'll add a TODO comment for this one. This useEffect initially lacked some dependencies which contradicts the rules of hooks but if I include all used variables/functions - it causes unnecessary re-runs which breaks a bunch of tests. So in order to make this optimization more clear - I'd prefer to make it in a separate PR, especially considering that currently the flow works as intended (just not following the rules of hooks).

So I've just left those commented there to not forget that they should also be acknowledged once I get to the point of optimizing it so I'll add a comment explaining that.

bakoushin · 2024-12-23T17:20:10Z

Epic work! 🎉

Left few nits/questions

emerge-tools · 2025-01-02T09:48:15Z

📸 Snapshot Test

Base build not found

No build was found for the base commit 8891d00. This is required to generate a snapshot diff for your pull request.

It's possible that you created a branch off the base commit before all of the CI steps have finished processing, e.g. the one that uploads a build to our system. If that's the case, no problem! Just wait and this will eventually resolve.

🛸 Powered by Emerge Tools

bakoushin · 2025-01-02T13:14:07Z

src/components/TokenEnterAmount.tsx

@@ -201,9 +215,11 @@ export function useEnterAmount(props: {
        displayAmount: getDisplayLocalAmount(parsedLocalAmount, localCurrencySymbol),
      },
    }
-  }, [amount, amountType, localCurrencySymbol])
+  }, [amount, amountType, localCurrencySymbol, props.token])


not directly related to this PR, but i'm trying to better understand the logic behind the inclusion/exclusion some variables to the dependency list.

for instance, we included the localCurrencySymbol, but not usdToLocalRate. yeah, it is highly unlikely that the rate could suddenly change while this component is rendered. but what is the reason for choosing one over another? it seems equally unlikely that the currency symbol will suddenly change.

@bakoushin that's just again my inattentiveness! Everything used in the scope should be included in the deps list. Will fix this now!

bakoushin · 2025-01-02T13:23:47Z

locales/base/translation.json

@@ -2823,5 +2823,6 @@
    "availableBalance": "Available: <0></0>",
    "selectToken": "Select token",
    "fiatPriceUnavailable": "Price unavailable"
-  }
+  },
+  "on": "on"


i guess the translation of the word "on" is highly dependent on context. perhaps we can use some template to provide it? kind of like this:

"tokenDescription": "{{tokenName}} on {{tokenNetwork}}"

and, with that, shall we include this item in the tokenEnterAmount group above?

do we need to keep the "on"?

bakoushin · 2025-01-02T13:57:44Z

src/earn/poolInfoScreen/BeforeDepositBottomSheet.tsx

@@ -283,6 +285,9 @@ export default function BeforeDepositBottomSheet({
  const { t } = useTranslation()

  const { availableShortcutIds } = pool


not sure if those files should appear in the diff

@bakoushin oops, that's a merge going wrong.

@bakoushin this one's resolved!

bakoushin · 2025-01-02T14:08:38Z

src/points/PointsHistoryBottomSheet.test.tsx

@@ -127,7 +127,7 @@ describe(PointsHistoryBottomSheet, () => {
    ).toBeTruthy()
    expect(tree.getByText('points.history.cards.createWallet.subtitle')).toBeTruthy()

-    expect(tree.getByText('March')).toBeTruthy()
+    expect(tree.getByText('March', { exact: false })).toBeTruthy()


perhaps this change is worth its own (yet tiny) PR, so other team members could benefit from the fix before this epic PR is merged.
also, it will provide more context as to why it is fixed.

@bakoushin addressed in this PR: #6392

…ion string

bakoushin · 2025-01-02T14:47:18Z

src/swap/SwapScreenV2.tsx

+        [Field.FROM]: processedAmountsFrom.token.bignum
+          ? processedAmountsFrom.token.bignum.toString()
+          : '',


supernit: perhaps we can make use of a nullish coalescing operator and save 2 lines here? 😅

processedAmountsFrom.token.bignum?.toString() ?? ''

@bakoushin it's even gonna be 4 lines cause it has to be done for both FROM and TO 😄

bakoushin · 2025-01-02T14:51:18Z

src/swap/SwapScreenV2.tsx

+    let newSwitchedToNetwork: typeof switchedToNetworkId | null = null
+
+    switch (true) {
+      // If were selecting a field that was already selected in the other input then switch inputs


looks like typo: "we're"

bakoushin · 2025-01-02T15:19:42Z

src/swap/SwapScreenV2.tsx

+    if (!fromToken) {
+      // Should never happen
+      return
+    }
+    AppAnalytics.track(SwapEvents.swap_screen_percentage_selected, {
+      tokenSymbol: fromToken.symbol,
+      tokenId: fromToken.tokenId,
+      tokenNetworkId: fromToken.networkId,
+      percentage,
+    })
+  }
+
+  function onPressLearnMore() {
+    AppAnalytics.track(SwapEvents.swap_learn_more)
+    navigate(Screens.WebViewScreen, { uri: links.swapLearnMore })
+  }
+
+  function onPressLearnMoreFees() {
+    AppAnalytics.track(SwapEvents.swap_gas_fees_learn_more)
+    navigate(Screens.WebViewScreen, { uri: links.transactionFeesLearnMore })
+  }
+
+  return (
+    <SafeAreaView style={styles.safeAreaContainer} testID="SwapScreen">
+      <CustomHeader


stylistic nit: most of the handlers above start with handle-, but these two start with on-. it would be nice to have a consistent naming (my preference is to keep using handle-)

@bakoushin fixed!

bakoushin

🚀

sviderock added 18 commits November 18, 2024 16:44

add new TokenEnterAmount component

e5ab0a5

fix exports

2fbc1b2

Merge branch 'main' into slava/add-new-enter-amount-component

b12d50f

Add reusable useEnterAmount hook

8af7b8d

remove unused code

44c0aab

change flow on send

1aa17d6

fix tests

4ed89fc

fix jumpstart

d0c7099

change earn flow

98d6552

remove unnecessary changes

b7e5107

Merge branch 'main' into slava/add-new-enter-amount-component

6ca6f92

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

f2185bc

…ter-amount-hook

Merge branch 'slava/add-enter-amount-hook' into slava/change-send-flow

8762401

fix knip

98938bc

uncomment some tests

5a3f776

fixes

9360626

Merge branch 'slava/change-send-flow' into slava/change-earn-flow

f322e23

wip: change swap flow

a54d71f

sviderock changed the base branch from main to slava/change-earn-flow November 19, 2024 12:44

sviderock added 11 commits November 20, 2024 15:39

add tests

b3f3c67

clear up console logs

8ce68a4

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

c2773fa

…ter-amount-hook

WIP: tests for the hook

37455cb

reformat groupNumber, finish number formatting tests

8357b18

add useEnterAmount initial tests

f5db952

finish tests

130b620

revert unnecessary change

10fb5bd

Merge branch 'main' into slava/add-new-enter-amount-component

112e5f9

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

bce9c2b

…ter-amount-hook

Merge branch 'slava/add-enter-amount-hook' into slava/change-send-flow

7048f8e

bakoushin reviewed Dec 23, 2024

View reviewed changes

multiple fixes as per review

726153d

bakoushin reviewed Jan 2, 2025

View reviewed changes

Merge branch 'main' into slava/change-swap-flow

9c6886d

sviderock force-pushed the slava/change-swap-flow branch from 0130a21 to 9c6886d Compare January 2, 2025 14:21

sviderock added 3 commits January 2, 2025 16:22

fix incomplete list of deps for processedAmounts memo

27a7f58

more fixes as per review

67c69ba

fix tests affected by moving token description to a separate translat…

68319ed

…ion string

bakoushin reviewed Jan 2, 2025

View reviewed changes

sviderock added 2 commits January 2, 2025 17:00

more fixes as per review

bd8999c

fix typo

e6b5971

bakoushin reviewed Jan 2, 2025

View reviewed changes

change remaining "on" handlers with "handle"

7caeb77

bakoushin approved these changes Jan 2, 2025

View reviewed changes

+                            />
+                          </View>
+                          <View>

+                        </View>
+                      </View>
+                      <View>

		@@ -283,6 +285,9 @@ export default function BeforeDepositBottomSheet({
		const { t } = useTranslation()

		const { availableShortcutIds } = pool

feat(TokenEnterAmount): add new component to Swap flow #6247

Are you sure you want to change the base?

feat(TokenEnterAmount): add new component to Swap flow #6247

Conversation

sviderock commented Nov 19, 2024 • edited Loading

Test plan

Related issues

Backwards compatibility

Network scalability

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin commented Dec 23, 2024

emerge-tools bot commented Jan 2, 2025 • edited Loading

📸 Snapshot Test

Base build not found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin left a comment

Choose a reason for hiding this comment

sviderock commented Nov 19, 2024 •

edited

Loading

bakoushin Dec 23, 2024 •

edited

Loading

emerge-tools bot commented Jan 2, 2025 •

edited

Loading

sviderock Jan 2, 2025 •

edited

Loading

sviderock Jan 2, 2025 •

edited

Loading