[#22036] fix: max fee issue when swapping from ETH #22048
Conversation
mohsen-ghafouri
requested review from
briansztamfater,
vkjr and
shivekkhurana
and removed request for
briansztamfater
Jenkins BuildsClick to see older builds (80)
|
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
59e339a to
00dc6ca
Compare
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
5 times, most recently
from
eb5a3ac to
7e25193
Compare
| @@ -309,3 +309,40 @@ | |||
| (-> collectible | |||
| transforms/json->clj | |||
| transform-collectible)) | |||
|
|
|||
| (defn- bridge-amount-greater-than-bonder-fees? | |||
There was a problem hiding this comment.
Just moved them from send.event to data-store file so i can used in swap as well.
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
7e25193 to
227c618
Compare
| @@ -583,3 +584,82 @@ | |||
| [:dispatch | |||
| [:navigate-to-within-stack | |||
| [:screen/wallet.swap-select-asset-to-pay :screen/wallet.swap-select-account]]]])}))) | |||
|
|
|||
| (rf/reg-event-fx :wallet/get-swap-proposal-fee | |||
There was a problem hiding this comment.
This is a simplified version of :wallet/start-get-swap-proposal, designed to fetch only the relevant fee fields early using a synchronous RPC call.
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
4 times, most recently
from
159e225 to
82891db
Compare
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
82891db to
e98d146
Compare
|
@VolodLytvynenko as i checked issue 3, the design related to that flow is https://www.figma.com/design/AD2JSKg0I8dZcylyiGa62O/Swap-for-Mobile?node-id=76-132707&t=6IkldMKUJdS0jXw5-4, so we should red highlight when user try to enter an amount above available amount (total balance - fee) 
|
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
1665301 to
5529929
Compare
|
I applied changes about Max Button label (it shows exact amount now), also i couldn't reproduce the issue 3, can be related to when USDC is not available temporary. I couldn't find any clue how it happens. from everywhere that i tested, it shows USDC for ETH. Please let me know if any other issue remain for this PR. thanks @VolodLytvynenko Simulator.Screen.Recording.-.iPhone.13.-.2025-02-25.at.17.19.03.mp4 |
@mohsen-ghafouri the screen you share related to another case.
but, this is a low-priority issue and can be considered as a follow-up. |
|
@mohsen-ghafouri one more issue is found. Take a look please #22191 |
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
5529929 to
cea972e
Compare
I believe this was implemented before the new flow. For other assets, we don’t know the fee upfront, so the fee error only shows after the user fills in the amount and receives a quote. But for ETH, since we have the fee available, we can consider it as the user types the amount—if it exceeds the available balance minus the fee, we display a red highlight to indicate that they should reduce the amount. |
|
@VolodLytvynenko issue #22191 and Issue #22188 resolved. could you please check again. |
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
2 times, most recently
from
1e341b7 to
047e3d3
Compare
71% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestWalletCollectibles:
Passed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletCollectibles:
|
| @@ -29,6 +29,15 @@ | |||
| :account account | |||
| :test-networks-enabled? test-networks-enabled? | |||
| :token-symbol (get-in data [:asset-to-pay :symbol])})) | |||
| received-asset (if-not (nil? asset-to-receive) | |||
There was a problem hiding this comment.
currently default assets for new account are "ETH", "DAI" and "SNT", I added this part to fallback to "SNT" if there is no "USDC" in user's account, we can remove this part after we implement #22160
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
2122cd2 to
e76d550
Compare
79% of end-end tests have passedFailed tests (3)Click to expandClass TestWalletCollectibles:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (11)Click to expandClass TestWalletCollectibles:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
|
|
Hey @vkjr @briansztamfater @clauxx @smohamedjavid @alwx, I would like to have more eyes on this PR if you have time please. |
| (defn button-loader | ||
| [theme] | ||
| {:width 100 | ||
| :height 22 | ||
| :border-radius 6 | ||
| :background-color (loader-color theme)}) |
There was a problem hiding this comment.
i noticed in the last video that the pay input size changes slightly when the balance button is loaded. Could be due to the height of the loader.
| from-locked-amount {} | ||
| send-type constants/send-type-swap | ||
| request-uuid (str (random-uuid)) | ||
| params [(cond-> |
There was a problem hiding this comment.
This is quite a duplication of functionality with :wallet/start-get-swap-proposal and it also duplicates all the "not-so-good-patterns". For example, to-address and from-address are useless variables that can be substituted with wallet-address. disabled-to-chain-ids and disabled-from-chain-ids are identical, and so on.
Instead of duplicating it we can create a separate function that will prepare parameters for get-suggested-routes. Something like params-for-swap-routes-request That function can get db as an argument and encapsulate all the commonalities between :wallet/get-swap-proposal-fee and :wallet/start-get-swap-proposal. All the differences can go in additional args. Also as a bonus it will hide the list of status-go parameters from event.
Wdyt?
There was a problem hiding this comment.
+1 on this. There's too much duplication with the router events.
There was a problem hiding this comment.
Sure let me see if i can transfer some logic to utils or in specific sub,
There was a problem hiding this comment.
I believe a function in the same file that has common logic between :wallet/start-get-swap-proposal and yours :wallet/get-swap-proposal-fee would be sufficient
|
Hi @mohsen-ghafouri . Could you please resolve any conflicts and rebase the current PR? Thanx! |
|
Hey @VolodLytvynenko, sure. it requires some adjustment regarding latest feedback, will let you know once it's being ready |
… button for low amount
mohsen-ghafouri
force-pushed
the
fix/max-fee-button
branch
from
e76d550 to
fd0c792
Compare
fixes #22036
Summary
The Max button fills the send field with all available ETH. This leaves no ETH to pay gas, leading to an error.
To fix this, we need to initially fetches the proposal using the maximum safe amount to determine the estimated gas fees and it display Max Button with corrected value.
As discussed:
Note
To display correct max balance for ETH we should have max fee, and fee comes with swap proposal. in initial state without amount we cannot have fee (As far as I tested, it give us error to get fee without asset amount, let me know if you have any other suggestion), so I just hide the max button till user enter some amount. we have to hide Max button when ever we are fetching gas fee to avoid display wrong amount.
Areas that may be impacted
Steps to test
Result
Simulator.Screen.Recording.-.iPhone.13.-.2025-02-20.at.23.06.58.mp4
status: ready
The max value does not show the exact amount #22186
Validation incorrectly highlighted by red when ETH balance is insufficient for gas covering
unknown asset is shown in the "assets to receive" field when opening swap via long tap
Max value fills entire ETH balance, causing "not enough assets to pay gas fee" error