Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bug]: When Smart Transactions enabled, transaction in 'Unapproved' state can be edited #25356

Closed
sleepytanya opened this issue Jun 15, 2024 · 3 comments · Fixed by #25510
Closed

[Bug]: When Smart Transactions enabled, transaction in 'Unapproved' state can be edited #25356

sleepytanya opened this issue Jun 15, 2024 · 3 comments · Fixed by #25510
Labels
regression-RC-12.0.0 release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-transactions Transactions team type-bug

Comments

@sleepytanya
Copy link
Contributor

sleepytanya commented Jun 15, 2024 •
edited
Loading

Describe the bug

When Smart Transactions enabled, transaction in 'Unapproved' state can be edited.

  • User is allowed to reject the transaction but it doesn't work.
  • User is allowed to edit the transaction and the confirmation screen that is shown is the one that was removed form the send+swap flow and should not be displayed to the user (the last confirmation screen with the gas settings and estimations).
  • Origin and destination token can be edited (with different outcomes, where selecting different destination token results in a new transaction)
  • User is allowed to edit the recipient address which results in MetaMask stuck on infinite spinner and console error.

Expected behavior

Screenshots/Recordings

Screenshot 2024-06-15 at 17 02 53
editUnapproved.mov

In this video random address appears in the recipient field, screen automatically switches from confirmation to the address selection with recipient's accounts button inactive, after clicking 'Continue' MetaMask freezes (Firefox):

random.RecipientFreeze.mov

Chrome, random screen switch from confirmation to address selection:

screenSwitch.mov
Screenshot 2024-06-16 at 02 54 17

Steps to reproduce

  1. Start send+swap transaction
  2. Edit 'Unapproved' transaction

Error messages or log output

No response

Version

12.0.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response
@sleepytanya sleepytanya mentioned this issue Jun 15, 2024
Closed
@sleepytanya sleepytanya added the team-transactions Transactions team label Jun 16, 2024
@gauthierpetetin gauthierpetetin added the Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. label Jun 20, 2024
@forest-diggs-consensys
Copy link

Should retest with latest build (STX disabled for Swap & Send transactions)

@sleepytanya
Copy link
Contributor Author

sleepytanya commented Jun 20, 2024 •
edited
Loading

Latest build [eebc4d5]
The unapproved transaction still can be edited. STX is not disabled for send+swap transactions in this scenario, no errors in the console. Send+swap txs seems to be disabled if started by clicking 'Send'.

3.mov

@danjm danjm added the release-blocker This bug is blocking the next release label Jun 24, 2024
@BZahory BZahory mentioned this issue Jun 25, 2024
Merged
7 tasks
BZahory added a commit that referenced this issue Jun 25, 2024
@BZahory
fix: do not use STX for swap+send approval (#25510)
30c44cc 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Paired w/ @dan437 and @forest-diggs-consensys 

We removed STX functionality for Swap+Send in #25422. However, we still
get the STX flow because the `swapApproval` type we use is still routed
as an STX.

Since only Swap+Send uses the `swapApproval` type for STXs routed
through `submitSmartTransactionHook`, we can exclude it in the same way
we excluded the `swapAndSend` type in #25422.

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25510?quickstart=1)

## **Related issues**

Fixes: #25356

## **Manual testing steps**

1. Enable Smart Transactions in settings
2. Switch to Ethereum Mainnet
3. Submit a swap+send transaction that requires an approval (i.e. from
an ERC-20 w/o an existing approval)
4. Ensure that the STX screen does not show after submitting
5. Ensure that the contract interaction isn't marked as "unapproved"

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**


https://github.com/MetaMask/metamask-extension/assets/44588480/d3fb13ca-4aa0-43a8-8689-37f7e49af10a

<!-- [screenshots/recordings] -->

### **After**


https://github.com/MetaMask/metamask-extension/assets/44588480/d34ab118-ed69-4418-8c35-06774099e092

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 25, 2024
@benjisclowder
Copy link
Contributor

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

1. What PR fixed the issue?
2. Can you pinpoint the commit from which the issue originated?
3. Write a short explanation of the technical cause of the bug
4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @BZahory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
regression-RC-12.0.0 release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-blocker This bug is blocking the next release Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-transactions Transactions team type-bug
Projects
Bugs by severity
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: do not use STX for swap+send approval MetaMask/metamask-extension
6 participants
@gauthierpetetin @danjm @metamaskbot @sleepytanya @forest-diggs-consensys @benjisclowder