-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Use input as data in eth_sendTransaction #300
base: main
Are you sure you want to change the base?
Conversation
src/wallet.ts
Outdated
const txParamsData = data || input; | ||
const txParams: TransactionParams & { data?: string } = { | ||
...restParams, | ||
...(txParamsData ? { data: txParamsData } : undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For initial maximal compatibility, how about setting both input
and data
, removing one in a follow-up release?
After this change, input
is now a special case filtered out from output while any other free-name fields are still passed through. This is a bit counter-intuitive and might be good to do in a two-step process?
...(txParamsData ? { data: txParamsData } : undefined), | |
...( | |
txParamsData | |
? { data: txParamsData, input: txParamsData } | |
: undefined | |
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here b47e1ba
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure we want to set both? the original geth PR says:
bail if both are set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really modify the params at all? Considering there are other implementations which may work differently. Maybe the validation that both aren't provided with differing values is enough for the sake of this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only consumer of this should be TransactionController, which drops input
(and other unexpected fields) as one of the first steps of transaction processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public package with external dependents who rely on the current API, though, which also need to be considered.
https://www.npmjs.com/browse/depended/@metamask/eth-json-rpc-middleware
There are likely other dependents as well, which don't necessarily show up on npmjs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MetaMask is also actively advising existing users of deprecated web3-provider-engine
to migrate to @metamask/eth-json-rpc-middleware
.
The Ethereum execution API specs have
eth_sendTransaction
with ainput
field, however nodes such as geth have been using thedata
field instead, addinginput
support later.To make
eth_sendTransaction
align better with execution api, this PR makeseth_sendTransaction
acceptsinput
ORdata
(if both are provided, they must be equal) and maps it to thedata
field in the object that theprocessTransaction
callback expectsSee: https://github.com/MetaMask/MetaMask-planning/issues/2214