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

New Library Releases (tx.supports() integration, Common custom chains + enum Chain/HF, Util v7.1.0) #1327

Merged
merged 15 commits into from Jul 8, 2021

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Jun 30, 2021

Releases with finalized london support and other substantial updates.

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #1327 (2759b0f) into master (e896399) will increase coverage by 1.46%.
The diff coverage is 90.90%.

Impacted file tree graph

Flag Coverage Δ
block 85.68% <50.00%> (+0.16%) ⬆️
blockchain 83.43% <ø> (-0.07%) ⬇️
client 84.81% <ø> (+0.48%) ⬆️
common 93.75% <ø> (ø)
devp2p 83.55% <ø> (ø)
ethash 82.83% <ø> (ø)
tx 88.36% <100.00%> (ø)
vm 82.64% <ø> (+3.40%) ⬆️

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

@holgerd77
Copy link
Member Author

Still WIP and some releases missing (block, blockchain and VM), already open for some early review though. 😄

Would wait on this until Friday to be able to include the london mainnet block to Common, have added the blocked label.

@holgerd77 holgerd77 requested review from ryanio, alcuadrado and emersonmacro and removed request for emersonmacro June 30, 2021 11:58
@holgerd77
Copy link
Member Author

Also //cc in @fvictorio from Hardhat here

@holgerd77
Copy link
Member Author

Short update: I was initially planning to do a mixture of minor version and bugfix releases here depending on the significance of features added to some of the libraries, but I am just realizing - also due to the discussions along #1329 - that this addition of the mainnet (and others) london block numbers to Common (together with bumping the Common version on all of the other releases) is actually a pretty significant thing, since this marks the first releases with full london support (before things like reading block data from mainnet via RPC and then deciding on if it's a pre or post london block would just not work yet).

So I will switch here to do all releases as minor releases - think this is very much justified - and will also more prominently point the london readiness out in the release notes of all libraries once I am updating the blocks.

I would target a Friday evening (CET time) release on this, so it would be great if the PR can already get some (partial) reviews so that we make sure we are ready by then. This would be important for Hardhat and others so that they can integrate early on.

@holgerd77
Copy link
Member Author

One little request: please also take these kind of reviews seriously and do a thorough and conscientious review. While this is not doing substantial code changes code changes along such PRs are nevertheless pretty heavy, and a single misspelled updated version number can in the worst case ruin the whole range of releases. 🙂

@holgerd77
Copy link
Member Author

Hmm, seems it will take another 1-2 days until there is a decision on a london mainnet block, so I guess we'll have to postpone the release until early next week (Monday or eventually Tuesday). Would nevertheless be good if someone can already start reviewing here so that we'll be ready by then.

ryanio
ryanio previously approved these changes Jul 2, 2021
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

just reviewed, looks excellent! helpful additional documentation, comprehensive changelogs, and digging the consistency of the singular enum naming. couldn't even find any typos 😋

@holgerd77
Copy link
Member Author

Ok, I've now added the finalized london block numbers here in 9236f91 and added a respective "Finalized London HF Support" section to the release notes of all libraries, I also included the missing release notes from #1333 (since the PR didn't get merged due to being an out-of-schedule release).

This is now open for a final review (please be a bit loose on eventual additional typos and things on that level). 😄

It would also be good if you don't merge other PRs until this release has been done.

I would then wait for ethereum/execution-specs#223 to be merged and then do the release (likely tomorrow morning I would assume).

@holgerd77
Copy link
Member Author

Client tests are actually failing for the PR, which is a bit annoying (with UncaughtException: Error: Unable to resolve module [multiformats/hashes/sha2] from [/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/libp2p-crypto/src/keys/rsa-class.js]). Might have to do with the package-lock.json file I added on root along this latest commit round.

I likely won't have the time to fix and would have a tendency to ignore on this PR (and merge by admin-merge), since this is clearly not relevant for the PR. If someone wants to investigate and has an easy fix feel free to push here though (make sure to then give an additional approval). Otherwise I guess we can tackle separately as well.

@holgerd77
Copy link
Member Author

Another approval would be nice here, would like to do the releases in 2 hours or so.

@holgerd77
Copy link
Member Author

Ah, removed the package-lock.json and this indeed fixed the CI run for now 😄, this should nevertheless be tackled at some point.

I also added an additional reference to #1330 in the block release notes, @ryanio great that you picked this up - I actually planned to do so as well and integrate in the release but went over it. Really nice, would have been a pity if this would have missed this release round.

Nevertheless: it would still be good if someone could short-term approve here. 😄

(I would otherwise admin-merge since Ryan already reviewed the most part here and I very much planned these releases in for the morning, but would feel a bit uneasy on this)

@holgerd77
Copy link
Member Author

Ok, I will then go the uneasy path and just merge here and do the releases. 🙃

@holgerd77 holgerd77 merged commit 2ba896e into master Jul 8, 2021
@holgerd77 holgerd77 deleted the new-releases branch July 8, 2021 09:08
@holgerd77
Copy link
Member Author

Just released the following releases on npm:

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Checked the .ts files and especially cross-checked the block numbers / fork hashes for the hardforks. Looks good!

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.

None yet

4 participants