Swap Transaction screen redesign #3392

jagodarybacka · 2023-05-18T13:28:53Z

Resolves #3342
Resolves #2943
Resolves #3175
Resolves #3370
Resolves #2804

What

Redesign and fixes for Swaps.

Scope:

removed summary screen, Review swap button will take user to Sign Transaction screen
implemented balance change summary component on the Sign Transaction screen
added route and DAO fee info to transaction details
code improvements on Swaps page
removed unused files and merged transactions Details components
improved price truncate util

Testing

test in-wallet Swaps on various networks and with different tokens
test errornot enough for network fees

Test for regressions on other signing screens:

send
contract interactions
token approval
message signing

Latest build: extension-builds-3392 (as of Fri, 26 May 2023 11:15:38 GMT).

- change CTA on Swaps page - move around methods to cleanup code

- new design for sell/buy assets summary - details about route moved to details panel

To display last calculated price impact value on transaction screen let's copy that value from swap quote to transaction annotation.

Update `truncateDecimalAmount` function so it is better handling numbers with a lot of zeros in the decimal part.

Shadowfiend

For future reference, this would have been much easier to review as 3-4 separate PRs 😬 Dropping a quick review of the truncateDecimalAmount changes, which I love, and which I think can be split out to a PR by themselves fwiw.

I'll continue to move through the changes today.

Shadowfiend · 2023-05-22T17:21:43Z

background/lib/utils/index.ts

+ * @param value floating point number as a string or number
+ * @param decimalLength desired length of decimal part
+ * @param maxDecimalLength max length of decimal part - will try to look
+ *                        for sgnificant digits up to this point


Suggested change

* for sgnificant digits up to this point

* for significant digits up to this point

background/lib/utils/index.ts

background/lib/tests/utils.unit.test.ts

background/lib/utils/index.ts

background/lib/tests/utils.unit.test.ts

Shadowfiend

Did a second round of review. Last bit will probably have to wait until tomorrow, my brain is leaking swap material 😅

background/redux-slices/0x-swap.ts

ui/components/Signing/SignatureDetails/TransactionSignatureDetails/DetailsPanel.tsx

ui/_locales/en/messages.json

ui/components/SignTransaction/SignTransactionPanelCombined.tsx

Shadowfiend · 2023-05-22T19:24:15Z

ui/components/Signing/SignatureDetails/TransactionSignatureDetails/DetailsPanel.tsx

-import React, { ReactElement } from "react"
-import { EnrichedEVMTransactionRequest } from "@tallyho/tally-background/services/enrichment"
-import SignTransactionDetailPanel from "../../../SignTransaction/SignTransactionDetailPanel"
+/* eslint-disable react-hooks/exhaustive-deps */


Would love to find a way to kill this, btw. Probably not worth futzing with it right now though 🙈

...ignatureDetails/TransactionSignatureDetails/TransactionSignatureSummary/SwapAssetSummary.tsx

background/lib/tests/utils.unit.test.ts

...ignatureDetails/TransactionSignatureDetails/TransactionSignatureSummary/SwapAssetDetails.tsx

Shadowfiend · 2023-05-23T20:59:36Z

