Skip to content

Block sync#139

Merged
tzdybal merged 11 commits intomainfrom
tzdybal/block_sync
Oct 13, 2021
Merged

Block sync#139
tzdybal merged 11 commits intomainfrom
tzdybal/block_sync

Conversation

@tzdybal
Copy link
Contributor

@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/go-acc@v0.2.6
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
Contributor 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.

4 participants

@tzdybal @codecov-commenter @adlerjohn @liamsi