feat(ReviewTransaction): add new review component to SendConfirmation #6436

sviderock · 2025-01-22T12:08:46Z

Description

This PR adds new ReviewTransaction component to the SendConfirmation.

Test plan

Fixed existing tests.

Related issues

Relates to RET-1295

Backwards compatibility

Yes

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)

…t-item-to-new-review

…iew-to-send-flow

codecov · 2025-01-22T12:15:57Z

Codecov Report

Attention: Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (f34cfbe) to head (7b12103).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/send/SendConfirmation.tsx	94.59%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6436   +/-   ##
=======================================
  Coverage   89.11%   89.11%           
=======================================
  Files         733      731    -2     
  Lines       31907    31859   -48     
  Branches     6099     6082   -17     
=======================================
- Hits        28433    28391   -42     
+ Misses       3427     3422    -5     
+ Partials       47       46    -1

Files with missing lines	Coverage Δ
src/account/utils.ts	`100.00% <100.00%> (ø)`
src/components/Avatar.tsx	`100.00% <100.00%> (ø)`
src/recipients/recipient.ts	`78.90% <100.00%> (ø)`
src/send/SendConfirmation.tsx	`94.44% <94.59%> (-1.39%)`	⬇️

... and 2 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 f34cfbe...7b12103. Read the comment docs.

…iew-to-send-flow

kathaypacific

looking great!

kathaypacific · 2025-01-27T09:34:38Z

src/send/__snapshots__/SendConfirmation.test.tsx.snap

-      "bottom": "additive",
-      "left": "additive",
-      "right": "additive",
+      "bottom": "off",


can we remove the test that generates this snapshot? snapshot tests are particularly ineffective in a refactor like this

Sure, I didn't know removal of snapshots are allowed! (assumed there was a reason for existing ones to be present considering that there are only a few of those)

@kathaypacific further down the line, should I also remove snapshots if any other changes for other tasks involve updating snapshot? Or should that decision depend on the task/component?

yes please remove any snapshots tests going forward that are not adding any value! ideally we want to replace all snapshot tests but sometimes you don't want to get bogged down with rewriting tests, so it depends on the task. but any suite where it looks like there's coverage over the main functionality already, please feel free to delete the snapshots tests

kathaypacific · 2025-01-27T09:36:48Z

src/send/SendConfirmation.test.tsx

-    )
-
-    expect(queryByTestId('RecipientAddress')).toBeFalsy()
-  })


should we backfill on some tests about what is rendered? i know there are unit tests for the components that are used, but tests here would help to ensure that the components are used correctly

(this doesn't have to be elaborate btw, but one test that asserts that a happy scenario is rendered might be helpful?)

Of course! My understanding was that if the current tests were enough for the existing screen - they should be fine for the new one, considering that it is basically just a redesign. So I removed the tests that are now covered with the reusable component.
But I see the point as it totally makes sense to ensure that all of the old and new pieces are rendered correctly. Will do!

kathaypacific · 2025-01-27T09:41:16Z

src/send/SendConfirmation.tsx

  const walletAddress = useSelector(walletAddressSelector)
  const feeCurrencies = useSelector((state) => feeCurrenciesSelector(state, tokenInfo!.networkId))
-
-  const dispatch = useDispatch()
+  const { maxFeeAmount, feeCurrency: feeTokenInfo } =


can we please double check whether we want to be showing the maxFeeAmount or the estimatedFeeAmount? this doesn't matter so much on most networks but when transacting on Ethereum, this could make a big difference. on the designs i see that we are using the approx symbol so i guess that we want to show estimatedFeeAmount...

What's the best way for me to check this? Considering that this is the same value that was used for all this time I presumed that this was an intended way (as it wouldn't ever be the case that we estimated some X value and but it appeared to be bigger once the transaction was completed). Should I check it out with Kayla or Laura?

yes perhaps this is a detail we can check separately, i can help to start a thread in figma. for context, we discussed this at length with swaps because we saw the max vs estimated gas fees differed by $20-60USD at times on Ethereum, of course this depends on the gas price and usd price of ETH on the day but this is why we decided to show estimated value rather than max for swaps (and show the max in the bottom sheet instead)

Thank you, I'll pick it up from there and will re-post the final decision here for visibility!

kathaypacific · 2025-01-27T09:41:38Z

src/send/SendConfirmation.tsx

+      token: formatValueToDisplay(tokenFeeAmount),
+      local: localFeeAmount ? formatValueToDisplay(localFeeAmount) : undefined,
+    }),
+    [localFeeAmount, tokenFeeAmount, feeTokenInfo]


Suggested change

[localFeeAmount, tokenFeeAmount, feeTokenInfo]

[localFeeAmount, tokenFeeAmount]

i wonder if this variable is necessary given that we only use it in one place below?

Forgot to remove after the last changes to this variable. Will remove!

kathaypacific · 2025-01-27T09:45:32Z

src/send/SendConfirmation.tsx

@@ -174,124 +138,101 @@ function SendConfirmation(props: Props) {
        tokenId,
        usdAmount,
        recipient,
-        fromModal,
+        // TODO remove fromModal from `sendPayment`
+        false,


does this break any existing flows? taking a quick look i think it affects how the navigation works after the transaction is finished, perhaps it is worth keeping this variable and renaming it instead?

As I was looking at it I thought that once the modal is removed then we'll always redirect the user to the home page (cause now its a separate screen and not the overlay modal). But you're right, I misunderstood it. It's not as much related to whether its overlay or separate screen but more to the aspect of whether its a request from a DAPP that expects you to get back to it once you press back button instead of being redirected to the home screen (in this case bidali and it's also the only thing using this different redirect).

