-
Notifications
You must be signed in to change notification settings - Fork 990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add send collectible fee validation (and small send collectible fixes) #22232
base: develop
Are you sure you want to change the base?
Conversation
(let [chosen-route (:best suggested-routes-data) | ||
{:keys [token collectible token-display-name | ||
receiver-network-values | ||
sender-network-values tx-type]} (get-in db [:wallet :ui :send]) | ||
token-decimals (if collectible 0 (:decimals token)) | ||
native-token? (and token (= token-display-name "ETH")) | ||
to-network-amounts-by-chain (send-utils/network-amounts-by-chain | ||
{:route chosen-route | ||
:token-decimals token-decimals | ||
:native-token? native-token? | ||
:receiver? true})] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only change in this event is moving these definitions above the enough-assets?
condition, so that we get the network information if there's no assets also.
Jenkins BuildsClick to see older builds (4)
|
71% of end-end tests have passed
Failed tests (4)Click to expandClass TestWalletCollectibles:
Class TestWalletOneDevice:
Passed tests (10)Click to expandClass TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
|
b1611b0
to
3c4f2e2
Compare
@clauxx thanks for the PR! Please take a look at the issues below. ISSUE 1 Fee validation is cleared after changing fee settingsPreconditions: User has Account A which holds some collectibles but 0 eth (no eth to cover fee) Steps:
Actual result: validation is reset. Slider is active and user can proceed with signing transaction. telegram-cloud-document-2-5366074879118437717.mp4 |
ISSUE 2 Est time is not shown on confirmation screenSteps:
Actual result: Est time is not shown. Previously (in current develop) we have shown Est time, but there was no value which is also not okay. So from one point removing
Expected result: I am not sure what is the expected result here and which Est time value should we show. Should this value come from backend using the same method we use for dapp transactions or here we should use some other method? Or should we use hardcoded value of Est time? When we select fee in transaction setting, the time value seems to be hardcoded there (see screenshot below). Should we user the same hardcoded values from transaction settings on confirmation screen or rely on some estimated time that is calculated on the backend? |
fixes #22213
Summary
a. show the "Not enough assets to pay gas fees" alert banner
b. highlight the fee
c. disable the slider
a. show the "No routes found ..." alert banner
b. disable the slider
a. removed activity loader
b. show transaction details banner (est. time, max fees, recipient gets) in loading state
c. show disabled slide button
a. instead of the exact minutes (e.g. 15 min), show the intervals (e.g. 1-2 min, 3-5 min, >5 min) like we do for dapp transactions
status: ready