-
Notifications
You must be signed in to change notification settings - Fork 9
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
[Bug fix] Prevent same Transaction from being commited twice. #539
Conversation
public struct ViewState: Equatable { | ||
// TODO: declare some properties | ||
} | ||
public struct ViewState: Equatable {} |
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.
You can just remove it
showApproveButton: transactionWithLockFee != nil, | ||
canApproveTX: canApproveTX |
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.
hm, why do we need both?
@@ -119,6 +121,7 @@ extension TransactionReview { | |||
Button(L10n.TransactionReview.approveButtonTitle, asset: AssetResource.lock) { | |||
viewStore.send(.approveTapped) | |||
} | |||
.controlState(viewStore.canApproveTX ? .enabled : .disabled) |
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.
Do we need both "Button visible but disabled", and "Button invisible"?
@@ -30,18 +31,21 @@ public struct TransactionReview: Sendable, FeatureReducer { | |||
public var proofs: TransactionReviewProofs.State? = nil | |||
public var networkFee: TransactionReviewNetworkFee.State? = nil | |||
public let ephemeralNotaryPrivateKey: Curve25519.Signing.PrivateKey | |||
public var canApproveTX: Bool = 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.
should it start out 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.
Not visible in beginning as you pointed out. But yeah ought to clean up later
@@ -198,6 +202,7 @@ public struct TransactionReview: Sendable, FeatureReducer { | |||
|
|||
case .approveTapped: | |||
guard let transactionWithLockFee = state.transactionWithLockFee else { return .none } | |||
state.canApproveTX = false |
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.
aha, so this is more of a "already approving TX"? Then perhaps it should be an enum with informatively named cases?
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.
sure, I can change to status
and some enum
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.
ah actually, can we do it later? I want to change all this code anyway...?
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 am ok with doing this later
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.
sure, just wanted to make sure i understood the logic
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.
Great to have this, but it seems to me that canApproveTX should be more like enum TXStatus instead?
Jira ticket: paste link here
This is a small yet important security related fix, which prevents users from ever approving, signing and submitting the same transaction twice.
The fix is small, and 100% safe. What we do is that for a given TransactionManifest to review, sign and submit, we generate the transaction
nonce
in the same context where we have the TransactionManifest, and pass both toTransactionReview
feature, this is fundamentally different to what we are doing today, which is generate a new nonce everytime Approve TX button is pressed (bad and dangerous and should never have been implemented this way!).For "belt and suspenders", I also persist the Approve Button state in TransactionReview, making sure it cannot be pressed again until tx has failed.
PR submission checklist