fix: swap max for non native assets #5923

herbig · 2023-12-21T19:27:26Z

Description

Brings back the existing Max button in the TradeAssetInput component, for ERC20 tokens. This allows users to easily fill the input field with their max balance.

This was originally removed to simplify the logic for calculating gas costs, but since ERC20 tokens are not used for paying native gas it is not an issue to bring it back for those.

Pull Request Type

🐛 Bug fix (Non-breaking Change: Fixes an issue)
🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #5828

Risk

This change adds a listener to the Max button, causing it to appear, and fills the input field with the total balance, for non-native (ERC20) tokens only.

This is low-risk, it is unlikely there are unintended consequences to this. Users are generally familiar with the placement and function of a Max button, so should understand what it is doing.

Testing

Engineering

View the swap (Trade/Bridge) feature at http://localhost:3000/#/trade
Confirm that the Max button appears when swapping FROM an ERC20 token to anything else.
Confirm that clicking the Max button inputs the value displayed in the top right (Balance) into the input field.
Confirm that the Max button DOES NOT appear when swapping from a native currency (e.g. ETH).

Operations

This is user facing. Testing can be the same as above.

Screenshots (if applicable)

src/components/DeFi/components/AssetInput.tsx

src/components/DeFi/components/PercentOptionsButtonGroup.tsx

herbig · 2023-12-21T19:30:59Z

src/components/DeFi/components/PercentOptionsButtonGroup.tsx

+    }  else if (onPercentOptionClick) {
+      onPercentOptionClick(option)
+    }
+  }, [onPercentOptionClick, onMaxClick, option])


changed the logic slightly, since onPercentOptionClick is now optional

Since both are now optional, we could end up in a situation where we forget to pass onMaxClick, and the regular onClick handler gets fired, which could have very different logic as the max one

0xApotheosis

Solid start. A few initial thoughts:

We'll want to use the ASSET_NAMESPACE constant from the caip package, rather than the erc20 string literal.
Whilst the ticket AC does indeed say we only want to handle erc20s, we'll also likely want to handle bep20s (Binance Smart Chain), too. You'll notice non-native BSC assets do not currently support send max in this branch.
We might want to look at using the isEvmChainId and isNativeEvmAsset helpers to achieve this in a more maintainable way.
A few lint issues to fix up 🙏

herbig · 2023-12-21T21:33:14Z

Solid start. A few initial thoughts:

* We'll want to use the `ASSET_NAMESPACE` constant from the `caip` package, rather than the `erc20` string literal.

* Whilst the ticket AC does indeed say we only want to handle `erc20`s, we'll also likely want to handle `bep20`s (Binance Smart Chain), too. You'll notice non-native BSC assets do not currently support send max in this branch.

* We might want to look at using the `isEvmChainId` and `isNativeEvmAsset` helpers to achieve this in a more maintainable way.

* A few lint issues to fix up 🙏

thanks @0xApotheosis those helper functions are great, and good call about bep20, this should now be handled

woodenfurniture · 2023-12-21T21:55:43Z

Hey @herbig thanks for the PR! 🍻

src/components/MultiHopTrade/components/TradeAssetInput.tsx

gomesalexandre

Solid start indeed, thanks a lot for the contribution @herbig!

Following on the great review from @0xApotheosis, I've added a few comments, as this implementation is sane for the most part at runtime but potentially brings some major type safety risks.

From a functional perspective, the max button is indeed available for ERC-20 sell assets:

It looks like the scientific notation is used however, versus precision in the send modal, see:

You can see the send modal implementation for reference BigNumber.toPrecision() is what you want here:

web/src/components/Modals/Send/hooks/useSendDetails/useSendDetails.tsx

Line 199 in 0f05cf8

setValue(SendFormFields.CryptoAmount, cryptoHumanBalance.toPrecision())

Fiat max. is also happy.

And confirmed the max. button is only shown for EVM tokens.

gomesalexandre · 2023-12-21T21:41:39Z

src/components/DeFi/components/PercentOptionsButtonGroup.tsx

@@ -41,15 +45,15 @@ export const PercentOptionsButton: React.FC<MaxButtonProps> = ({

 type MaxButtonGroupProps = {
  isDisabled?: boolean
-  onClick: (args: number) => void
+  onPercentOptionClick?: (args: number) => void


Since this is a shared component, we absolutely want a discriminated union here - one or the other should always be present: if we ever get to rendering this component, we should have an event handler for it.

gomesalexandre · 2023-12-21T21:45:59Z

src/components/DeFi/components/PercentOptionsButtonGroup.tsx

+    }  else if (onPercentOptionClick) {
+      onPercentOptionClick(option)
+    }
+  }, [onPercentOptionClick, onMaxClick, option])


Since both are now optional, we could end up in a situation where we forget to pass onMaxClick, and the regular onClick handler gets fired, which could have very different logic as the max one

gomesalexandre · 2023-12-21T21:50:19Z

src/components/MultiHopTrade/components/TradeAssetInput.tsx

+  const showMax = useMemo(
+    () => isEvmChainId(fromAssetId(assetId).chainId) && !isNativeEvmAsset(assetId),
+    [assetId],
+  )


The rendering of the button should be controlled by the presence of percentOptions, not that of the handler.

I believe a more semantic way to achieve this would be to pass the current default percentOptions ([1]) array as percentOptions if this condition is true, and an empty array otherwise.

This way, we would pass this consistently (at least for this component), and have the options rendering logic in a better place.

@gomesalexandre is there an instance in the app where we currently display percentOptions that I could see? I couldn't find any but maybe I missed it. My initial thought was that if it wasn't being used the code should just be pulled out altogether and only support the Max button.

<TradeAssetInput itself is passed percentOptions. If you inspect consumers, eventually reaching <TradeInput />, you'll see this guy:

web/src/components/MultiHopTrade/components/TradeInput/TradeInput.tsx

Line 105 in d613a96

const percentOptions = [1]

This is the prop that is controling which percent options are being displayed - showMax here is effectively redundant, as we're passing 1 (max) as an option from the parent, but then have children programmatically (not) render it

gomesalexandre · 2023-12-21T21:53:57Z

src/components/MultiHopTrade/components/TradeAssetInput.tsx

+  }, [balance, props])
+
+  const showMax = useMemo(
+    () => isEvmChainId(fromAssetId(assetId).chainId) && !isNativeEvmAsset(assetId),


We also have a isToken method you may want to take a look at to keep things declarative, it effectively checks for "is non-native asset", which realistically means EVM chain tokens currently, though that could change in the future (as e.g Cosmos SDK chains can have tokens as wells, though we do not support those in the swapper) so the isEvmChainId() check is definitely welcomed here!

@gomesalexandre hmm yeah maybe isToken is more future thinking, it would be better that if Cosmos support is added later on that the Max button will appear by default

Either or, that one is fine as-is as we most likely won't support tokens from non-EVM chains for trading in the future but we may. Would prefer it regardless as it reads nicer and is more declarative

herbig · 2023-12-22T18:42:02Z

It looks like the scientific notation is used however, versus precision in the send modal, see:

@gomesalexandre oh good catch, yeah I didn't see that since I only checked a few assets that would not have triggered scientific notation

maybe I'm missing something, but to me it seems both of those screenshots would be wrong.

Scientific notation in the Swap field isn't great UX, and I'll need to fix that.

But in the Send screen it looks as though you're sending 4.414, which isn't accurate.

Also so we're clear, the Max button in Send is already in production, and unrelated to Max here in the Swap, so I think we have a bug there in production. I've just tried prod Max and got this:

gomesalexandre · 2024-01-03T15:10:07Z

It looks like the scientific notation is used however, versus precision in the send modal, see:

@gomesalexandre oh good catch, yeah I didn't see that since I only checked a few assets that would not have triggered scientific notation

maybe I'm missing something, but to me it seems both of those screenshots would be wrong.

Scientific notation in the Swap field isn't great UX, and I'll need to fix that.

The scientific notation should never be user-facing, we always want to have either full-precision when applicable, or trimmed decimal places depending on the screens and product spec. For things like sends and swapping, we want to ensure we display the actual amounts that users are going to send out of their wallet.

i.e when sending FOX, you can see that the balance (for display purposes only) has trimmed precision, but max. click actually inputs the full-precision amount, with 18 digits in the case of FOX:

But in the Send screen it looks as though you're sending 4.414, which isn't accurate.

Not sure what do you mean here @herbig? Clicking max/percent options will replace the input with the prorated one, with full asset precision, i.e in the screenshot above 115.618374110339184543 FOX (not e.g 115.61 FOX)

Also so we're clear, the Max button in Send is already in production, and unrelated to Max here in the Swap, so I think we have a bug there in production. I've just tried prod Max and got this:

That's correct as per our current flow, though definitely not the best UX: we do fees deduction (on native assets only obviously) so trying to send 0.0003 ETH would result in more fees than the actual balance, hence the negative value.

0xean · 2024-01-08T21:50:19Z

@herbig - thanks for the PR and work on this issue. @gomesalexandre is going to take it over from here since we haven't heard from you in a bit.

I would love to send some FOX tokens your way as a way to thank you for the work on this, if you want to DM me I can get that arranged. Thanks again for your contributions.

gomesalexandre · 2024-01-08T22:56:27Z

Closing in favor of #5961

bring back Max button for ERC20 tokens #5828

1a192ac

herbig requested a review from a team as a code owner December 21, 2023 19:27

herbig commented Dec 21, 2023

View reviewed changes

src/components/DeFi/components/AssetInput.tsx Show resolved Hide resolved

herbig commented Dec 21, 2023

View reviewed changes

src/components/DeFi/components/PercentOptionsButtonGroup.tsx Show resolved Hide resolved

herbig commented Dec 21, 2023

View reviewed changes

0xApotheosis reviewed Dec 21, 2023

View reviewed changes

herbig added 2 commits December 21, 2023 16:28

lint fixes

ca51c48

use helper functions

e883b6b

Merge branch 'develop' into readd-max-5828

c735c48

0xApotheosis reviewed Dec 21, 2023

View reviewed changes

src/components/MultiHopTrade/components/TradeAssetInput.tsx Outdated Show resolved Hide resolved

herbig added 2 commits December 21, 2023 16:59

update from review

5784340

Merge branch 'develop' into readd-max-5828

67e6dbd

gomesalexandre reviewed Dec 21, 2023

View reviewed changes

Merge branch 'develop' into readd-max-5828

d613a96

gomesalexandre mentioned this pull request Jan 8, 2024

feat: non native assets max swaps #5961

Merged

3 tasks

gomesalexandre closed this Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: swap max for non native assets #5923

fix: swap max for non native assets #5923

herbig commented Dec 21, 2023

herbig Dec 21, 2023

gomesalexandre Dec 21, 2023

0xApotheosis left a comment

herbig commented Dec 21, 2023

woodenfurniture commented Dec 21, 2023

gomesalexandre left a comment

gomesalexandre Dec 21, 2023

gomesalexandre Dec 21, 2023

gomesalexandre Dec 21, 2023

herbig Dec 22, 2023

gomesalexandre Jan 3, 2024

gomesalexandre Dec 21, 2023

herbig Dec 22, 2023

gomesalexandre Jan 3, 2024

herbig commented Dec 22, 2023 •

edited

Loading

gomesalexandre commented Jan 3, 2024

0xean commented Jan 8, 2024

gomesalexandre commented Jan 8, 2024

fix: swap max for non native assets #5923

fix: swap max for non native assets #5923

Conversation

herbig commented Dec 21, 2023

Description

Pull Request Type

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xApotheosis left a comment

Choose a reason for hiding this comment

herbig commented Dec 21, 2023

woodenfurniture commented Dec 21, 2023

gomesalexandre 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herbig commented Dec 22, 2023 • edited Loading

gomesalexandre commented Jan 3, 2024

0xean commented Jan 8, 2024

gomesalexandre commented Jan 8, 2024

herbig commented Dec 22, 2023 •

edited

Loading