background/lib/utils/index.ts

 export function truncateDecimalAmount(
  value: number | string,
-  decimalLength: number
+  decimalLength: number,
+  maxDecimalLength = decimalLength


Incidentally we should probably add this parameter to the truncateDecimalAmount call in SignTransactionTransferInfoProvider!

Isn't that component used only in the unused old transactions flow?

… Lol. I guess I'm cleaning that up now before I embarrass myself again 😆

The new flow uses localizedDecimalAmount—which is unfortunately subject to the problem of not always showing the amount of precision we might prefer.

Bleh. Can't make myself go down that rabbit hole right now 😅

Anyway, I said I'd clean it up and I'm cleaning it up (#3404 😝).

Shadowfiend

Still not quite done, but almost there. Left a couple more notes—I like how much removing the final quote has removed from our swap page state machine 😅

Shadowfiend · 2023-05-23T21:00:51Z

ui/components/Swap/SwapQuoteBalanceChange.tsx

+  const rawFromAmount = formatUnits(fromAsset.amount, fromAsset.asset.decimals)
+  const fromAmount = truncateDecimalAmount(rawFromAmount, 2, 8)
+  const rawToAmount = formatUnits(toAsset.amount, toAsset.asset.decimals)
+  const toAmount = truncateDecimalAmount(rawToAmount, 2, 8)


Non-blocking: I kind of hate that we're doing this stuff manually when some version of it has already been done with AssetDecimalAmount conversion. Idly wondering if we can standardize this a little more and push it into AssetDecimalAmount, where we also handle localization...

Yeah definitely this is something we should clean up 👀

Edit: so after some digging it seems like we have even more redundancy around truncating amounts than I expected 😞 do you think that we should edit enrichAssetAmountWithDecimalValues so it is using truncateDecimalAmount? Do we want to keep using toLocaleString?

I think we do need to continue to use toLocaleString—in many ways I hate that we've got truncateDecimalAmount at all, because it makes assumptions about number format that aren't localizable.

We do have the tools to unify all of it by taking the number and then passing it to toLocaleString, or using formatToParts and dealing with the fraction component. That said, I don't think it's a pressing concern at the moment.

ui/components/Swap/SwapQuoteBalanceChange.tsx

ui/pages/Swap.tsx

Shadowfiend

Still not quite done, but almost there. Left a couple more notes—I like how much removing the final quote has removed from our swap page state machine 😅

jagodarybacka · 2023-05-24T15:02:22Z

Alright so there is still a problem that goes like this - sometimes we are able to show $ price on the swap screen but on the transaction screen we are losing it.

It happens because swaps screen is using checkCurrencyAmount that will fetch the prices if there are no recent prices saved on the asset. Transaction screen is using saved recent prices which may be empty for some assets.

I think we should ensure that if prices are fetched then they are saved/cached so we can avoid problems like that and just use one method to get $ price where needed instead of having multiple ways to do the same thing which is causing bugs and problems.

But I think it is not blocking to this PR so I'll undraft it and I can continue fixing that in the next PR.

EDIT: added issue #3406

- simplify the code - don't treat `decimalLength=0` as en exception - adjust unit tests Co-Authored-By: Antonio Salazar Cardozo <savedfastcool@gmail.com>

Shadowfiend · 2023-05-24T18:29:19Z

I think we should ensure that if prices are fetched then they are saved/cached so we can avoid problems like that and just use one method to get $ price where needed instead of having multiple ways to do the same thing which is causing bugs and problems.

But I think it is not blocking to this PR so I'll undraft it and I can continue fixing that in the next PR.

Agree across the board, love the thinking. One way to force-fetch prices that also updates the cache ✅ and pushing to a separate PR ✅

ui/utils/swap.ts

Shadowfiend

I believe most of my concerns are cleared here, so going to move into testing tomorrow.

Shadowfiend

This looks awesome 🤩 Approving, but going to give @VladUXUI a chance to do some design QA before landing this. Let's rock and roll 🎸

VladUXUI · 2023-05-26T05:59:23Z

This is working and looking awesome! No changes on my part!

Shadowfiend · 2023-05-26T11:12:08Z

Lfg!

@kkosiorowska

## What's Changed * Verified/Unverified tokens by @kkosiorowska in #3394 * Swap Transaction screen redesign by @jagodarybacka in #3392 * Fixes for verified/unverified assets by @kkosiorowska in #3410 * Opening activity details from token verification level by @kkosiorowska in #3416 * Add altlayer.io to MM impersonation list by @jagodarybacka in #3422 * Fix the issue of adding a custom token by @kkosiorowska in #3423 * Remove the verified/unverified assets by @kkosiorowska in #3413 * Add zkSync custom network to NFTs supported networks by @jagodarybacka in #3425 * v0.35.0 by @kkosiorowska in #3407 **Full Changelog**: v0.35.0...v0.36.0 Latest build: [extension-builds-3427](https://github.com/tahowallet/extension/suites/13298114425/artifacts/726017768) (as of Thu, 01 Jun 2023 11:24:04 GMT).

…n` (#3404) Draft until #3392 lands. This is the first step in eliminating all vestiges of the old transaction signing flow. The feature flag can't be dropped yet, as there are a few other entanglements left. A few components were still in use: - Several around Ledger status; these have been moved to SignerLedger/ under the new signing structure. - The loading screen for signature data was still in use; it's been moved to `Signing/SigningLoading`. - The network account info top bar, which is static in the signing flow (instead of modifiable in the main wallet) was also in SignTransaction; it's been moved to `SigningNetworkAccountInfoTopBar` under `Signing`. - The `SignTransactionDetailWarning` component, used to display warnings such as when assets are being transferred directly to a contract address; it's been moved to `TransactionSignatureDetailsWarning` under the `TransactionSignatureDetails` section of `Signing`. Other than name adjustments, the moved components have no changes. ## To Test Testing is mostly focused on making sure we're still following the new flow in signing, since we're ripping out a lot of code here. - [x] Ensure that each type of transaction signing has its summary data displayed correctly. - [x] ETH transfer (can be from the extension). - [x] ERC20 transfer (can be from the extension). - [x] Approval transaction (the Tally test wallet does not have SUSHI approved, so an easy way is to try to swap SUSHI). - [x] Normal contract interaction (a swap with an approved asset will work here). - [x] For all of the above, make sure fee changes are applied correctly and, if there are other internal form elements (e.g., the approval transaction amount), that these are also applied correctly. - [x] Ensure that each type of data signing has its summary data displayed correctly. - [x] Sign In With Ethereum. Can be checked at https://login.xyz/ under Get Started. - [x] Check regular message signing still looks visually consistent. Can be checked at https://mintkudos.xyz/ on Polygon by hitting Connect Wallet. - [x] Check typed data signing still looks visually consistent. Can be checked at https://app.uniswap.org/#/swap trying to swap from USDC using the Tally test wallet by clicking “Allow the Uniswap Protocol to use your USDC”. Do not confirm this signature so that it remains a viable test path for later. Ensure addresses can be clicked to be copied, and ensure the Etherscan icon is not orange unless hovered. - [x] Ensure Ledger accounts are going through the new Ledger flow by checking that connection status is displayed at the top of the signing screen for data, typed data, and contract interactions. Latest build: [extension-builds-3404](https://github.com/tahowallet/extension/suites/13248105974/artifacts/722152504) (as of Tue, 30 May 2023 16:44:37 GMT).

jagodarybacka added 4 commits May 16, 2023 16:01

Remove unused file SignTransactionSwapAssetInfoProvider

1a2c9a3

Get rid of the Swap Summary page and refactor Swaps page

65e923a

- change CTA on Swaps page - move around methods to cleanup code

Add new components to swap transaction screen

73da0bc

- new design for sell/buy assets summary - details about route moved to details panel

Remove unused components

1e32d2d

jagodarybacka self-assigned this May 18, 2023

jagodarybacka added 7 commits May 19, 2023 12:23

Add estimated swap price impact to transaction annotation

c3700c3

To display last calculated price impact value on transaction screen let's copy that value from swap quote to transaction annotation.

Merge branch 'main' into swap-tx-redesign

b1ad589

Improve truncating decimal amounts

fbcd9e2

Update `truncateDecimalAmount` function so it is better handling numbers with a lot of zeros in the decimal part.

Move SignTransactionDetaillPanel to DetailPanel

0df2a87

Fix PriceDetails warning styles

f41c2ee

Add swap annotation type guard

52f7489

Fix empty space lefy by "not enough for fees" warning

2b115ee

Shadowfiend requested changes May 22, 2023

View reviewed changes

Shadowfiend reviewed May 22, 2023

View reviewed changes

jagodarybacka added 3 commits May 23, 2023 15:37

Changes in the translation strings

7cb4773

Add more test cases for truncateDecimalAmount

a981abc

Move 0x swap selectors to separate file

f5339e7

Shadowfiend reviewed May 23, 2023

View reviewed changes

background/lib/tests/utils.unit.test.ts Outdated Show resolved Hide resolved

Shadowfiend reviewed May 23, 2023

View reviewed changes

...ignatureDetails/TransactionSignatureDetails/TransactionSignatureSummary/SwapAssetDetails.tsx Outdated Show resolved Hide resolved

Shadowfiend reviewed May 23, 2023

View reviewed changes

jagodarybacka added 3 commits May 24, 2023 13:28

Use keyPrefix to get swap details translations

f7f7bac

Add switcher component for additional transaction details

6657473

Fix types in selectSwapBuyAssets selector

16056f1

jagodarybacka marked this pull request as ready for review May 24, 2023 15:03

jagodarybacka and others added 2 commits May 24, 2023 17:07

Fix unused imports

0e06347

Refactor truncateDecimalAmount util

2b0b23f

- simplify the code - don't treat `decimalLength=0` as en exception - adjust unit tests Co-Authored-By: Antonio Salazar Cardozo <savedfastcool@gmail.com>

Shadowfiend mentioned this pull request May 25, 2023

Unsigned Integers: Remove everything in ui/components/SignTransaction #3404

Merged

11 tasks

Shadowfiend reviewed May 25, 2023

View reviewed changes

ui/utils/swap.ts Outdated Show resolved Hide resolved

Shadowfiend reviewed May 25, 2023

View reviewed changes

jagodarybacka added 2 commits May 25, 2023 11:06

Change color names

1b3fc8d

Remove outdated TODO comment

63ae78f

jagodarybacka mentioned this pull request May 25, 2023

Missing $ price on swap transaction screen #3406

Open

Shadowfiend approved these changes May 25, 2023

View reviewed changes

Merge branch 'main' into swap-tx-redesign

4f47245

Shadowfiend enabled auto-merge May 26, 2023 11:12

Shadowfiend merged commit 65fa6b2 into main May 26, 2023

Shadowfiend deleted the swap-tx-redesign branch May 26, 2023 11:15

Shadowfiend mentioned this pull request May 26, 2023

Broken estimated network fee on swap summary screen #3347

Open

kkosiorowska mentioned this pull request Jun 1, 2023

v0.36.0 #3427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap Transaction screen redesign #3392

Swap Transaction screen redesign #3392

jagodarybacka commented May 18, 2023 •

edited by github-actions bot

Loading

Shadowfiend left a comment

Shadowfiend May 22, 2023

Shadowfiend left a comment

Shadowfiend May 22, 2023

Shadowfiend May 23, 2023

jagodarybacka May 24, 2023

Shadowfiend May 24, 2023

Shadowfiend May 25, 2023

Shadowfiend left a comment

Shadowfiend May 23, 2023

jagodarybacka May 24, 2023 •

edited

Loading

Shadowfiend May 24, 2023

Shadowfiend left a comment

jagodarybacka commented May 24, 2023 •

edited

Loading

Shadowfiend commented May 24, 2023

Shadowfiend left a comment

Shadowfiend left a comment

VladUXUI commented May 26, 2023

Shadowfiend commented May 26, 2023

	* for sgnificant digits up to this point
	* for significant digits up to this point

Swap Transaction screen redesign #3392

Swap Transaction screen redesign #3392

Conversation

jagodarybacka commented May 18, 2023 • edited by github-actions bot Loading

What

Testing

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a 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

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagodarybacka May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

jagodarybacka commented May 24, 2023 • edited Loading

Shadowfiend commented May 24, 2023

Shadowfiend left a comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

VladUXUI commented May 26, 2023

Shadowfiend commented May 26, 2023

jagodarybacka commented May 18, 2023 •

edited by github-actions bot

Loading

jagodarybacka May 24, 2023 •

edited

Loading

jagodarybacka commented May 24, 2023 •

edited

Loading