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

sviderock · 2024-12-19T13:21:36Z

src/components/TokenEnterAmount.tsx

-              styles.primaryAmountText,
-              inputStyle,
-              Platform.select({ ios: { lineHeight: undefined } }),
+        <View>


This addition/removal is purely a result of adding extra View wrapper around the input as it is needed for the new loading check below. The actual TextField props are unchanged.

sviderock · 2024-12-19T13:22:29Z

src/components/TokenEnterAmount.tsx

+export function getDisplayTokenName<T extends { symbol: string; networkId: NetworkId }>(token: T) {
+  return `${token.symbol} on ${NETWORK_NAMES[token.networkId]}`
+}
+


This was moved to a function just for the sake of being able to use it in the SwapScreenV2.test.tsx file as well.

not related to the actual change, but it will show "on" without translation no matter what language is used? are we okay with that?

@bakoushin You're right, my bad! Will add it to the translations.

sviderock · 2024-12-19T13:26:36Z

src/swap/SwapScreenV2.tsx

+    networkId: fromToken?.networkId || networkConfig.defaultNetworkId,
+    slippagePercentage: maxSlippagePercentage,
+    enableAppFee: enableAppFee,
+    onError: (error) => {


onError and onSuccess are the same as identical useEffects from SwapScreen.tsx

sviderock · 2024-12-19T13:31:38Z

src/swap/SwapScreenV2.tsx

+    })
+  }
+
+  function onSelectTokenFrom(selectedToken: TokenBalance, tokenPositionInList: number) {


In SwapScreen.tsx functionality of onSelectTokenFrom and onSelectTokenTo functions is implemented in handleConfirmSelectToken. These two functions execute the same logic but as a two separate handlers. When I was trying to understand the flow of the data in the existing component it seemed like separating this function into separate handlers with straightforward actions could make it more understandable.

Obviosuly, that's subjective so please let me know if you have any comments or objections!

shall we keep the handle- prefix?

perhaps we can keep 2 handlers, but extract most of the logic into a separate function?
the main difference i noticed is the field prop, the rest seems identical.
it feels not quite right to introduce that amount of ducplication

bakoushin · 2024-12-23T12:13:29Z

src/components/TokenEnterAmount.tsx

@@ -139,6 +145,13 @@ export function useEnterAmount(props: {
   *       - `local.displayAmount` -> `localDisplayAmount`
   */
  const processedAmounts = useMemo(() => {
+    if (!props.token) {


is it okay that this variable is not listed in the dependecies list?

@bakoushin accidentally missed it! Will add it to the list.

P.S. Sometimes I really wish we had eslint rule for this 😄

bakoushin · 2024-12-23T12:32:53Z

src/swap/useSwapQuote.ts

@@ -183,7 +187,7 @@ function useSwapQuote({
        return null
      }

-      if (!swapAmount[updatedField].gt(0)) {
+      if (!swapAmount[updatedField] || !swapAmount[updatedField].gt(0)) {


i wonder if we need this condition: !swapAmount[updatedField].gt(0)?
i noticed that we do almost the same check against the swpapAmountInWay just below

@bakoushin I've just adjusted the existing logic to reduce the amount of potential changes. But I can remove if needed!

bakoushin · 2024-12-23T15:05:25Z

src/swap/SwapScreenV2.tsx

+                onInputChange={handleAmountInputChange}
+                amountType={amountType}
+                toggleAmountType={handleToggleAmountType}
+                onOpenTokenPicker={onOpenTokenPickerFrom}


supernit: shall we prefix this handler with handle- instead of on- for naming consistency?

@bakoushin I think it makes sense to keep props of components with on to follow the regular naming convention of event handlers for components (e.g. onClick/onBlur etc).

Naming function handlers with handle totally makes sense to me though. Will change that!

bakoushin · 2024-12-23T15:08:32Z

src/swap/SwapScreenV2.tsx

+                amountType={amountType}
+                onOpenTokenPicker={onOpenTokenPickerTo}
+                loading={shouldShowSkeletons}
+                testID="SwapAmountInput"


this testID is identical to the "from" component. perhaps a typo?

@bakoushin its per design. It was like that before rework and I've kept it the same cause in the test files it searches for the same testIDs but in different containers (from/to)

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

src/swap/SwapScreenV2.tsx

+      >
+        <View style={{ display: 'flex', justifyContent: 'space-between', flexGrow: 1 }}>
+          <View style={{ flexShrink: 0 }}>
+            <View style={[styles.warningsContainer, { gap: 4 }]}>


"warnings" sounds kind of confusing: there is no any warning inside this view

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

src/swap/SwapScreenV2.tsx

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


do we need this <View> wrapper?

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 · 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 · 2024-12-23T15:41:47Z

src/swap/SwapScreenV2.tsx

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


do we need this <View> wrapper?

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 · 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 · 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 · 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 · 2024-12-23T17:04:03Z

src/swap/SwapScreenV2.tsx

+      // clearQuote,
+      // replaceAmountTo,


shall we remove it?

or uncomment? 😅

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

Epic work! 🎉

Left few nits/questions

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

sviderock added 6 commits December 19, 2024 12:58

hide SwapScreenV2 behind the feature flag

aa79667

reduce unnecessary changes

76b4dba

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

8c6c71a

try to reduce unnecessary changes even more

8599b18

rename component

c3ba81b

fix export default

930a919

sviderock commented Dec 19, 2024

View reviewed changes

more fixes

5e780d9

sviderock marked this pull request as ready for review December 19, 2024 13:32

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

98c06f9

bakoushin reviewed Dec 23, 2024

View reviewed changes

src/swap/SwapScreenV2.tsx

/>

</View>

<View>

Copy link

Contributor

bakoushin Dec 23, 2024

Choose a reason for hiding this comment

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

do we need this <View> wrapper?

bakoushin reviewed Dec 23, 2024

View reviewed changes

src/swap/SwapScreenV2.tsx

</View>

</View>

<View>

Copy link

Contributor

bakoushin Dec 23, 2024

Choose a reason for hiding this comment

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

do we need this <View> wrapper?

bakoushin reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sviderock commented Nov 19, 2024 •

edited

Loading

sviderock Dec 19, 2024

sviderock Dec 19, 2024 •

edited

Loading

bakoushin Dec 23, 2024

sviderock Dec 23, 2024

sviderock Dec 19, 2024

sviderock Dec 19, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024 •

edited

Loading

sviderock Dec 23, 2024

bakoushin Dec 23, 2024

sviderock Dec 23, 2024

bakoushin Dec 23, 2024 •

edited

Loading

sviderock Dec 31, 2024

bakoushin Dec 23, 2024

sviderock Dec 31, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024 •

edited

Loading

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin commented Dec 23, 2024

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

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

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

sviderock Dec 19, 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

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

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

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

bakoushin commented Dec 23, 2024

sviderock commented Nov 19, 2024 •

edited

Loading

sviderock Dec 19, 2024 •

edited

Loading

bakoushin Dec 23, 2024 •

edited

Loading

bakoushin Dec 23, 2024 •

edited

Loading

bakoushin Dec 23, 2024 •

edited

Loading