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

refactor(service/header): extract store initialization from Syncer #430

Merged
merged 10 commits into from
Feb 15, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Feb 12, 2022

Previously, Syncer managed Store initialization, which means putting initial/genesis head to the Store, so the Node can sync on top. It's definitely not Syncer's responsibility to manage this and this was causing me troubles while I worked on other stuff, so I decided to extract that. As result, we have a more coherent and transparent initialization code.

Based on #428
Closes: #447

Review commit by commit

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict

@Wondertan
Copy link
Member Author

Wondertan commented Feb 13, 2022

Yes and it is very minor, but you can review the PR anyway. I am going to resolve the conflict here and in other PRs altogether with any other rebase conflicts that might appear after applying review feedback.

node/services/service.go Outdated Show resolved Hide resolved
node/services/service.go Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
service/header/interface.go Outdated Show resolved Hide resolved
service/header/store.go Show resolved Hide resolved
service/header/store.go Show resolved Hide resolved
service/header/store.go Show resolved Hide resolved
Wondertan added a commit that referenced this pull request Feb 15, 2022
Wondertan added a commit that referenced this pull request Feb 15, 2022
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs issue in opening comment

node/services/service.go Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
service/header/store.go Show resolved Hide resolved
service/header/store_height_indexer.go Outdated Show resolved Hide resolved
service/header/store_height_indexer.go Show resolved Hide resolved
service/header/store_init.go Outdated Show resolved Hide resolved
@Wondertan Wondertan changed the base branch from hlib/store-heightsub to main February 15, 2022 16:09
Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
@Wondertan
Copy link
Member Author

Added closing issue

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but some problems with race -- these are known issues though right?

@Wondertan
Copy link
Member Author

Right

@Wondertan Wondertan merged commit d49912b into main Feb 15, 2022
@Wondertan Wondertan deleted the hlib/store-init branch May 14, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:header Extended header
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

service/header: extract store initialization from Syncer
4 participants