chore: remove now useless bls init #6513
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6513 +/- ##
============================================
- Coverage 61.72% 61.55% -0.18%
============================================
Files 555 556 +1
Lines 58302 58586 +284
Branches 1846 1847 +1
============================================
+ Hits 35985 36060 +75
- Misses 22277 22486 +209
Partials 40 40 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Mar 6, 2024
packages/light-client/src/index.ts
Outdated
| @@ -149,15 +150,16 @@ export class Lightclient { | |||
| static async initializeFromCheckpointRoot( | |||
| args: Omit<LightclientInitArgs, "bootstrap"> & { | |||
| checkpointRoot: phase0.Checkpoint["root"]; | |||
| blsImplementation?: Implementation; | |||
There was a problem hiding this comment.
Having an internal implementation as part of arguments seems bit strange to me, if this is really required can't we follow same approach used in ssz and discv5, i.e. using a setter function
another example
There was a problem hiding this comment.
setHasher is setting some global then? I think we should move away from those, they introduce fragility and break expectations.
e.g. setImplementation could be called after the init is done, making it void.
There is a similar issue with the active preset, where you have to do things in the right order or it breaks. It also introduces coupling (or forces lack of coupling) between dependencies.
In general I fell like explicit is better over implicit (env variable, global variable), and letting user decide of implementation sounds like it.
There was a problem hiding this comment.
they introduce fragility and break expectations.
How does this change expectations though? From the code I would expect now I can do this
lc1.initializeFromCheckpointRoot({blsImplementation: "herumi"})
// this light client instance will use different bls implementation
lc2.initializeFromCheckpointRoot({blsImplementation: "blst-native"})but does this actually work and it that the intended use case?
There is a similar issue with the active preset, where you have to do things in the right order or it breaks. It also introduces coupling (or forces lack of coupling) between dependencies.
This is a protection that ensures preset values can't be modified after those are loaded, much better to throw an error explicitly than to risk inconsistencies.
There was a problem hiding this comment.
How does this change expectations though? From the code I would expect now I can do this
It looks marginally better to me but indeed there is still room to shoot yourself in the foot.
Your comment made me realize that having explicit init specifically here is probably wrong (and hopefully not required anymore as related to karma). Indeed, some code changes might force init to be already done before this code is reached.
A better solution might be:
- to remove
initcode fromlight-client - to address configurability directly in
blslibrary
There was a problem hiding this comment.
I see two patterns we can follow here
- either use dependency injection
- or load and set internally
If we wanna use dependency injection, the whole implementation should be passed imo and not just a name which then causing the library to be loaded internally.
I feel like the change in this PR was mixing both concepts.
they introduce fragility and break expectations.
I think the safest approach is to allow setter to be called exactly once to ensure same implementation is used globally. Dependency injection gives more flexibility but also has the risk that you have to pass the implementation in possibly multiple places and you might accidentally miss one and it implicitly uses a default / fallback implementation.
That's how setHasher works and I think it's good for this use case as you would usually wanna use same hashing implementation across the whole project.
maybe @wemeetagain can give some more insights on this
There was a problem hiding this comment.
I shared some ideas here: ChainSafe/bls#157
There was a problem hiding this comment.
With setHasher, user must ensure that the call is made at the right moment or there is a risk of having some default implementation being used (the one that kicks in when setHasher is not called).
Combined with other libs being intertwined, this looks a bit fragile to me (and hard to know for sure the call has been properly taken into consideration).
There was a problem hiding this comment.
Removed the implementation selection, this code was specific to karma anyway.
jeluard
changed the title
nflaig
approved these changes
Mar 13, 2024
|
🎉 This PR is included in v1.18.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Remove
blsinitcode previously required for karma.