feat(TokenEnterAmount): add new flow to EnterAmount.tsx #6245

sviderock · 2024-11-19T06:29:08Z

Description

3/5 PR for new Enter Amount component. This PR uses the new component and hook in the Send flow. A lot of tests are adjusted to follow the new flow.

AmountInput component was moved purely for compatibility with EarnEnterAmount component. This is an outdated boilerplate code which will be removed in the next PR (4/5). Most of the line changes is this specific component and a lot of tests' adjusting. There's really not many changes other than replacing boilerplate code with the same code from a reusable hook.

Test plan

Changes a bunch of test files to relate to the new changes. All the tests related to MAX button are skipped for now.

send.flow.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)

codecov · 2024-11-19T07:17:03Z

Codecov Report

Attention: Patch coverage is 98.96907% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.93%. Comparing base (1ea0927) to head (5e2dbee).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/TokenEnterAmount.tsx	97.56%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6245      +/-   ##
==========================================
- Coverage   88.95%   88.93%   -0.02%     
==========================================
  Files         739      739              
  Lines       31644    31609      -35     
  Branches     5892     5883       -9     
==========================================
- Hits        28149    28113      -36     
+ Misses       3450     3296     -154     
- Partials       45      200     +155

Files with missing lines	Coverage Δ
src/components/LabelWithInfo.tsx	`100.00% <100.00%> (ø)`
src/components/TokenDisplay.tsx	`100.00% <100.00%> (ø)`
src/icons/DownArrowIcon.tsx	`100.00% <ø> (ø)`
src/jumpstart/JumpstartEnterAmount.tsx	`93.33% <ø> (ø)`
src/send/EnterAmount.tsx	`94.03% <100.00%> (-1.27%)`	⬇️
src/send/SendEnterAmount.tsx	`93.75% <100.00%> (+0.13%)`	⬆️
src/send/usePrepareSendTransactions.ts	`86.20% <ø> (ø)`
src/components/TokenEnterAmount.tsx	`94.51% <97.56%> (+2.09%)`	⬆️

... and 68 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea0927...5e2dbee. Read the comment docs.

…ter-amount-hook

kathaypacific

amazing work!!! it looks really great 🚀 i had some small ponderings, and i think you may also need to adjust the e2e tests for the send flow?

kathaypacific · 2024-11-26T15:03:16Z

src/send/EnterAmount.tsx

