Skip to content

Cancel transaction when swaps submission is failed because the simulation fails#10288

Merged
danjm merged 2 commits intodevelopfrom
fix-swap-simulationFails-autofail
Jan 26, 2021
Merged

Cancel transaction when swaps submission is failed because the simulation fails#10288
danjm merged 2 commits intodevelopfrom
fix-swap-simulationFails-autofail

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented Jan 26, 2021

This PR fixes a bug with the code added here https://github.com/MetaMask/metamask-extension/pull/9947/files#diff-79e282e22d97801a4a8ac7638c098e38eb40e3c46ad24ff4f7db7405dac2faabR719

That code prevents a swap transaction for which the gas estimation fails from being signed and published to the blockchain. However, there is a bug in the code: the swaps transaction is not cancelled. This leaves an unapproved transaction in state, which the user would see after exiting swaps and attempting to return to the home page. The user could then still submit the swap, which is what we are trying to prevent.

The solution is to cancel the swap in addition to taking the user to the error screen.

@danjm danjm requested a review from a team as a code owner January 26, 2021 13:42
@danjm danjm requested a review from Gudahtt January 26, 2021 13:42
@github-actions
Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [35b7a44]
Page Load Metrics (697 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint499368115
domContentLoaded45294069513565
load45394169713465
domInteractive45293969513565

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jan 26, 2021

Is it possible that this might work on the second attempt? e.g. if the estimate gas call succeeded the second time? I thought this was how the linked PR was intended to work originally.

@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Jan 26, 2021

Is it possible that this might work on the second attempt? e.g. if the estimate gas call succeeded the second time? I thought this was how the linked PR was intended to work originally.

@Gudahtt I'm not exactly sure what you mean here, but the intended purpose is to simply cancel a transaction which has an estimateGas call fail at the time the user attempts to confirm a swap. There would be no "second time" as the transaction has been cancelled. The user would be creating a new transaction if they were to try again.

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jan 26, 2021

I was referring to this:

This leaves an unapproved transaction in state, which the user would see after exiting swaps and attempting to return to the home page. The user could then still submit the swap, which is what we are trying to prevent.

The described "bug", I was questioning whether it was really a bug or not.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [83d61f4]
Page Load Metrics (765 ± 131 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint42146723215
domContentLoaded3431401764271130
load3451407765272131
domInteractive3431401764271130

@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Jan 26, 2021

@Gudahtt Thanks for the clarification. I see what you mean now.

So, yes, it is possible that it could work on the second attempt. But this is risky and creates a number of extra pieces of work.

When the user exits swap in the scenario where we don't cancel the transaction, then they are shown the standard confirm screen and the transaction is labelled a "contract interaction". This screen also does not prevent submission if simulationFails is truthy, it just warns the user about it. So users will be in a situation where they probably don't know what that transaction is, and even if they do, we provide no protection against submission in the case where the second attempt fails.

So we would need to add new UX for how the user goes from the first attempt to the second attempt. Given that this whole scenario is a somewhat rare case, I think it is okay for now that they just go through the whole swaps flow again

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jan 26, 2021

Ah I see, I hadn't realized it fully escaped the swaps UI in this case. That sounds bad.

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@danjm danjm merged commit d0df03b into develop Jan 26, 2021
@danjm danjm deleted the fix-swap-simulationFails-autofail branch January 26, 2021 20:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants