core/qbft: refactor to use generics #501
Conversation
| @@ -23,7 +23,7 @@ repos: | |||
| exclude: ".pb.go" | |||
|
|
|||
| - repo: https://github.com/corverroos/fiximports | |||
| rev: 496e5392530966aa8f12e88a289c07782075cf91 | |||
There was a problem hiding this comment.
had to bump fiximports to support go1.18 generics
Codecov Report
@@ Coverage Diff @@
## main #501 +/- ##
==========================================
- Coverage 56.60% 56.46% -0.15%
==========================================
Files 87 87
Lines 7976 7979 +3
==========================================
- Hits 4515 4505 -10
- Misses 2862 2874 +12
- Partials 599 600 +1
Continue to review full report at Codecov.
|
| // Msg defines the inter process messages. | ||
| type Msg struct { | ||
| type Msg[I any, V Value[V]] interface { |
There was a problem hiding this comment.
Changing message to an interface simplifies explicit justifications, since external buffer of signed messages isn't required.
There was a problem hiding this comment.
Also, here it is not clear to me what is I and what is V
There was a problem hiding this comment.
So, I guess V is the type of the value reaching a consensus on? But I?
There was a problem hiding this comment.
I is the instance of consensus as per the paper
corverroos
force-pushed
the
corver/qbftgeneric
branch
from
e0ccd94
to
990e835
Compare
| @@ -27,34 +26,42 @@ import ( | |||
| "github.com/obolnetwork/charon/app/errors" | |||
| ) | |||
|
|
|||
| // Value defines the generic value type constraint | |||
| // that provides an equality method. | |||
| type Value[V any] interface { | |||
There was a problem hiding this comment.
@corverroos I don't think the name of this interface represents the methods it countais. Something like Equaler is better
There was a problem hiding this comment.
Yeah, but using V Value for the "value" makes a lot more sense than Equaler for "value". In this case, this interface defines what constraints the "value" type. Which is only that it has an equal method.
There was a problem hiding this comment.
I see that this is the interface for the value we are reaching consensus for, it makes sense to have an interface called Value. I was confused because it didn't have other methods, I guess you will need to add it some other method to be able to use it, like something that returns V? In order to use the Value[v] returned by the consensus?
There was a problem hiding this comment.
Yeah, we will use this interface when we integrate it into the core workflow components
| // Transport abstracts the transport layer between processes in the consensus system. | ||
| // | ||
| // Note that broadcasting doesn't return an error. Since this algorithm is idempotent | ||
| // it is suggested to just retry broadcasting indefinitely until it succeeds or times out. | ||
| type Transport struct { | ||
| type Transport[I any, V Value[V]] struct { |
There was a problem hiding this comment.
@corverroos what is I and what is V ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yup:
// Instance identifies the consensus instance.
Instance() I
// Value being proposed.
Value() V
|
I like that messages changes from public fields to public accessors |
corverroos
added
the
merge when ready
Refactors qbft to use generics. This simplified the integration significantly since an external buffer isn't required and casting and conversion and (errors) isn't required.
category: refactor
ticket: #445
feature_set: alpha