Remove unnecessary field from MaybeBytes #4259
Conversation
alarso16
requested review from
StephenButtolph,
joshua-kim and
rrazvan1
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the MaybeBytes protobuf message by removing the redundant IsNothing boolean field and using nil to represent the "Nothing" state instead. This change streamlines the protocol by using the natural protobuf pattern where nil messages represent absent values.
Key changes:
- Removed
IsNothingfield fromMaybeBytesprotobuf definition - Updated conversion logic to use nil/non-nil patterns instead of boolean flags
- Cleaned up validation code that previously checked for invalid state combinations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proto/sync/sync.proto | Removed is_nothing field from MaybeBytes message and added clarifying comment |
| proto/pb/sync/sync.pb.go | Generated protobuf code reflecting the field removal |
| x/sync/network_server.go | Updated conversion functions and removed validation for invalid state combinations |
| x/sync/manager.go | Simplified request building to use new conversion helper function |
| x/merkledb/proof.go | Updated ProofNode and ChangeProof serialization to use nil pattern |
| x/merkledb/proof_test.go | Removed tests for invalid state combinations that are no longer possible |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func MaybeToProto(m maybe.Maybe[[]byte]) *pb.MaybeBytes { | ||
| if m.IsNothing() { | ||
| return nil | ||
| } | ||
| return &pb.MaybeBytes{ | ||
| Value: m.Value(), | ||
| } | ||
| } | ||
|
|
||
| func ProtoToMaybe(mb *pb.MaybeBytes) maybe.Maybe[[]byte] { | ||
| if mb == nil { | ||
| return maybe.Nothing[[]byte]() | ||
| } | ||
| return maybe.Some(mb.Value) | ||
| } |
There was a problem hiding this comment.
The cost of copy-paste here isn't bad and avoids the awkward utils package (which typically ends up becoming a trashcan package without a clear abstraction boundary). Even if the cost of copy and paste was worse, we're not maintaining merkledb long-term anyways.
There was a problem hiding this comment.
I feel like this aligns reasonably with grpcutils
3 participants
Why this should be merged
Redundant field, closes #4256
How this works
Use nil as Nothing.
How this was tested
Existing UT
Need to be documented in RELEASES.md?
No