hotstuff icon indicating copy to clipboard operation
hotstuff copied to clipboard

eventloop: make it an interface

Open johningve opened this issue 2 years ago • 2 comments

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.

johningve avatar May 23 '22 09:05 johningve

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.

codecov-commenter avatar May 23 '22 09:05 codecov-commenter

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 avatar Sep 23 '22 12:09 meling

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

  • 5: Interface pollution,
  • 6: Interface on the producer side, and
  • 7: Returning interfaces

meling avatar Feb 11 '23 19:02 meling