relab / hotstuff

MIT License
167 stars 53 forks source link

eventloop: make it an interface #57

Closed johningve closed 1 year ago

johningve commented 2 years ago

This adds an interface for the event loop, allowing for alternative implementations or overriding methods.

I added the interface in order to override the AddEvent method for the Twins nodes, but I no longer have a need for that. However, I think it might be useful to have this interface in case it becomes useful later.

codecov-commenter commented 2 years ago

Codecov Report

Merging #57 (a0ff339) into master (3e72795) will increase coverage by 0.13%. The diff coverage is 57.89%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   53.72%   53.85%   +0.13%     
==========================================
  Files          70       70              
  Lines        6362     6365       +3     
==========================================
+ Hits         3418     3428      +10     
+ Misses       2670     2662       -8     
- Partials      274      275       +1     
Flag Coverage Δ
unittests 53.85% <57.89%> (+0.13%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/modules.go 60.78% <20.00%> (-3.80%) :arrow_down:
eventloop/eventloop.go 49.62% <71.42%> (-2.97%) :arrow_down:
client/client.go 75.27% <0.00%> (-2.20%) :arrow_down:
consensus/consensus.go 65.86% <0.00%> (+2.39%) :arrow_up:
consensus/chainedhotstuff/chainedhotstuff.go 89.28% <0.00%> (+5.35%) :arrow_up:
consensus/votingmachine.go 85.71% <0.00%> (+14.28%) :arrow_up:

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 3e72795...a0ff339. Read the comment docs.

meling commented 1 year ago

Have done a quick high-level scan of this PR. My general comment is that the number of methods in the interface is larger than typical Go interfaces. Also, it is recommended practice to return structs from constructor functions instead of an interface. Will try to get some time to look more carefully at the whole code base before making recommendations.

meling commented 1 year ago

I'm closing this PR since it goes against the idiomatic Go style of keeping interfaces small, and interfaces are usually not returned from (constructor) functions but instead accepted as input to functions. Interfaces should be defined where they are used, not upfront, just in case it becomes useful.

The book 100 Go Mistakes and How to Avoid Them provides some good guidelines for use of interfaces in items