-
Notifications
You must be signed in to change notification settings - Fork 986
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
Quick transaction settings #22019
base: develop
Are you sure you want to change the base?
Quick transaction settings #22019
Conversation
Jenkins BuildsClick to see older builds (55)
|
875e1c0
to
333f74f
Compare
22b4750
to
7a2b24c
Compare
@@ -9,35 +9,33 @@ | |||
|
|||
(defn view | |||
[{:keys [track-text customization-color auth-button-label on-auth-success on-auth-fail | |||
auth-button-icon-left size blur? container-style disabled? dependencies on-complete] | |||
auth-button-icon-left size blur? container-style disabled? dependencies] |
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.
I removed on-complete
because we used it to prevent the standard authorization slider from doing standard authorization, which is a bit illogical :)
So now this slider works only for standard authorization, for cases when you need to choose between keycard or standard auth there is a simple slider.
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.
can you explain why the changes to standard_auth were needed in this case? Asking cause I'm finishing up on the PR which also removes it, but also allows to sign e.g transaction data with standard_auth (including the keycard support). So I'm wondering what was blocking it to make sure it's also supported there?
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.
update: changes were discussed on a call with @clauxx and we aligned our development.
new-path) | ||
precision) | ||
:eip-1559-enabled true | ||
:tx-max-fees-per-gas (send-utils/convert-to-gwei |
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.
This is a new field from status-go
that holds currently used gas. It need to be used for fee calculation
new-path) | ||
precision) | ||
:suggested-gas-fees-for-setting | ||
{:transaction-setting/normal (send-utils/convert-to-gwei |
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.
These three now used to calculate possible fees on different quick settings levels.
:flow-id :wallet-send-flow}]]]})) | ||
|
||
(rf/reg-event-fx | ||
:wallet.send/auth-slider-completed |
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.
When slider moved by user we initiate sequence of building transaction and signing it
:type :negative | ||
:text (:message error)}]))}]}))) | ||
|
||
(rf/reg-event-fx |
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.
This is the key refactoring.
Before if user doesnt need keycard sign, we initiated standard authorization from slider component and than were signing from this event. But when user required keycard sign, we initiated keycard authorization from this event with consecutive sign.
Now both types of authorization run from here, that simplified caller code.
:current 31000} | ||
:nonce {:last-transaction 21 | ||
:current 22}}) | ||
(assoc-in [:wallet :ui :send :confirmed-tx-setting] :transaction-setting/fast))})) |
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.
Saved selected user setting
path-tx-identity (send-utils/path-identity route) | ||
params [path-tx-identity gas-rate]] | ||
{:db (assoc-in db [:wallet :ui :send :confirmed-tx-setting] confirmed-setting) | ||
:json-rpc/call [{:method "wallet_setFeeMode" |
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.
status-go
endpoint for selecting quick transaction setting
(i18n/label :t/slide-to-send)) | ||
:container-style {:z-index 2} | ||
:disabled? (not transaction-for-signing) | ||
[quo/slide-button |
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.
Replaced auth slider with simple slider.
currency-symbol | ||
fee-in-fiat)] | ||
fee-formatted)))) | ||
(fn [[account route currency currency-symbol prices-per-token]] |
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.
Removed argument from this sub, because we can extract it from db
3d8d193
to
5462f90
Compare
b750f3e
to
c8727a3
Compare
@vkjr please, let us know when it is ready to testing, we need to take it ASAP as the feature is important for release and high-risk. |
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.
Looks good, but there is one usage of the standard-auth slider with the on-complete
prop that wasn't adapted - the create-account
namespace, which I assume would be broken for keycard here.
fixes #21019
Summary
Quick transaction settings added. Now you can select normal/fast/urgent mode and see the estimated gas fee.
Screen.Recording.2025-02-07.at.11.39.38.mov
Review notes
Adding quick transaction settings required changes in events flow. Before this PR our app was calling status-go function
BuildTransactionsFromRoute
at the moment when we navigated to transaction confirmation screen. This function stops the route updates, so the only thing we had to do is to sign the transactions.But in order to allow user to change transaction settings we need to send changes to status-go and keep getting route updates with new fees.
So the events order was changed. Now we navigate to transaction confirmation screen and still getting the routes update. But when user slides the sign button we:
BuildTransactionsFromRoute
wallet.router.sign-transactions
signalAlong the way the authrization slider was refactored. Update: authorization slider changes were reverted back to not interfere with current @clauxx refactoring of authorization process
Testing notes
This PR has changed the order of events during send/bridge, so they should be tested.
Platforms
Areas that may be impacted
Functional
Steps to test
status: ready