Finalization of the roster change #43
Conversation
Gilthoniel
commented
May 11, 2020
Gilthoniel
commented
- Implements a catch up process for new joining participant
- Implements roster change including new players
- Unit tests
Gilthoniel
force-pushed
the
roster_change_full
branch
from
165efe7
to
2dfb970
Compare
| "golang.org/x/xerrors" | ||
| ) | ||
|
|
||
| type operations struct { |
There was a problem hiding this comment.
I would add a small explanation about what an operation is responsible for and what it does.
|
|
||
| err = ops.commitBlock(block) | ||
| if err != nil { | ||
| // No wrapping to avoid redundancy. |
There was a problem hiding this comment.
it is becoming harder and harder to find review's comments 😄
consensus/cosipbft/mod.go
Outdated
| for _, link := range chain.links { | ||
| if last == nil || bytes.Equal(last.To, link.from[:]) { | ||
| linkpb, err := c.encoder.Pack(link) | ||
| if err != nil { | ||
| return xerrors.Errorf("couldn't pack link: %v", err) | ||
| } | ||
|
|
||
| err = c.storage.Store(linkpb.(*ForwardLinkProto)) | ||
| if err != nil { | ||
| return xerrors.Errorf("couldn't store link: %v", err) | ||
| } | ||
|
|
||
| last = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I personally dislike the way last has a double purpose: first holding the last stored block and then indicating if we started storing new blocks. I would find it easier to understand if using a separate variable for that:
| for _, link := range chain.links { | |
| if last == nil || bytes.Equal(last.To, link.from[:]) { | |
| linkpb, err := c.encoder.Pack(link) | |
| if err != nil { | |
| return xerrors.Errorf("couldn't pack link: %v", err) | |
| } | |
| err = c.storage.Store(linkpb.(*ForwardLinkProto)) | |
| if err != nil { | |
| return xerrors.Errorf("couldn't store link: %v", err) | |
| } | |
| last = nil | |
| } | |
| } | |
| store := false | |
| for _, link := range chain.links { | |
| store := store || bytes.Equal(last.To, link.from[:]) | |
| if store { | |
| linkpb, err := c.encoder.Pack(link) | |
| if err != nil { | |
| return xerrors.Errorf("couldn't pack link: %v", err) | |
| } | |
| err = c.storage.Store(linkpb.(*ForwardLinkProto)) | |
| if err != nil { | |
| return xerrors.Errorf("couldn't store link: %v", err) | |
| } | |
| } | |
| } |
ledger/byzcoin/roster/task.go
Outdated
| if removals[i] == removals[i+1] { | ||
| removals = append(removals[:i], removals[i+1:]...) | ||
| } else { | ||
| // Only moves to the next when all occurances of the same index are |
There was a problem hiding this comment.
| // Only moves to the next when all occurances of the same index are | |
| // Only moves to the next when all occurences of the same index are |
ledger/byzcoin/roster/task.go
Outdated
| player, err := f.unpackPlayer(pb.GetAddr(), pb.GetPublicKey()) | ||
| if err != nil { | ||
| // The error is not wrap to avoid redundancy with the private function. | ||
| return nil, err |
There was a problem hiding this comment.
I am not sure why this would be redundant.
For example without wrap you could have
couldn't decode public key
and if you add one:
failed to unpack player: couldn't decode public key
If I had to choose I would go for the second one. IMO no wrap should be an extreme exception where there is no other way around, because there is often a good reason to avoid a wrap but it is not in our interest when something bad happens. Having more and more exceptions is a slippery slope, (I feel this PR as an unusual number of no-wrap error).
There was a problem hiding this comment.
I have the tendency to use private functions to reduce the code block sizes because I don't like big blocks but I assume it will be redundant when doing that. But it's true it's not always a benefit.
