Rename Module to F3, pass network name to Client, fix error wrapping #308
Conversation
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
|
I know it is an ugly PR (with multiple changes) let me know if I should split of these oneliners. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
=======================================
Coverage 85.33% 85.33%
=======================================
Files 14 14
Lines 1459 1459
=======================================
Hits 1245 1245
Misses 136 136
Partials 78 78
|
| // Validate the message. | ||
| if err = ValidateMessage(comt.power, comt.beacon, p.host, msg); err != nil { | ||
| return nil, fmt.Errorf("%w: %w", ErrValidationInvalid, err) | ||
| return nil, xerrors.Errorf("%v: %w", err, ErrValidationInvalid) |
There was a problem hiding this comment.
Can you please remind me what is the advantage of using xerrors over fmt in this case?
There was a problem hiding this comment.
It tracks line information and displays it when formatted with %+v, making debugging easier.
The key change here is that the first %w was invalid.
| Client | ||
| runner *gpbftRunner | ||
| } | ||
| type gpbftHost gpbftRunner |
There was a problem hiding this comment.
To separate internal (towards gpbft.Participant) and external (towards f3.F3) APIs
There was a problem hiding this comment.
My sense is that this is superfluous if the goal is to make the code easier to understand for mere mortals who maintain this repo.
|
|
||
| // gpbftRunner is responsible for running gpbft.Participant, taking in all concurrent events and | ||
| // passing them to gpbft in a single thread. | ||
| type gpbftRunner struct { |
There was a problem hiding this comment.
Would be nice to reduce concepts here: gpbftHost/gpbftRunner/Module.
There was a problem hiding this comment.
Module got renamed F3, host and runner are the same thing just with different APIs exposed
There was a problem hiding this comment.
Module got renamed F3
Has this change been pushed in this PR?
There was a problem hiding this comment.
Uhhh, I accidentally un-did it.
|
Also please make sure PR description is in place as this repo uses squash merge and GitHub picks the PR description as commit message by default 🙏 |
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
| return func() { s.Close() }, s.Start() | ||
| } | ||
|
|
||
| type fakeSigner struct { |
There was a problem hiding this comment.
What is the rationale for having this wrapper?
There was a problem hiding this comment.
I need to override the Marshal function.
| Client | ||
| runner *gpbftRunner | ||
| } | ||
| type gpbftHost gpbftRunner |
There was a problem hiding this comment.
My sense is that this is superfluous if the goal is to make the code easier to understand for mere mortals who maintain this repo.
It simplifies the structure of F3, gpbftHost/Runner. gpbftHost becomes a newtype of gpbftRunner to isolate exposure of APIs.