Release - 1.2.6 (new PR #3353)

[nivida]

Description

This release will be done without the release of an RC version and the defined test period of one week because of the importance changing of the ENS Registry address does have.

Release Guideline

Added

• ENS module extended with the possibility to add a custom registry (Adds the possibility to define a custom registry for the ENS module #3301)
• Görli testnet ENS registry added to the known registries (Adds the görli testnet ENS registry to the known registries #3338)

Changed

• Ensure '0x' prefix is existing for Accounts.sign and Accounts.privateKeyToAccount (web3.eth.accounts.privateKeyToAccount accepts private key without "0x" prefix but returns incorrect result #3041)
• All ENS registry addresses changed to the newly deployed Registry address

Fixed

• Add missing subscription.on('connected') TS type definition (missing types for Subscription.on('connected') #3319)

Compare View

v1.2.5 -> v1.2.6

Type of change

• Release

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run test with success and extended the tests if necessary.
• I ran npm run build and tested the resulting file from dist folder in a browser.
• I have tested the minified file locally

[coveralls]

Coverage remained the same at 85.466% when pulling 1ccfe27 on release/1.2.6 into a121b6f on 1.x.

[alcuadrado]

I think it would be better to release 1.2.6 with only the ENS addresses changes. This minimizes the chance of breaking something else, while still updates the ENS registry.

[cgewecke]

@nivida I agree with @alcuadrado

1.2.6 should be the smallest difference from 1.2.5 which will work. We should honor the ideas behind the release process as closely as possible to reduce risk.

[michaelsbradleyjr]

ENS changes look good!

One question: when I look at the v1.2.5...release/1.2.6 comparison, there seem to be more changes than listed in the Added...Changed...Fixed lists above. So is the intention to release with only the commits corresponding to those lists or all the commits seen in the comparison view?

[nivida]

@alcuadrado @cgewecke

1.2.6 should be the smallest difference from 1.2.5 which will work. We should honor the ideas behind the release process as closely as possible to reduce risk.
I think it would be better to release 1.2.6 with only the ENS addresses changes. This minimizes the chance of breaking something else, while still updates the ENS registry.

I see the point but do not agree with it in this case. Because we do have time to check those small improvements which will definitely not introduce a breaking change are we able to release them by side of the changed addresses. I really appreciate to be as aligned as possible with the Release Guidlines we do have defined but I'm the meaning we shouldn't exaggerate.

@michaelsbradleyjr

ENS changes look good!

👌

v1.2.5...release/1.2.6

About what changes do you talk about? The CHANGELOG got updated always and there shouldn't be any missing point.

[cgewecke]

@nivida

we shouldn't exaggerate

I can see the point you're making, although I think it's easier to have confidence that everything works if you contribute to the project every day.

The release protocol makes sure the broader community has a chance to evaluate how they might be affected by changes. Its goal is to bridge the gap between what they know about their code and what the maintainers here believe is ok.

Keeping the change-set as small as possible vs release/1.2.5 seems reasonable and should not impose undue burdens on this release.

Why not be extra-cautious here - even if it seems unnecessary - esp. since some post 1.2.5 changes directly affect the ENS module?

[nivida]

@cgewecke Thanks for your input and to explain your thoughts.

it's easier to have confidence

I wouldn't say it is just confidence because our test cases do cover enough to be sure anything does work as it was working before. The changes we have applied to the code base do not remove any single line they just add optional additional logic which can be released as a patch release. By side of it did we improve our PR review process extremely to be sure anything which does get merged will not introduce a breaking change.

Keeping the change-set as small as possible vs release/1.2.5 seems reasonable...

I think this isn't something which we should see as an unwritten rule. Let us define exactly the rules for the changeset for a normal release and what exactly can be included in an "emergency case" as we have now. This would probably also be a good point to add different emergency levels and the related escalation rules to it.

Why not be as cautious as possible here - even if it seems unnecessary - esp. since some post 1.2.5 changes directly affect the ENS module?

Because the changes do definitely not introduce a breaking change and we reviewed them extensively. Those are clearly changes that can be released with a patch release without any further steps.

[cgewecke]

Let us define exactly the rules for the changeset for a normal release and what exactly can be included in an "emergency case" as we have now. This would probably also be a good point to add different emergency levels etc. and the related escalation rules to it.

Yes, agree. Unfortunately as often happens what to do in emergencies is discovered in an emergency.

Even though you are likely correct about the current 1.x branch being publishable, it still seems like the most conservative thing to do is print 1.2.5 plus address changes, (maybe plus the goerli address as in #3338?)

I don't think your assessment of the safety of 1.x is simply "wrong" or something - just that there is a lower-risk alternative which is also available and that's what looks closest to the ideas expressed by the release/review guidelines.

[holgerd77]

I also would pledge here for the minimal version, no need here to break out here from the defined new guidelines, this will unnecessarily lower the trust in the process.

[michaelsbradleyjr]

@nivida

About what changes do you talk about?

I looked again, and it seems that the other commits in the comparison mostly involve changes to tests/docs, so I was mistaken that something was left out of the Added...Changed...Fixed lists.

[nivida]

@michaelsbradleyjr 👌 (this confusion is a clear argument to introduce conventional commits as once mentioned by you)

It is probably worth to create a Release Process Improvements issue out of the discussions we had and to list all required improvements up and to be open for further feedback from the community and especially from the contributors of the web3.js project.

Now to come back to the actual topic we have. Instead of proceeding the discussion is it probably better to list up the facts we have and to decide based on those.

---

Release of the already proposed PR

Violated Release Rules: 1, 2, 3, and 4 (with exception of the last sentence)
Violated PR Rules: 2

Other Disadvantages

• Uncertainty by the reviewers also with existing unit and e2e test cases for web3.eth.accounts.privateKeyToAccount accepts private key without "0x" prefix but returns incorrect result #3041 and Adds the possibility to define a custom registry for the ENS module #3301.

---

Proposed release from the reviewers

Violated Release Rules: 1, 2, 3, and 4 (with exception of the last sentence)
Violated PR Rules: 2
Violated Review Rules: 2

Other Disadvantages

• Release PR can't get merged back into 1.x (base head of the release branch will be behind)
• The time until the new addresses do get published will get increased

---

It would be interesting to find out what violations should weigh how heavy and how we handle the listed disadvantages in the future.

I will publish it tomorrow and do what we have decided (CEST).
@holgerd77 @cgewecke @alcuadrado @JosefJ

Edit:
My vote is to release the current PR because of the emergency case it is and the QA we have.

Release of the already proposed PR: 🚀
Proposed release from @cgewecke: : ❤️

[holgerd77]

@nivida Thanks for the structured writeup and summary, really great to have some in-between analysis here. I am a bit in doubt if counting the number of violated rules is a fair comparison here, since a single non-RC-channeled change atm just violates as much rules as all additional arbitrary number of changes. 😛

I think we just have a new and additional case here which hasn't been included in the release process definition yet. So I think it's a good idea to open a new Release Process Improvements issue and add rules for such an emergency case and eventually also discuss other improvement ideas coming up.

[holgerd77]

@nivida Really love the icons you've got chosen for the vote, very intuitive. 😋

[nivida]

I've opened #3353 which does show clearly the discrepancy between 1.x -> release branch and I've changed the title of this PR to forward a user to the correct PR for the release.

@michaelsbradleyjr Thanks for your vote but I've followed the recommendations from the overall team lead of the EF JS Team @holgerd77 :)