-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Transaction input
and data
refactors
#6294
Conversation
…, don't autofill unless both or nullish
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6294 +/- ##
==========================================
- Coverage 88.89% 88.86% -0.03%
==========================================
Files 199 199
Lines 7706 7707 +1
Branches 2115 2120 +5
==========================================
- Hits 6850 6849 -1
- Misses 856 858 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Deploying with Cloudflare Pages
|
@@ -42,7 +42,7 @@ const getEthereumjsTxDataFromTransaction = ( | |||
gasLimit: transaction.gasLimit ?? transaction.gas, | |||
to: transaction.to, | |||
value: transaction.value, | |||
data: transaction.input, | |||
data: transaction.data ?? transaction.input, |
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 property name is still data
because that's what TransactionFactory.fromTxData is expecting
input?: Bytes; | ||
data?: Bytes; |
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 breaking change for eth_types
, but input
won't be apart of this object if the user uses data
LGTM needs a changelog update |
…ctionSchema optional with transactoinInfoSchema the default value
…paring equality for TransactionDataAndInputError check
transactionSchema, | ||
options: { | ||
transactionSchema?: ValidationSchemaInput | typeof transactionSchema; | ||
fillInputAndData?: boolean; |
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.
A new parameter, fillInputAndData
, was added to the options
argument for formatTransaction
. This method is used for formatting both user submitted and RPC response transaction objects. So, we need a flag to determine whether to leave the transaction object as given, or fill in the input
or data
properties if they weren't present. The fillInputAndData
option is set in rpc_method_wrappers.ts
for relevant methods
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, and things similar, needs to be in the documentation.
@Muhammad-Altabba Need your re-review |
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.
Good job!
All good now I think.
However, the documentation needs to be updated accordingly.
transactionSchema, | ||
options: { | ||
transactionSchema?: ValidationSchemaInput | typeof transactionSchema; | ||
fillInputAndData?: boolean; |
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, and things similar, needs to be in the documentation.
closes #6183