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]: Transaction with invalid/unsupported type cannot be cleared from the wallet and remains as Unapproved #23180

Closed
seaona opened this issue Feb 26, 2024 · 4 comments
Closed

[Bug]: Transaction with invalid/unsupported type cannot be cleared from the wallet and remains as Unapproved #23180

seaona opened this issue Feb 26, 2024 · 4 comments
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. team-transactions Transactions team type-bug

Comments

@seaona
Copy link
Contributor

seaona commented Feb 26, 2024 •
edited
Loading

Describe the bug

Problem: whenever I trigger a not-supported or invalid transaction type, with gasLimit and gasPrice params, if I accept the transaction MM stays loading, but the transaction stays in an Unapproved state. If I try to reject it, nothing happens and tx remains Unapproved and cannot clear it from the wallet

Expected behavior

I think that, if the tx type is invalid or not supported, we shouldn't allow to proceed with the MM popup?
cc @bschorchit @matthewwalsh0

Screenshots/Recordings

unapproved-tx-not-cleared.mp4

Steps to reproduce

  1. Go to the test dapp (locally)
  2. Change the Send Eth (legacy) tx type to type: '0x3'
  3. Trigger that tx on the test dapp
  4. Accept it --MM stays loading
  5. Go to the full view of the wallet
  6. See tx stays as Unapproved
  7. Try to reject it
  8. See tx stays as Unapproved

Error messages or log output

No response

Version

11.10 but it's an pre-existing issue

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response
@seaona seaona added type-bug team-confirmations-system DEPRECATED: please use "team-confirmations" label instead labels Feb 26, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Feb 26, 2024
@seaona seaona mentioned this issue Mar 6, 2024
Merged
@bschorchit
Copy link

I think that, if the tx type is invalid or not supported, we shouldn't allow to proceed with the MM popup?

I agree

@bschorchit bschorchit added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Apr 7, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Apr 7, 2024
@cryptotavares cryptotavares added team-confirmations Push issues to confirmations team team-transactions Transactions team and removed team-confirmations-system DEPRECATED: please use "team-confirmations" label instead team-confirmations Push issues to confirmations team labels Apr 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Jul 28, 2024
@anaamolnar
Copy link

Should I remove the stale label from this? @bschorchit

@github-actions github-actions bot removed the stale issues and PRs marked as stale label Jul 31, 2024
@dbrans dbrans mentioned this issue Aug 12, 2024
Merged
3 tasks
dbrans added a commit to MetaMask/core that referenced this issue Aug 12, 2024
@dbrans
fix(TXL-207): fixes gaps in transaction validation and async error lo…
7796d35 
…gging (#4596)

## Explanation
This PR repairs some gaps in our validation of transaction parameters:
* validate that if a transaction envelope type is specified that it is a
valid type
* validate transaction parameters before right before adding transaction
to state
* catch and log async errors

Notes
* The steps to reproduce in
MetaMask/metamask-extension#23180 revealed an
async error that was being swallowed.

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

## References
This PR will fix these bugs once clients are updated:
* Related: MetaMask/MetaMask-planning#2341 
* Related: MetaMask/metamask-extension#23180

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/transaction-controller`

- **FIXED**: Fixes gaps in transaction parameter validation and async
error logging

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
@bschorchit
Copy link

Closing as a fix has been merged: MetaMask/core#4596

@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Aug 22, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. team-transactions Transactions team type-bug
Projects
Bugs by severity
Archived in project
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cryptotavares @bschorchit @seaona @anaamolnar