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

Tx: Small fixes and improvements #1144

Merged
merged 10 commits into from
Mar 10, 2021
Merged

Tx: Small fixes and improvements #1144

merged 10 commits into from
Mar 10, 2021

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Mar 10, 2021

This PR applies a few small fixes and improvements to the new tx library with EIP2718 and 2930 support:

  • AccessListEIP2930Transaction - removes asList parameter in raw(), preferring to use serialize()
  • TransactionFactory - for clarity, renames fromRawData to fromSerializedData
  • For clarity, renames _validateExceedsMaxInteger(validateCannotExceedMaxInteger) to _validateCannotExceedMaxInteger(values)
  • Renames internal usage of fromRlpSerializedTx to fromSerializedTx
  • Introduces common.copy() for easier deep copy functionality.

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1144 (405cadf) into master (53150c1) will increase coverage by 0.05%.
The diff coverage is 77.77%.

Impacted file tree graph

Flag Coverage Δ
block 81.88% <0.00%> (+0.27%) ⬆️
blockchain 84.19% <ø> (+0.06%) ⬆️
client 84.81% <100.00%> (-0.03%) ⬇️
common 86.90% <0.00%> (-0.11%) ⬇️
devp2p 84.53% <ø> (+0.25%) ⬆️
ethash 82.08% <ø> (ø)
tx 84.01% <80.00%> (-0.86%) ⬇️
vm 81.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This all looks great, thanks for the continued improvements and diving deep here. 😄 👍

this.transactions.map((tx) => <Buffer[]>tx.raw()),
this.transactions.map((tx) =>
'transactionType' in tx && tx.transactionType > 0 ? tx.serialize() : tx.raw()
) as Buffer[],
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this makes sense. I think this is the right thing to do to get the whole raw and serialized data setup consistent again and avoid this unlucky splitting of the tx.raw() method.

We just should be aware here that this introduces another tx type induced version dependency between the libraries, so block serialization will now be wrong when an old block version is used upon a tx-type-containing tx release (but so be it).

I guess we minimally need to communicate the set of library versions to be used when using typed txs very clearly if we want to stay in the minor release realm (as we've done with the clique changes, these were still a bit better encapsulated by the Common consensusType setting though). 🤔

//cc @cgewecke

@holgerd77 holgerd77 closed this Mar 10, 2021
@holgerd77 holgerd77 reopened this Mar 10, 2021
@holgerd77
Copy link
Member

Argh. Accidentally hit the "close" button and need to wait for the tests again. 😛

@holgerd77 holgerd77 merged commit e8bdbf6 into master Mar 10, 2021
@holgerd77 holgerd77 deleted the tx-followup-work branch March 10, 2021 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants