Conversation
…nstructors
- Add UnmarshalTaggedVersioned/UnmarshalVersioned helpers in types/versions.go
to collapse the decode-then-check-version boilerplate.
- MarshalCBOR now copies the receiver before defaulting Version=1, so
marshaling is a pure read operation.
- Add NewXxx() constructors for top-level versioned types; fix existing
txsystem constructors that produced Version=0 structs.
…yCertificate has changed)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes versioning and CBOR serialization across the codebase by introducing explicit version fields, constructor functions, and centralized unmarshaling helpers. It also updates CBOR tags to a new range (39000+). Feedback identifies a logic inconsistency where GetVersion() provides a default value of 1, causing unmarshaling to succeed for missing versions while subsequent IsValid() checks fail because they inspect the raw version field directly.
| func UnmarshalTaggedVersioned[A any](tag CborTag, expectedVersion Version, data []byte, aliasPtr *A, v Versioned) error { | ||
| if err := Cbor.UnmarshalTaggedValue(tag, data, aliasPtr); err != nil { | ||
| return err | ||
| } | ||
| if got := v.GetVersion(); got != expectedVersion { | ||
| return fmt.Errorf("invalid version (type %T), expected %d, got %d", v, expectedVersion, got) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The UnmarshalTaggedVersioned helper relies on v.GetVersion() to validate the version. However, most implementations of GetVersion() in this repository (e.g., in Header, InputRecord, ShardTreeCertificate) return a default value of 1 if the Version field is 0. This means that unmarshaling will succeed even if the version is missing or zero in the CBOR data.
This creates an inconsistency because the IsValid() methods for these types typically check Version == 1 directly and will fail if the field is 0. This results in a situation where unmarshaling succeeds but produces an object that is considered invalid by its own validation logic.
Consider either making GetVersion() return the actual field value (to enforce explicit versioning during unmarshaling) or updating IsValid() to use GetVersion() so that the default value is respected consistently.
| func (cert ShardTreeCertificate) GetVersion() Version { | ||
| if cert.Version > 0 { | ||
| return cert.Version | ||
| } | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
This implementation of GetVersion() returns 1 if cert.Version is 0. While this provides a default, it conflicts with the IsValid() check on line 43 which requires cert.Version to be exactly 1.
If unmarshaling data with a missing or zero version field, UnmarshalTaggedVersioned will succeed (because GetVersion() returns 1), but the resulting object will fail IsValid().
To fix this, you should either:
- Make
GetVersion()returncert.Versiondirectly to enforce that the version is present in the data. - Update
IsValid()to checkcert.GetVersion() != 1instead ofcert.Version != 1.
func (cert ShardTreeCertificate) GetVersion() Version {
return cert.Version
}
3 participants
part of unicitynetwork/bft-core#18