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

Finalizer based on LMD votes, not connected to actual balances yet #52

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

leolara
Copy link

@leolara leolara commented Jun 27, 2022

No description provided.

@leolara leolara force-pushed the leolara/lmd-finalizer branch 4 times, most recently from eb2c76b to fafc325 Compare June 28, 2022 07:57
}

// New LMDFinalizer with logger `log`.
func New(latestFinalized *chaindb.Block, log zerolog.Logger, handler newLFBHandler) LMDFinalizer {
Copy link
Collaborator

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.

Copy link
Author

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?

Copy link
Collaborator

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.

services/blocks/standard/lmdfinalizer/finalizer.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants