feat: calculate and show max fees on transaction confirmation page #20133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (48)
|
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
6 times, most recently
from
1c9ed29
to
d71fa5f
Compare
briansztamfater
commented
May 21, 2024
Comment on lines
217
to
243
| fee-in-native-token (when-not confirm-disabled? | ||
| (send-utils/calculate-full-route-gas-fee route)) | ||
| fee-in-crypto-formatted (when fee-in-native-token | ||
| (utils/get-standard-crypto-format | ||
| native-token | ||
| fee-in-native-token)) | ||
| fee-in-fiat (when-not confirm-disabled? | ||
| (utils/calculate-token-fiat-value | ||
| {:currency fiat-currency | ||
| :balance fee-in-native-token | ||
| :token native-token})) | ||
| fee-formatted (when fee-in-fiat | ||
| (utils/get-standard-fiat-format | ||
| fee-in-crypto-formatted | ||
| currency-symbol | ||
| fee-in-fiat)) |
There was a problem hiding this comment.
Moved logic to a sub
briansztamfater
commented
May 21, 2024
Comment on lines
+242
to
+250
| [transaction-details | ||
| {:estimated-time-min estimated-time-min | ||
| :max-fees fee-formatted | ||
| :token-display-name token-display-name | ||
| :amount amount | ||
| :to-network bridge-to-network | ||
| :theme theme | ||
| :route route | ||
| :transaction-type transaction-type}] |
There was a problem hiding this comment.
Designs changed, now we should display transaction details right above the slider
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
2 times, most recently
from
21b6bf3
to
653288f
Compare
briansztamfater
requested review from
vkjr,
J-Son89,
OmarBasem,
BalogunofAfrica,
mmilad75 and
ulisesmac
|
🥳 |
J-Son89
reviewed
May 21, 2024
| @@ -222,12 +209,17 @@ | |||
| (get-in collectible [:preview-url :uri])) | |||
| transaction-type (:tx-type send-transaction-data) | |||
| estimated-time-min (reduce + (map :estimated-time route)) | |||
| max-fees "-" | |||
OmarBasem
approved these changes
May 22, 2024
Comment on lines
+474
to
+597
|
|
||
| (rf/reg-sub | ||
| :wallet/wallet-send-fee-fiat-formatted | ||
| :<- [:wallet/wallet-send-route] | ||
| :<- [:profile/currency] | ||
| :<- [:profile/currency-symbol] | ||
| (fn [[route currency currency-symbol] [_ token-for-fees]] | ||
| (let [fee-in-native-token (send-utils/calculate-full-route-gas-fee route) | ||
| fee-in-crypto-formatted (utils/get-standard-crypto-format | ||
| token-for-fees | ||
| fee-in-native-token) | ||
| fee-in-fiat (utils/calculate-token-fiat-value | ||
| {:currency currency | ||
| :balance fee-in-native-token | ||
| :token token-for-fees}) | ||
| fee-formatted (utils/get-standard-fiat-format | ||
| fee-in-crypto-formatted | ||
| currency-symbol | ||
| fee-in-fiat)] | ||
| fee-formatted))) |
There was a problem hiding this comment.
I think it's better to return them as a map and include fee-in-native-token, fee-in-crypto-formatted, and so on.
There was a problem hiding this comment.
@mmilad75 We can create separate subs for each required value. For instance, if we need fee-in-native-token, we can create a new sub :wallet/wallet-send-fee-in-native-token. Then, we can refactor the existing sub to use this new sub as an input signal with the :<- macro.
| :padding-bottom 16}) | ||
| [{:keys [loading-suggested-routes? route-loaded?]}] | ||
| {:flex-direction :row | ||
| :width "100%" |
There was a problem hiding this comment.
Would :flex 1 or :flex-grow 1 work?
There was a problem hiding this comment.
I tried :flex 1, but for some reason the height is ignored. :width "100%" worked fine instead
81% of end-end tests have passedFailed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
|
BalogunofAfrica
approved these changes
May 22, 2024
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
3 times, most recently
from
b4939df
to
db8b039
Compare
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
3 times, most recently
from
d494e16
to
e523927
Compare
mariia-skrypnyk
force-pushed
the
feat/transaction-confirmation-fees
branch
from
e523927
to
a1ae06f
Compare
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
from
a1ae06f
to
bebc7c6
Compare
88% of end-end tests have passedFailed tests (2)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (46)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
Hey @briansztamfater ! Thanks for your PR. |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/transaction-confirmation-fees
branch
from
bebc7c6
to
be6fca6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18408
Summary
This PR adds fee information on transaction confirmation page. Also it updates the UI to comply with latest designs of this screen, which shows the fees, estimated time and amount at the bottom of the screen, above the slider.
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready