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.

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

Move eth_sendTransaction method handler to a separate module #5968

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Mar 15, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

The method handler for eth_sendTransaction has been moved to a separate module to simplify unit testing. Unit tests have been added as well.

This module was written in TypeScript, which asked for some additional validation of the method parameters. We now throw a more explicit message when the params are missing, and when the transaction parameters are not an object. Previously the method would still throw an error in these scenarios, just with a less helpful message. The change in error message should be the only functional change here.

Issue

This refactor is intended to make testing PR #5619 easier.

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

package.json Show resolved Hide resolved
@Gudahtt Gudahtt force-pushed the refactor-send-transaction branch from 16fae8d to fed001a Compare March 15, 2023 19:51
The method handler for `eth_sendTransaction` has been moved to a
separate module to simplify unit testing. Unit tests have been added
as well.

This module was written in TypeScript, which asked for some additional
validation of the method parameters. We now throw a more explicit
message when the params are missing, and when the transaction
parameters are not an object. Previously the method would still throw
an error in these scenarios, just with a less helpful message. The
change in error message should be the only functional change here.

This refactor is intended to make testing PR #5619 easier.
@Gudahtt Gudahtt force-pushed the refactor-send-transaction branch from fed001a to 0c17e7c Compare March 15, 2023 20:42
The Metro configuration has been updated to support `.cjs` file
extensions. The `@metamask/utils` dependency relies upon the library
`superstruct` which uses `.cjs` for its main exported module.
@Gudahtt Gudahtt marked this pull request as ready for review March 15, 2023 22:21
@Gudahtt Gudahtt requested a review from a team as a code owner March 15, 2023 22:21
@Gudahtt Gudahtt added No QA Needed Apply this label when your PR does not need any QA effort. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 15, 2023
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Really clean implementation. LGTM

@Gudahtt Gudahtt merged commit 49a6305 into main Mar 15, 2023
@Gudahtt Gudahtt deleted the refactor-send-transaction branch March 15, 2023 23:45
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2023
@gantunesr gantunesr added release-6.3.0 Issue or pull request that will be included in release 6.3.0 and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-6.3.0 Issue or pull request that will be included in release 6.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants