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

Berlin Releases #1151

Merged
merged 13 commits into from
Mar 22, 2021
Merged

Berlin Releases #1151

merged 13 commits into from
Mar 22, 2021

Conversation

holgerd77
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #1151 (5c90450) into master (b2d171c) will decrease coverage by 0.13%.
The diff coverage is 25.00%.

Impacted file tree graph

Flag Coverage Δ
block 81.82% <ø> (ø)
blockchain 84.19% <ø> (ø)
client 83.63% <ø> (-0.41%) ⬇️
common 87.39% <ø> (ø)
devp2p 84.08% <ø> (ø)
ethash 82.08% <ø> (ø)
tx 84.25% <ø> (ø)
vm 81.67% <25.00%> (ø)

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

@holgerd77 holgerd77 force-pushed the berlin-releases branch 2 times, most recently from da3a642 to 3e293cb Compare March 16, 2021 23:23
@holgerd77
Copy link
Member Author

91 files changed and counting. Ugh. This is getting really dense.

@holgerd77
Copy link
Member Author

Ok, this is now ready for review. 😄

(which doesn't mean that we could not take 1-3 other changes and/or fixes in, it would just be good if this would be reviewed early on so that this can then easily be re-approved upon an eventual rebase)

@holgerd77
Copy link
Member Author

Files changed: 119. Argh. 😁

const tx = AccessListEIP2930Transaction.fromTxData(txData, { common })
```

A mechanism to generate access lists from tx data based on a certain network state in not part of this library.
Copy link
Contributor

@cgewecke cgewecke Mar 19, 2021

Choose a reason for hiding this comment

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

Is there any advice that can be given about how to identify storage slots? i.e associate them with a contract variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgewecke it is possible to derive the storage keys for a contract from its own code (check out this example), however getting the access list for a more complex transaction can be tricky as it can change based on the latest network state.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely think we should introduce some tools to help generate access lists though, I expect that it will be requested if it helps lower the cost of transactions. Perhaps it's better suited at the library layer in web3.js and ethers where there is a connection to an Ethereum node.

Do we think there will be JSON-RPC endpoints or helper tools from wallets like MetaMask to generate contract transactions containing access lists? Etherscan would actually be in a really good position to generate them when creating a tx on their "write contract" page.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we'll relatively for sure add this as a feature to the VM - see also #1152 - since this is just dropping out there on the sideline and will likely also have it's use cases, not sure if it is applicable for these kind of usages since there is still this "where to get the state from" problem?

Would this be another case for an RPC-based StateManager (this was in discussion sometime ago somewhere 😛 ) retrieving all the state by RPC on demand? This would be really cool in general, thinking about this once again (and actually might also work for use cases like MetaMask I would say for generating access lists?).

cgewecke
cgewecke previously approved these changes Mar 19, 2021
Copy link
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

This is huge! Everything LGTM 💯

Really exciting release I think. Looks great!

(Down to just a handful of test failures in the Hardhat upgrade - am not anticipating any last minute surprises coming from over there fwiw.)

packages/tx/CHANGELOG.md Outdated Show resolved Hide resolved
packages/tx/README.md Outdated Show resolved Hide resolved
packages/tx/README.md Outdated Show resolved Hide resolved
@ryanio
Copy link
Contributor

ryanio commented Mar 19, 2021

looks great! found just a few small typos but other than that great job adding thorough notes to the changelogs and readmes :)

@holgerd77
Copy link
Member Author

@ryanio thanks 🥳 , do we want to add your block generation method in the release as well or do you rather want to give this a bit more time?

Co-authored-by: Ryan Ghods <ryan@ryanio.com>
@holgerd77
Copy link
Member Author

BlockBuilder still needs some work #1158, I would then continue here with the planned release schedule and target a release for tomorrow. Can someone give this a final review? 😄

@holgerd77
Copy link
Member Author

holgerd77 commented Mar 22, 2021

(respectively eventually just re-approve, was already reviewed by @cgewecke but I dismissed by applying some typo corrections (these situation are always a bit annoying, on the one side on does not want to leave this in the code, on the other hand it's a bit laborious to have this whole process triggered again. Not sure what's the solution for this))

@cgewecke cgewecke self-requested a review March 22, 2021 13:55
Copy link
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

🚀

@holgerd77 holgerd77 merged commit 4672761 into master Mar 22, 2021
@holgerd77 holgerd77 deleted the berlin-releases branch March 22, 2021 14:23
@ethereumjs ethereumjs deleted a comment from 304s304 Mar 23, 2021
@holgerd77
Copy link
Member Author

Just published the following releases on npm:

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.

3 participants