fix: Update SyncChainMmr endpoint upper bound logic#1860
fix: Update SyncChainMmr endpoint upper bound logic#1860
Conversation
sergerad
added
the
no changelog
| oneof upper_bound { | ||
| // Sync up to this specific block number (inclusive), clamped to the effective tip. | ||
| fixed32 block_num = 3; | ||
| // Sync up to the latest committed block (chain tip). | ||
| bool last_committed = 4; | ||
| // Sync up to the latest proven block. | ||
| bool last_proven = 5; | ||
| } |
There was a problem hiding this comment.
Is this the recommended way to emulate sum types in protobuf?
Haven't seen bool used as a placeholder before - what happens if they're manually set to false?
There was a problem hiding this comment.
The value of the boolean is ignored
match value {
UpperBound::BlockNum(block_num) => Ok(Self::BlockNumber(block_num.into())),
UpperBound::CommittedChainTip(_) => Ok(Self::CommittedChainTip),
UpperBound::ProvenChainTip(_) => Ok(Self::ProvenChainTip),
}There was a problem hiding this comment.
I know. But since these are numbered fields, and protobuf default inits fields at the receiver, what actually happens if I send last_proven = false -- does it not still get last_committed = false.
There was a problem hiding this comment.
Not entirely sure. Depends on what the generator does under the hood but I thought better to change oneof + enum anyway.
SantiagoPittella
left a comment
SantiagoPittella
left a comment
There was a problem hiding this comment.
Looks good, agree w Mirko's comments.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one comment inline and agree with @Mirko-von-Leipzig's comments.
4 participants
Closes #1842.