I will adjust the name of the screen in Screens and rename all the related arguments/variable.

@kathaypacific I've reverted this change for this PR. It will not disrupt the existing flows anymore but the naming change will involve renaming all the occurrences of the screen name and fromModal argument which will add ~10 extra files changes + update of RootStateSchema so I would suggest committing renames in the follow-up PR, would you agree?

oh yes totally, makes sense

@kathaypacific here's the PR for the renaming!

kathaypacific · 2025-01-27T09:46:00Z

src/send/SendConfirmation.tsx

-          : undefined
-      }
+    <ReviewTransaction
+      title="Review Send"


should this use translations?

kathaypacific · 2025-01-27T09:46:21Z

src/send/SendConfirmation.tsx

+          {tokenInfo && (
+            <ReviewSummaryItem
+              testID="SendConfirmationToken"
+              label="Sending"


should this use translations?

kathaypacific · 2025-01-27T09:47:03Z

src/send/SendConfirmation.tsx

+          <ReviewDetailsItem
+            testID="SendConfirmationNetwork"
+            label={t('transactionDetails.network')}
+            value={tokenInfo && NETWORK_NAMES[tokenInfo.networkId]}


Suggested change

value={tokenInfo && NETWORK_NAMES[tokenInfo.networkId]}

value={NETWORK_NAMES[tokenInfo.networkId]}

kathaypacific

🚀✨

sviderock added 12 commits January 22, 2025 11:54

add initial structure for ReviewTransaction component

a05b888

add contact item to the ReviewTransaction

ec19eab

add to ignorelist in knip for now

e020751

Merge branch 'slava/new-review-screen-skeleton' into slava/add-contac…

47c821a

…t-item-to-new-review

add component for total line

38a9733

use new ReviewTransaction component for SendConfirmation

a1ba00b

remove unused ReviewFrame component

346f24c

remove unused ShortenedAddress component, move its function outside

be50ea4

add translations

ffad452

Merge branch 'slava/add-total-value-to-review' into slava/add-new-rev…

93e5d47

…iew-to-send-flow

add more translations

14a7a71

fix tests

b37fdbf

fix translations being in a wrong place

0719a2c

sviderock mentioned this pull request Jan 22, 2025

feat: add new Review component and apply it to SendConfirmation #6402

Closed

1 task

sviderock added 15 commits January 22, 2025 18:05

fix some values not being formatted

7f69607

Merge branch 'slava/add-total-value-to-review' into slava/add-new-rev…

302c6af

…iew-to-send-flow

Merge branch 'main' into slava/add-total-value-to-review

07cb0fb

fix translations

9a1b08b

fix test

c607567

Merge branch 'slava/add-total-value-to-review' into slava/add-new-rev…

851a2b5

…iew-to-send-flow

fix snapshot and translation

a716663

fixes as per review

dbe721a

Merge branch 'main' into slava/add-total-value-to-review

4ac4793

Merge branch 'slava/add-total-value-to-review' into slava/add-new-rev…

a30c611

…iew-to-send-flow

update snapshot

b4f3d43

fix e2e and bad merge issues

9da3945

fix as per review

994fbf9

Merge branch 'main' into slava/add-total-value-to-review

0b3e84c

Merge branch 'slava/add-total-value-to-review' into slava/add-new-rev…

6e3b7d9

…iew-to-send-flow

sviderock force-pushed the slava/add-new-review-to-send-flow branch from bcc174b to 6e3b7d9 Compare January 24, 2025 14:26

Base automatically changed from slava/add-total-value-to-review to main January 24, 2025 15:05

Merge branch 'main' into slava/add-new-review-to-send-flow

b7b2bab

kathaypacific reviewed Jan 27, 2025

View reviewed changes

sviderock added 2 commits January 27, 2025 13:09

fix most issues as per review besides renaming fromModal

e6b32bb

revert fromModal change

7b12103

kathaypacific approved these changes Jan 27, 2025

View reviewed changes

sviderock added this pull request to the merge queue Jan 27, 2025

Merged via the queue into main with commit dcac9cf Jan 27, 2025
15 checks passed

sviderock deleted the slava/add-new-review-to-send-flow branch January 27, 2025 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ReviewTransaction): add new review component to SendConfirmation #6436

feat(ReviewTransaction): add new review component to SendConfirmation #6436

sviderock commented Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

kathaypacific left a comment

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025 •

edited

Loading

sviderock Jan 27, 2025 •

edited

Loading

kathaypacific Jan 27, 2025

kathaypacific Jan 27, 2025

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025 •

edited

Loading

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025

sviderock Jan 27, 2025 •

edited

Loading

kathaypacific Jan 27, 2025

sviderock Jan 27, 2025

kathaypacific Jan 27, 2025

kathaypacific Jan 27, 2025

kathaypacific Jan 27, 2025

kathaypacific left a comment

	[localFeeAmount, tokenFeeAmount, feeTokenInfo]
	[localFeeAmount, tokenFeeAmount]

	value={tokenInfo && NETWORK_NAMES[tokenInfo.networkId]}
	value={NETWORK_NAMES[tokenInfo.networkId]}

feat(ReviewTransaction): add new review component to SendConfirmation #6436

feat(ReviewTransaction): add new review component to SendConfirmation #6436

Conversation

sviderock commented Jan 22, 2025

Description

Test plan

Related issues

Backwards compatibility

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

sviderock Jan 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 27, 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

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

codecov bot commented Jan 22, 2025 •

edited

Loading

sviderock Jan 27, 2025 •

edited

Loading

sviderock Jan 27, 2025 •

edited

Loading

sviderock Jan 27, 2025 •

edited

Loading

sviderock Jan 27, 2025 •

edited

Loading