V2 #14
V2 #14
Conversation
message/message.go
Outdated
| } | ||
|
|
||
| // Keccak hash engine for arbitrary input | ||
| type Keccak interface { |
There was a problem hiding this comment.
This doesn't have to be a custom interface, we can keccak internally
| func (s *MsgStore) Add(m message.Message) error { | ||
| info := m.GetInfo() | ||
| if info == nil { | ||
| return fmt.Errorf("%w: missing info field", ErrInvalidMessage) |
There was a problem hiding this comment.
Why not have these as custom errors instead of wrapping ErrInvalidMessage every time?
It forces custom parsing on the caller side
message/store/store_test.go
Outdated
| }, | ||
|
|
||
| { | ||
| expectedErrStr: "ok", |
There was a problem hiding this comment.
Please drop this pattern :)
message/message.go
Outdated
| // Signer is identified by its Address and used to generate signature for arbitrary payload | ||
| type Signer interface { | ||
| // Address returns the public ID of Signer | ||
| Address() []byte |
There was a problem hiding this comment.
I'm not entirely convinced it should be in the Signer interface. If we can move it, then Sign can just be a callback instead of an abstraction
sequencer/options.go
Outdated
| Feed message.Feed | ||
| Keccak message.Keccak | ||
| SignatureVerifier message.SignatureVerifier | ||
| Round0Duration time.Duration |
There was a problem hiding this comment.
This should have a default instead of being required to be specified explicitly
There was a problem hiding this comment.
We can just configure it through an option
| return bz | ||
| } | ||
|
|
||
| func (rcc *RoundChangeCertificate) HighestRoundBlock() ([]byte, uint64) { |
There was a problem hiding this comment.
Having this here feels random
message/message.go
Outdated
| } | ||
|
|
||
| // SignMsg returns msg with updated Signature field | ||
| func SignMsg[M IBFTMessage](msg M, signer Signer) M { |
There was a problem hiding this comment.
You don't need this insane indirection.
You generate the message signature when you want to send the message, and you have custom handlers for each message type (broadcasts)...why not just define a method on the message type called SignaturePayload and pipe it into the Sign function, inside those custom message type multicast funcs
There was a problem hiding this comment.
This function is literally called from those very same handlers where you know the exact message type, and yet there is typecasting here -- useless
message/message.go
Outdated
| } | ||
|
|
||
| // WrapMessages wraps concrete message types into Message type | ||
| func WrapMessages[M IBFTMessage](messages ...M) []Message { |
message/message.go
Outdated
| Subscription[M IBFTMessage] chan MsgNotification[M] | ||
| ) | ||
|
|
||
| func (r MsgNotificationFn[M]) Unwrap() []M { |
There was a problem hiding this comment.
Asking because the subscription already has type information on what it should return
| @@ -59,15 +59,13 @@ linters: | |||
| - gocheckcompilerdirectives # Checks that compiler directive comments (//go:) are valid | |||
| - gochecknoinits # Checks for init methods | |||
| - whitespace # Tool for detection of leading and trailing whitespace | |||
| - wsl # Forces you to use empty lines | |||
There was a problem hiding this comment.
Please bring back this + nlreturn
We can configure wsl if it's too noisy
|
I won't block the PR for the following reason, but I think we did one downgrade in terms of v1, which is the removal of full cluster tests (arbitrary cluster sizes) |
Description
Major refactor of the module.
sequencermessageengineibft.gofile)Changes include
Breaking changes
Everything
Checklist