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

Remove fork designation and fork guard aliases #7183

Open
nflaig opened this issue Oct 21, 2024 · 8 comments
Open

Remove fork designation and fork guard aliases #7183

nflaig opened this issue Oct 21, 2024 · 8 comments
Labels
scope-devex Issues for improving developer experience.
Milestone

Comments

@nflaig
Copy link
Member

nflaig commented Oct 21, 2024

Remove fork designation and fork guard aliases

/*
* Aliases only exported for backwards compatibility. This will be removed in
* lodestar v2.0. The types and guards above should be used in all places as
* they are more correct than using the "main feature" from a fork.
*/


Changes below have been implemented by #7441

Somehing we should consider for the 2.0 release is to rename pre / post fork types as current semantics are confusing and require feature awareness of each fork instead of just knowing about the fork name itself, and order of forks, which is simple since it's alphabetical.

e.g.

ForkPreBlobs --> ForkPreDeneb

ForkBlobs --> ForkPostDeneb

isForkBlobs --> isForkPostDeneb

etc.

see https://github.com/ChainSafe/lodestar/blob/unstable/packages/params/src/forkName.ts

@nflaig nflaig added the scope-devex Issues for improving developer experience. label Oct 21, 2024
@philknows philknows added this to the v2.0.0 milestone Oct 21, 2024
@philknows
Copy link
Member

Related #7011

@ensi321
Copy link
Contributor

ensi321 commented Oct 21, 2024

What's the reason we want to wait for 2.0 to make this change? Seems to me is just a straightforward refactor in the code.

Unless this feature is notable enough that we want to promote it as part of 2.0 launch

@nflaig
Copy link
Member Author

nflaig commented Oct 21, 2024

What's the reason we want to wait for 2.0 to make this change?

we should avoid breaking changes to @lodestar/types without bumping the major version, in case of this refactor, I don't think it's that urgent

@matthewkeil
Copy link
Member

Started this process on peerDAS branch PR #6353 to get the unit tests to pass in params package

fda353d

@nflaig
Copy link
Member Author

nflaig commented Feb 7, 2025

unit tests to pass in params package

how is a renaming of functions related to unit tests passing?

@matthewkeil
Copy link
Member

unit tests to pass in params package

how is a renaming of functions related to unit tests passing?

Adding fulu to the forkBlobs snapshot array was semantically incorrect. There are also a number of places in the code that ForkBlobs should be used to differentiate blobs vs columns code but that is not how the types/guards are setup.

@nflaig
Copy link
Member Author

nflaig commented Feb 10, 2025

Adding fulu to the forkBlobs snapshot array was semantically incorrect.

why? we still have blobs, the only place we use columns instead are on p2p

There are also a number of places in the code that ForkBlobs should be used to differentiate blobs vs columns code

hmm I am not sure if I understand what you mean here but in general can't you just use isForkPostFulu() to run the column code?

matthewkeil added a commit that referenced this issue Feb 12, 2025
**Motivation**

Start of resolution to #7183. Driven by peerDAS branch. `isForkBlobs` is
not longer semantically correct in `fulu` because that branch does not
have blobs, it has columns. Is the first major feature that was removed
so the types/typeguard naming semantics we were using kinda broke.
Updated to reflect pre/post fork instead of post"Feature".

**Description**

Rename pre/post fork names and type guards.

---------

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
@nflaig
Copy link
Member Author

nflaig commented Feb 12, 2025

Renaming has been done in #7441, we can keep this issue open to remove the aliases in our 2.0 release.

@nflaig nflaig changed the title Rename pre / post fork types Remove fork designations and fork guards aliases Feb 12, 2025
@nflaig nflaig changed the title Remove fork designations and fork guards aliases Remove fork designation and fork guard aliases Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-devex Issues for improving developer experience.
Projects
None yet
Development

No branches or pull requests

5 participants
@ensi321 @matthewkeil @nflaig @philknows and others