Skip to content

Block sync #139

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.

Sign up for GitHub

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

Jump to bottom
Merged
tzdybal merged 11 commits into from
Oct 13, 2021
Merged

Block sync #139

tzdybal merged 11 commits into from
Oct 13, 2021

Conversation

@tzdybal
Copy link
Member

@tzdybal tzdybal commented Oct 6, 2021 •
edited
Loading

  • blocks passed to blockManager after gossiping
  • aggregator renamed to blockManager
  • DALC(BlockRetriever) used to fetch block data
  • more refactoring (move blockManager out of node package)
  • more tests
    • block propagation specific tests
    • more assertions in integration test

@tzdybal
Block sync
ea0be5c 
* aggregator renamed to blockManager
* blocks are passed to blockManager after gossiping
* DALC(BlockRetriever) used to fetch block data
* more refactoring needed
* more tests needed
@tzdybal tzdybal self-assigned this Oct 6, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021 •
edited
Loading

Codecov Report

Merging #139 (4bf2ca5) into main (bed9ce7) will decrease coverage by 1.50%.
The diff coverage is 73.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
- Coverage   57.07%   55.57%   -1.51%     
==========================================
  Files          24       29       +5     
  Lines        2127     3741    +1614     
==========================================
+ Hits         1214     2079     +865     
- Misses        786     1375     +589     
- Partials      127      287     +160
Impacted Files Coverage Δ
block/manager.go 70.49% <70.49%> (ø)
node/node.go 65.03% <87.50%> (+12.15%) ⬆️
da/mock/mock.go 92.68% <89.47%> (+23.71%) ⬆️
state/executor.go 69.44% <100.00%> (ø)
types/pb/optimint/optimint.pb.go 43.87% <0.00%> (ø)
log/test/loggers.go 80.95% <0.00%> (ø)
conv/abci/block.go 100.00% <0.00%> (ø)
da/registry/registry.go 75.00% <0.00%> (ø)
mocks/Application.go 32.30% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bed9ce7...4bf2ca5. Read the comment docs.

@tzdybal tzdybal marked this pull request as ready for review October 8, 2021 16:05
@tzdybal tzdybal force-pushed the tzdybal/block_sync branch from b608536 to 9748cea Compare October 9, 2021 13:22
@adlerjohn
Copy link
Contributor

adlerjohn commented Oct 10, 2021

Can you resolve merge conflicts before review? ty

adlerjohn
adlerjohn reviewed Oct 10, 2021
View reviewed changes
@tzdybal tzdybal force-pushed the tzdybal/block_sync branch from 1ae0dbe to f91bef0 Compare October 11, 2021 19:42
@tzdybal tzdybal requested a review from adlerjohn October 12, 2021 06:57
liamsi
liamsi reviewed Oct 12, 2021
View reviewed changes
Copy link
Contributor

@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.

Looks great on first sight! More in-depth review following soon.

.github/workflows/test.yml
Comment on lines +26 to +27
go install github.com/ory/[email protected]
go-acc -o coverage.txt ./... -- -v --race
Copy link
Contributor

@liamsi liamsi Oct 12, 2021

Choose a reason for hiding this comment

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

Interesting, I didn't know about this tool.

Copy link
Member Author

@tzdybal tzdybal Oct 12, 2021

Choose a reason for hiding this comment

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

You can read about it here: https://www.ory.sh/golang-go-code-coverage-accurate/

adlerjohn
adlerjohn approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@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.

Looks fine to me, but suggest waiting for a review from @liamsi as well.

liamsi
liamsi approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@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.

I don't fully understand the rationale behind the renaming to block manager but this looks good to me.

@tzdybal tzdybal merged commit 0826c05 into main Oct 13, 2021
@tzdybal tzdybal deleted the tzdybal/block_sync branch October 13, 2021 19:42
This was referenced Oct 13, 2021
Closed
Closed
@tzdybal tzdybal mentioned this pull request Oct 14, 2021
Merged
@claude claude bot mentioned this pull request Oct 14, 2025
Merged
@claude claude bot mentioned this pull request Nov 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamsi liamsi liamsi approved these changes

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

@mattdf mattdf Awaiting requested review from mattdf

+1 more reviewer

@adlerjohn adlerjohn adlerjohn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tzdybal tzdybal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tzdybal @codecov-commenter @adlerjohn @liamsi