-
Notifications
You must be signed in to change notification settings - Fork 33
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
Finalizer based on LMD votes, not connected to actual balances yet #52
base: master
Are you sure you want to change the base?
Conversation
eb2c76b
to
fafc325
Compare
fafc325
to
1d713fa
Compare
} | ||
|
||
// New LMDFinalizer with logger `log`. | ||
func New(latestFinalized *chaindb.Block, log zerolog.Logger, handler newLFBHandler) LMDFinalizer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already starting to be overloaded with parameters. Using the .WithX()
parameter system that is used in other modules in chaind
would make this easier to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two are kind of mandatory. I guess I can make log optional. Do you want me to add a WithX() for latestFinalized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally everything but the context would be a parameter as it makes future changes significantly easier to manager (for example, if we wanted to allow multiple handlers we could just add a WithHandlers()
alongside the additional WithHandler()
and it would not break any existing code.
No description provided.