@@ -355,58 +225,62 @@ function EnterAmount({
      >
        <View style={styles.inputContainer}>
          <Text style={styles.title}>{t('sendEnterAmountScreen.title')}</Text>


i think there is some vertical padding missing between the enter amount and title?

(i think this is outdated)

@kathaypacific should be outdated as it was missing after initial merge with "max" functionality but then I fixed it.

kathaypacific · 2024-11-28T14:50:03Z

src/components/TokenEnterAmount.tsx

+  return value.replaceAll(groupingSeparator, '').replaceAll(decimalSeparator, '.')
+}
+
+export function roundLocalNumber(value: BigNumber | null) {


Suggested change

export function roundLocalNumber(value: BigNumber | null) {

export function roundFiatValue(value: BigNumber | null) {

side thought, no need to address now but i wonder if this will be handy to have as a general function that can be used in other places like the transaction feed....

@kathaypacific I think it definitely will be. Considering that it is currently only used in this file - there's no point in moving it to a general utility yet file but we should keep this in mind for when its gonna be useful.

kathaypacific · 2024-11-28T14:53:33Z

src/components/TokenEnterAmount.tsx

@@ -48,6 +49,16 @@ export function formatNumber(value: string) {
    .replaceAll('_', groupingSeparator)
 }

+export function unformatNumberForProcessing(value: string) {


Suggested change

export function unformatNumberForProcessing(value: string) {

export function formatNumberToStandard(value: string) {

or parseNumberToStandardFormat or applyUSNumberFormatting?

@kathaypacific hmm, I'm still unsure on this one. It is only used in a single place so far to actually un-format already formatted number that comes from the input and format it for the processing purposes. I think it makes a more clear distinction for the specific purpose of the function:

it unformats the number (which implies it was formatted in the first place)

it formats it specifically for processing

I've only made is a separate function to simplify description as otherwise I'd need to write a comment explaining what it does and why, which feels more concise in a form of a clear function name.

In case of formatNumberToStandard/parseNumberToStandardFormat - we need to define what we mean by "standard". And in case of applyUSNumberFormatting - we need to understand why we specifically use US number formatting (even though I'm not sure it's US specific as up to this point I thought this was a general format that everyone uses and was always using it myself 😄)

At least, that's my train of though. What do you think about it?

kathaypacific · 2024-11-28T15:17:32Z

src/components/TokenEnterAmount.tsx

    handleToggleAmountType,
    handleAmountInputChange,
+    handlePercentage,


Suggested change

handlePercentage,

handleSelectPercentageAmount,

kathaypacific · 2024-11-28T15:22:52Z

src/send/EnterAmount.tsx

-    tokenBottomSheetRef.current?.close()
+  const onSelectToken: TokenBottomSheetProps['onTokenSelected'] = (selectedToken) => {
+    setToken(selectedToken)
+    setAmount('')


ah, i was wondering how this setAmount would be used. generally i don't like the pattern of components that have a state and give other components access to the state and the state setter because it gives consumers of this component the ability to do a lot (and blurs the responsibility of the consumer and the component, since the component can be controlled absolutely).

if the consumers only need to be able to reset the amount, what do you think about changing the return property of useEnterAmount here from setAmount to something like resetAmount?

@kathaypacific I totally agree and I tried to refrain from exposing this state setter till the very end but in this particular case it felt like it was the clearest way to do this without trying to overcomplicate it.

I will rename it to replaceAmount as I already have an identical functionality incoming in Swap flow (cause we have two fields there and second is always un-editable and have to be set programatically).

kathaypacific · 2024-11-28T15:43:23Z

src/send/EnterAmount.tsx

    // NOTE: analytics is already fired by the bottom sheet, don't need one here
  }

  const onSelectPercentageAmount = (percentage: number) => {
-    setTokenAmountInput(token.balance.multipliedBy(percentage).toFormat({ decimalSeparator }))
-    setEnteredIn('token')
+    handlePercentage(percentage)
    setSelectedPercentage(percentage)


the handlePercentage and setSelectedPercentage calls in series made me think that it might make sense to move the EnterAmountOptions into TokenEnterAmount, what do you think? (not in this PR, but seems like a fun friday afternoon thing for someone at some point)

@kathaypacific agree!

kathaypacific · 2024-11-28T15:45:09Z

src/send/EnterAmount.tsx

+  const isAmountLessThanBalance =
+    processedAmounts.token.bignum && processedAmounts.token.bignum.lte(token.balance)
+  const showLowerAmountError =
+    processedAmounts.token.bignum && !isAmountLessThanBalance && !disableBalanceCheck


could this be simplified to const showLowerAmountError = processedAmounts.token.bignum && processedAmounts.token.bignum.gt(token.balance) && !disableBalanceCheck?

kathaypacific · 2024-11-28T15:48:53Z

src/send/EnterAmount.tsx

+      }, FETCH_UPDATED_TRANSACTIONS_DEBOUNCE_TIME_MS)
+      return () => clearTimeout(debouncedRefreshTransactions)
+    },
+    [processedAmounts.token.bignum, token]


should we try to use primitive values here? i'm not sure how effective BigNumber and TokenBalance is at triggering this effect

Suggested change

[processedAmounts.token.bignum, token]

[processedAmounts.token.bignum.toString(), token.tokenId]

kathaypacific · 2024-11-28T15:55:01Z

src/send/EnterAmount.tsx

+            onOpenTokenPicker={tokenSelectionDisabled ? undefined : onOpenTokenPicker}
+          />
+
+          {!!maxFeeAmount && (


this is also something that we can consider as a follow up, wanted to note it while it's fresh in my mind. i think one of the goals of the enter amount work is also to standardise errors and the fees display. so perhaps it makes sense to move these parts into the TokenEnterAmount (or have a component that wraps them, idk). i think that there's going to be some amount of duplication for handling these things in the various flows

@kathaypacific I'm all for it! I will create a task for this and would love to work on it!

kathaypacific · 2024-11-28T16:00:38Z

src/send/EnterAmount.test.tsx

@@ -158,12 +160,12 @@ describe('EnterAmount', () => {
    })

    expect(getByTestId('SendEnterAmount/TokenAmountInput')).toBeTruthy()
-    expect(getByTestId('SendEnterAmount/LocalAmountInput')).toBeTruthy()
+    // expect(getByTestId('SendEnterAmount/Max')).toBeTruthy()


…th tests

kathaypacific

🥳

kathaypacific · 2024-11-29T17:14:54Z

ios/Podfile.lock

@@ -1339,4 +1339,4 @@ SPEC CHECKSUMS:

 PODFILE CHECKSUM: e49ec411923f9bb1906dff8d0e27d0aa410bf3ad

-COCOAPODS: 1.16.2
+COCOAPODS: 1.15.2


ah, perhaps we don't include this change since it's unrelated

emerge-tools · 2024-12-02T12:01:49Z

📸 Snapshot Test

Base build not found

No build was found for the base commit 1ea0927. 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

kathaypacific · 2024-12-02T12:09:51Z

hey @sviderock this is awesome!! i will merge in your absence :)

i added a few minor changes after testing on android, noting it here in case you disagree with the changes / have ideas for how to fix the issues in a better way:

fixed fee amount formatting on Android (the "network fee" label was always clipped and showed only "N...") by adjusting some styles (use flex: 0 instead of flexShrink: 1)
fix the amount set by the "max" amount - the token around was rounded to 6 decimals however this uses the round up policy which means the "you need to lower the amount" error is always triggered and the user cannot proceed. i am using token.decimals for now
perhaps related to a laggy android device but on clearing the amount, the network fee was still displayed. i added a condition to hide the network fee if the amount is an empty string
removed the secondary amount maxWidth so that we can always see the token symbol
(this one is personal preference during testing) removed the keyboard dismiss on tap of switch token / local amount, to make it easier to switch to fiat amount on screen open (since there is an autofocus on screen open and the keyboard would be dismissed and need to be manually opened again)

here are some "before changes" screenshots:

Screen_Recording_20241202_113305_Valora.dev.mp4

after changes video:

Screen_Recording_20241202_131828_Valora.dev.mp4

sviderock added 6 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

sviderock changed the base branch from main to slava/add-enter-amount-hook November 19, 2024 06:29

sviderock force-pushed the slava/change-send-flow branch from 6f89ffd to 1aa17d6 Compare November 19, 2024 06:58

sviderock added 2 commits November 19, 2024 09:07

fix tests

4ed89fc

fix jumpstart

d0c7099

sviderock added 6 commits November 19, 2024 09:54

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

sviderock marked this pull request as ready for review November 19, 2024 09:08

sviderock added 12 commits November 19, 2024 11:13

fixes

9360626

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 8 commits November 26, 2024 16:48

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

3858035

unskip "max" tests

4eb45ba

replace props.children check with .toHaveTextContent() in tests

639e612

fix lint

080de6d

add spacing for title

e0de08b

fix percentages

8001e05

rename handler for better clarity

ae82212

clear percentage on token select

7714b4d

kathaypacific reviewed Nov 28, 2024

View reviewed changes

sviderock added 9 commits November 29, 2024 12:20

change as per the review

b91f09d

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

fbcd741

revert some changes

757a759

remove unused ref

8aed766

fix 2e2

ac325bd

fix assets e2e

c5bc604

try to fix assets again

c0b5ab2

ignore currency symbols for unformat function, cover new functions wi…

81991b5

…th tests

revert changes with removing symbols

7c746d6

kathaypacific approved these changes Nov 29, 2024

View reviewed changes

kathaypacific added 5 commits December 2, 2024 12:26

fix: fee amount formatting on android

12b4e47

fix: avoid showing fees when no amount

6d6ae26

fix: more than max amount set when max amount selected

e54e741

fix: ensure token symbol displayed in secondary amount

1aa565d

chore: increase switch icon touch area

5e2dbee

kathaypacific added this pull request to the merge queue Dec 2, 2024

Merged via the queue into main with commit 295b9f5 Dec 2, 2024
15 of 16 checks passed

kathaypacific deleted the slava/change-send-flow branch December 2, 2024 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(TokenEnterAmount): add new flow to EnterAmount.tsx #6245

feat(TokenEnterAmount): add new flow to EnterAmount.tsx #6245

sviderock commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Nov 26, 2024

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024 •

edited

Loading

kathaypacific Nov 28, 2024

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024 •

edited

Loading

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024 •

edited

Loading

kathaypacific Nov 28, 2024

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024 •

edited

Loading

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024

kathaypacific Nov 28, 2024

kathaypacific Nov 28, 2024

kathaypacific Nov 28, 2024

sviderock Nov 29, 2024

kathaypacific Nov 28, 2024

kathaypacific left a comment

kathaypacific Nov 29, 2024

emerge-tools bot commented Dec 2, 2024

kathaypacific commented Dec 2, 2024 •

edited

Loading

	export function roundLocalNumber(value: BigNumber \| null) {
	export function roundFiatValue(value: BigNumber \| null) {

	export function unformatNumberForProcessing(value: string) {
	export function formatNumberToStandard(value: string) {

	[processedAmounts.token.bignum, token]
	[processedAmounts.token.bignum.toString(), token.tokenId]

feat(TokenEnterAmount): add new flow to EnterAmount.tsx #6245

feat(TokenEnterAmount): add new flow to EnterAmount.tsx #6245

Conversation

sviderock commented Nov 19, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Nov 29, 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

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerge-tools bot commented Dec 2, 2024

📸 Snapshot Test

Base build not found

kathaypacific commented Dec 2, 2024 • edited Loading

sviderock commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

sviderock Nov 29, 2024 •

edited

Loading

sviderock Nov 29, 2024 •

edited

Loading

sviderock Nov 29, 2024 •

edited

Loading

sviderock Nov 29, 2024 •

edited

Loading

kathaypacific commented Dec 2, 2024 •

edited

Loading