Skip to content

Enhance transaction validation with standardized RPC errors - #1690

Merged
vinistevam merged 14 commits into
mainfrom
refactor/973-throw-rpc-error-validation
Sep 27, 2023
Merged

Enhance transaction validation with standardized RPC errors#1690
vinistevam merged 14 commits into
mainfrom
refactor/973-throw-rpc-error-validation

Conversation

@vinistevam

@vinistevamvinistevam commented Sep 20, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Explanation

This PR aims to update the TransactionController to throw standardised rpcErrors.invalidParams errors using the @metamask/rpc-errors package that will ensure they are standardised and more easily parsed and recognised by the dApps.

Context
The extension TransactionController uses the eth-rpc-errors package to throw standardised errors when validation fails. This update is a part of the ongoing TransactionController unification effort.

Note: As the colleagues pointed out in this PR eth-rpc-errors is deprecated so for equivalence the @metamask/rpc-errors package was added in the TransactionController and usages in the code was replaced accordingly.

Bump @metamask/rpc-errors to v6 in transaction-controller and assets-controller.

References

Changelog

@metamask/transaction-controller

  • CHANGED: update validateTxParams to throw standardised errors using the @metamask/rpc-errors package.
    • add @metamask/rpc-errors package.
    • remove eth-rpc-errors and replace usage with the new package.

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

@vinistevamvinistevam changed the title Refactor: Throw standardized RPC errors using eth-rpc-errors when validation fails adding transactionThrow standardized RPC errors using eth-rpc-errors when validation fails adding transactionSep 20, 2023
@vinistevamvinistevam changed the title Throw standardized RPC errors using eth-rpc-errors when validation fails adding transactionEnhance transaction validation with standardized RPC errors using eth-rpc-errorsSep 20, 2023
@vinistevam
vinistevam marked this pull request as ready for review September 20, 2023 10:19
@vinistevam
vinistevam requested a review from a team as a code ownerSeptember 20, 2023 10:19
OGPoyraz
OGPoyraz previously approved these changes Sep 20, 2023
View reviewed changes
@legobeat

legobeat commented Sep 20, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

eth-rpc-errors is deprecated - would it be possible to standardize directly on updated and maintained @metamask/rpc-errors?

There is an open draft PR for @metamask/rpc-errors update here:

@vinistevamvinistevam mentioned this pull request Sep 21, 2023
Merged
3 tasks
@matthewwalsh0

matthewwalsh0 commented Sep 21, 2023

Copy link
Copy Markdown
Member

eth-rpc-errors is deprecated - would it be possible to standardize directly on updated and maintained @metamask/rpc-errors?

There is an open draft PR for @metamask/rpc-errors update here:

This makes sense to me assuming it's functionally identical and the resulting exception are the same?

The goal here is to align with the extension so if this diverges further, it may be best to update the package down the line.

@legobeat

legobeat commented Sep 21, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@matthewwalsh0 Yes, the direction is that all active use of eth-rpc-errors must be actively removed/replaced with @metamask/rpc-errors including in MetaMask libraries, extension, and mobile. v4 and v6 are not completely identical.

The changelog is here: https://github.com/MetaMask/rpc-errors/blob/main/CHANGELOG.md

See esp v5.0.0 for API changes in the package.

The major thing here is the version upgrade - it's an associated package rename, not a change of dependencies per se.

Mrtenz
Mrtenz previously requested changes Sep 22, 2023
View reviewed changes

@MrtenzMrtenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use @metamask/rpc-errors.

@vinistevamvinistevam changed the title Enhance transaction validation with standardized RPC errors using eth-rpc-errorsEnhance transaction validation with standardized RPC errorsSep 22, 2023
@vinistevam

vinistevam commented Sep 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Please use @metamask/rpc-errors.

Thanks for the heads up @legobeat and @Mrtenz 🙏 I pushed the changes to use @metamask/rpc-errors instead of eth-rpc-errors

legobeat
legobeat previously requested changes Sep 22, 2023
View reviewed changes

@legobeatlegobeat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

v6 has compatibility fixes in types

Comment threadpackages/transaction-controller/package.json Outdated
@vinistevam
vinistevamforce-pushed the refactor/973-throw-rpc-error-validation branch from 1c098a1 to 919d77bCompareSeptember 25, 2023 08:46
@socket-security

socket-securityBot commented Sep 25, 2023

Copy link
Copy Markdown

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages:@metamask/rpc-errors@5.1.1

@legobeat
legobeat dismissed their stale reviewSeptember 26, 2023 09:52

this now uses @metamask/rpc-errors@6.0.0

@MrtenzMrtenz mentioned this pull request Sep 26, 2023
Closed
3 tasks
FrederikBolding
FrederikBolding previously approved these changes Sep 26, 2023
View reviewed changes
legobeat
legobeat approved these changes Sep 26, 2023
View reviewed changes

@legobeatlegobeat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

This was referenced Sep 27, 2023
Closed
Merged
@vinistevam
vinistevam merged commit 762882d into mainSep 27, 2023
@vinistevam
vinistevam deleted the refactor/973-throw-rpc-error-validation branch September 27, 2023 07:08
@legobeatlegobeat mentioned this pull request Sep 29, 2023
Merged
3 tasks
legobeat added a commit that referenced this pull request Oct 2, 2023
@legobeat
deps(approval-controller): move to @metamask/rpc-errors (#1743)
e14ca00
## Explanation
This replaces obsolete `eth-rpc-errors` with `@metamask/rpc-errors` in
`@metamask/approval-controller`. This should be coupled with #1639 and
can be merged before or after.
## References
#### Broken out from
- #1731 #### Blocking
- #1724 #### Related
- #1690
## Changelog
### `@metamask/approval-controller`
- **Changed**: Replaced `eth-rpc-errors` with `@metamask/rpc-errors`
### `@metamask/controller-utils`
- **Fixed**: Removed unused dependency `eth-rpc-errors`
## Checklist
- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FrederikBoldingFrederikBoldingFrederikBolding left review comments
@MrtenzMrtenzMrtenz left review comments
@OGPoyrazOGPoyrazOGPoyraz left review comments
+1 more reviewer
@legobeatlegobeatlegobeat approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vinistevam@legobeat@matthewwalsh0@FrederikBolding@Mrtenz@OGPoyraz