feat: Support proper encoding/decoding of TX metadata #1159
Conversation
Signed-off-by: Akhil Repala <[email protected]>
Signed-off-by: Akhil Repala <[email protected]>
Signed-off-by: Akhil Repala <[email protected]>
Signed-off-by: Akhil Repala <[email protected]>
| ) | ||
| } else { | ||
| t.Logf("Length mismatch: original length = %d, re-encoded length = %d", len(dataBytes), len(encoded)) | ||
| } |
There was a problem hiding this comment.
We want to keep the check for the re-encoded CBOR being identical to the original decoded CBOR. If that's failing with this new code, then that should be addressed
There was a problem hiding this comment.
Sure, I will revert back the changes here
ledger/common/tx.go
Outdated
| Type() int | ||
| Cbor() []byte | ||
| Metadata() *cbor.LazyValue | ||
| Metadata() TransactionMetadataSet |
There was a problem hiding this comment.
We don't want to use TransactionMetadataSet here, only TransactionMetadatum. The Set is used at the block level to return the metadatum for all TXs included in the block.
ledger/common/tx.go
Outdated
| type TransactionMetadataSet map[uint64]TransactionMetadatum | ||
|
|
||
| type TransactionMetadatum interface { | ||
| TypeName() string |
There was a problem hiding this comment.
Add a isTransactionMetadatum() function here, so that other types can't accidentally satisfy this interface.
ledger/conway/conway.go
Outdated
| TransactionBodies []ConwayTransactionBody | ||
| TransactionWitnessSets []ConwayTransactionWitnessSet | ||
| TransactionMetadataSet map[uint]*cbor.LazyValue | ||
| TransactionMetadataSet map[uint]common.TransactionMetadataSet |
There was a problem hiding this comment.
This should be TransactionMetadataSet TransactionMetadataSet. What you've got here will give you nested maps, which isn't what we want
There was a problem hiding this comment.
Do you mean TransactionMetadataSet.TransactionMetadataSet?
There was a problem hiding this comment.
Err, I meant TransactionMetadataSet common.TransactionMetadataSet
Signed-off-by: Chris Gianelloni <[email protected]>
📝 WalkthroughWalkthroughThis PR replaces usage of raw CBOR lazy values for transaction metadata with a structured typed model. It adds a new common TransactionMetadatum hierarchy and TransactionMetadataSet with CBOR (un)marshalling, updates the Transaction interface to return the new type, and modifies era implementations (Byron, Shelley, Mary, Allegra, Alonzo, Babbage, Conway) to use the new types and to append metadata directly during CBOR encoding. Tests updated to validate structural round-trips instead of strict byte-for-byte CBOR equality. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Files/areas to focus on:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ledger/babbage/babbage.go (2)
353-359: Bug: address of range variable yields duplicate pointersUsing &input appends the address of the loop variable, so all entries point to the same memory (the last element). Return the value, not its address.
func (b *BabbageTransactionBody) ReferenceInputs() []common.TransactionInput { ret := []common.TransactionInput{} for _, input := range b.TxReferenceInputs.Items() { - ret = append(ret, &input) + ret = append(ret, input) } return ret }
593-599: Bug: leaking zero hash when datum is absentReturning a pointer to an empty Blake2b256 causes RPC encoding to emit 32 zero bytes instead of “no datum”. Return nil when absent.
func (o BabbageTransactionOutput) DatumHash() *common.Blake2b256 { - if o.DatumOption != nil { - return o.DatumOption.hash - } - return &common.Blake2b256{} + if o.DatumOption != nil && o.DatumOption.hash != nil { + return o.DatumOption.hash + } + return nil }ledger/conway/conway.go (1)
468-474: Bug: address of range variable yields duplicate pointersSame issue as Babbage: &input captures the loop variable address; all entries will be the same. Append the value.
func (b *ConwayTransactionBody) ReferenceInputs() []common.TransactionInput { ret := []common.TransactionInput{} for _, input := range b.TxReferenceInputs.Items() { - ret = append(ret, &input) + ret = append(ret, input) } return ret }
♻️ Duplicate comments (2)
ledger/allegra/block_test.go (1)
63-76: Reinstate exact CBOR byte-for-byte round‑trip checkWe should keep the strict equality check in addition to the structural assertions, as previously requested. If it fails, that indicates an encoding divergence we must fix rather than relaxing the test.
Apply this diff right after encoding:
@@ if encoded == nil || len(encoded) == 0 { t.Fatal("Custom encoded CBOR from AllegraBlock is nil or empty") } - // Ensure the re-encoded CBOR is structurally valid and decodes back + // Exact byte-for-byte match with original CBOR + assert.Equal(t, dataBytes, encoded, "Re-encoded CBOR must match original CBOR") + + // Ensure the re-encoded CBOR is structurally valid and decodes back var redecoded allegra.AllegraBlock if err := redecoded.UnmarshalCBOR(encoded); err != nil { t.Fatalf("Re-encoded AllegraBlock failed to decode: %v", err) }ledger/common/tx.go (1)
34-35: Seal theTransactionMetadatuminterface.Add an unexported marker (e.g.,
isTransactionMetadatum()) so unrelated types can’t accidentally satisfy the interface. This was requested previously and is still applicable.Apply this diff:
type TransactionMetadatum interface { - TypeName() string + isTransactionMetadatum() + TypeName() string } type MetaInt struct { Value int64 } +func (MetaInt) isTransactionMetadatum() {} type MetaBytes struct { Value []byte } +func (MetaBytes) isTransactionMetadatum() {} type MetaText struct { Value string } +func (MetaText) isTransactionMetadatum() {} type MetaList struct { Items []TransactionMetadatum } +func (MetaList) isTransactionMetadatum() {} type MetaPair struct { Key TransactionMetadatum Value TransactionMetadatum } type MetaMap struct { Pairs []MetaPair } +func (MetaMap) isTransactionMetadatum() {}
🧹 Nitpick comments (4)
ledger/conway/conway.go (1)
58-60: Metadata model update acknowledged; ensure CBOR optional field encodingThe switch to common.TransactionMetadataSet and appending the value directly is good. As with other eras, please verify whether the 4th element should be omitted when metadata is absent instead of appending nil, to preserve canonical CBOR and hashes.
@@ func (t *ConwayTransaction) Cbor() []byte { - if t.TxMetadata != nil { - tmpObj = append(tmpObj, t.TxMetadata) - } else { - tmpObj = append(tmpObj, nil) - } + if t.TxMetadata != nil { + tmpObj = append(tmpObj, t.TxMetadata) + }Also applies to: 521-522, 635-637, 701-703
ledger/shelley/shelley.go (1)
56-56: Type alignment looks good; consider standardizing key width.Using
map[uint]common.TransactionMetadataSetworks, but elsewhere labels useuint64. Consider switching tomap[uint64]...for consistency and to avoid platform‑dependentuintwidth.ledger/allegra/allegra.go (1)
52-53: Type alignment looks good; consider standardizing key width.
map[uint]common.TransactionMetadataSetworks; aligning onuint64across packages would remove platform‐dependent width.ledger/mary/mary.go (1)
55-56: Type alignment looks good; consider key width consistency.As with other eras, consider
map[uint64]...for cross‑platform consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ledger/allegra/allegra.go(4 hunks)ledger/allegra/block_test.go(1 hunks)ledger/alonzo/alonzo.go(4 hunks)ledger/babbage/babbage.go(4 hunks)ledger/byron/byron.go(2 hunks)ledger/common/tx.go(3 hunks)ledger/conway/conway.go(4 hunks)ledger/mary/mary.go(4 hunks)ledger/shelley/shelley.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
ledger/byron/byron.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/conway/conway.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/conway.go (1)
ConwayTransaction(26-26)
ledger/common/tx.go (3)
cbor/decode.go (1)
Decode(29-44)cbor/cbor.go (1)
RawMessage(36-36)cbor/encode.go (1)
Encode(27-40)
ledger/allegra/block_test.go (1)
ledger/allegra/allegra.go (2)
AllegraBlock(46-53)AllegraBlock(66-68)
ledger/mary/mary.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/shelley/shelley.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/shelley.go (1)
ShelleyTransaction(26-26)
ledger/alonzo/alonzo.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
ledger/allegra/allegra.go (2)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)ledger/allegra.go (1)
AllegraTransaction(25-25)
ledger/babbage/babbage.go (1)
ledger/common/tx.go (1)
TransactionMetadataSet(41-41)
🔇 Additional comments (12)
ledger/alonzo/alonzo.go (1)
58-60: Verify canonical metadata encoding against Cardano specificationThe review comment assumes "canonical transaction encoding" should omit nil metadata rather than append it. However, CBOR's canonical/deterministic rules (RFC 8949) do not mandate omitting or appending (encoding) absent/null values — that is an application- or codec-level decision.
The current implementation (appending nil) is systematic across all era files (shelley, mary, conway, babbage, alonzo, allegra at lines 714, 416, 704, 919, 770, 416 respectively), suggesting this is an intentional design choice rather than an oversight. Before refactoring, confirm that Cardano's canonical transaction encoding specification actually requires omission over nil appending. Changing the serialization format could break compatibility with existing encoded transactions if the current behavior is what the protocol expects.
ledger/babbage/babbage.go (1)
58-60: Fix CBOR encoding to omit optional metadata field instead of appending nullThe current implementation at line 919 appends
nilwhen metadata is absent, but Cardano requires omitting optional metadata fields rather than encoding them as null. This breaks canonical CBOR encoding. Apply the proposed fix:if t.TxMetadata != nil { tmpObj = append(tmpObj, t.TxMetadata) -} else { - tmpObj = append(tmpObj, nil) }Likely an incorrect or invalid review comment.
ledger/byron/byron.go (1)
145-146: Remove LazyValue concern; verify semantic mapping with Byron test dataThe LazyValue semantics concern is unfounded—ByronTransaction.Attributes is directly typed as common.TransactionMetadataSet, not lazily evaluated. However, the core concern remains valid: there is no decode/round-trip test to verify that Byron transaction attributes correctly map to the TransactionMetadataSet semantics expected by the common Transaction interface. Byron's historical "attributes" field may have different constraints than Shelley-era aux metadata, and this mapping should be validated with real Byron on-chain data.
ledger/shelley/shelley.go (3)
543-544: Good: switch from LazyValue to typed metadata set.Replacing
*cbor.LazyValuewithcommon.TransactionMetadataSetimproves type safety and enables proper encode/decode.
653-655: Accessor is consistent with new interface.Returning
common.TransactionMetadataSetmatches the updatedcommon.Transactioninterface.
711-713: CBOR path: appending the typed metadata is correct.Appending
t.TxMetadatarelies on itsMarshalCBOR()— this is the right direction and avoids double-encoding. No issues here.ledger/allegra/allegra.go (3)
242-243: Typed metadata set adoption LGTM.
352-354: Accessor matches interface change.
413-415: CBOR encoding path is correct.Using the value directly ensures
MarshalCBOR()is honored.ledger/mary/mary.go (3)
250-251: Typed metadata set adoption LGTM.
364-366: Accessor matches interface change.
413-415: CBOR encoding path is correct.
ledger/common/tx.go
Outdated
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | ||
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | ||
| // Trying to decode as int64 | ||
| { | ||
| var v int64 | ||
| if _, err := cbor.Decode(b, &v); err == nil { | ||
| return MetaInt{Value: v}, nil | ||
| } | ||
| } | ||
| // Trying to decode as string | ||
| { | ||
| var s string | ||
| if _, err := cbor.Decode(b, &s); err == nil { | ||
| return MetaText{Value: s}, nil | ||
| } | ||
| } | ||
| // Trying to decode as []bytes | ||
| { | ||
| var bs []byte | ||
| if _, err := cbor.Decode(b, &bs); err == nil { | ||
| return MetaBytes{Value: bs}, nil | ||
| } | ||
| } | ||
| // Trying to decode as cbor.RawMessage first then recursively decode each value | ||
| { | ||
| var arr []cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &arr); err == nil { | ||
| items := make([]TransactionMetadatum, 0, len(arr)) | ||
| for _, it := range arr { | ||
| md, err := DecodeMetadatumRaw(it) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode list item: %w", err) | ||
| } | ||
| items = append(items, md) | ||
| } | ||
| return MetaList{Items: items}, nil | ||
| } | ||
| } | ||
| // Trying to decode as map[uint64]cbor.RawMessage first. | ||
| // Next trying to decode key as MetaInt and value as MetaMap | ||
| { | ||
| var m map[uint64]cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | ||
| pairs := make([]MetaPair, 0, len(m)) | ||
| for k, rv := range m { | ||
| val, err := DecodeMetadatumRaw(rv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | ||
| } | ||
| if k > math.MaxInt64 { | ||
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | ||
| } | ||
| pairs = append(pairs, MetaPair{ | ||
| Key: MetaInt{Value: int64(k)}, | ||
| Value: val, | ||
| }) | ||
| } | ||
| return MetaMap{Pairs: pairs}, nil | ||
| } | ||
| } | ||
| // Trying to decode as map[string]cbor.RawMessage first. | ||
| // Next trying to decode key as MetaText and value as MetaMap | ||
| { | ||
| var m map[string]cbor.RawMessage | ||
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | ||
| pairs := make([]MetaPair, 0, len(m)) | ||
| for k, rv := range m { | ||
| val, err := DecodeMetadatumRaw(rv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode map(text) value: %w", err) | ||
| } | ||
| pairs = append(pairs, MetaPair{ | ||
| Key: MetaText{Value: k}, | ||
| Value: val, | ||
| }) | ||
| } | ||
| return MetaMap{Pairs: pairs}, nil | ||
| } | ||
| } | ||
|
|
||
| return nil, errors.New("unsupported metadatum shape") | ||
| } |
There was a problem hiding this comment.
Map decoding misses valid shapes (negative int keys, byte-string keys).
CDDL allows metadatum map keys to be any metadatum (int — incl. negative, bytes, text). Current decoding only handles map[uint64]... and map[string].... Negative int keys won’t decode; byte-string keys can’t decode at all. This breaks spec compliance for some on-chain metadata.
Minimal improvement (adds negative int keys support):
@@
- // Trying to decode as map[uint64]cbor.RawMessage first.
+ // Trying to decode as map[uint64]cbor.RawMessage first.
@@
if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 {
@@
return MetaMap{Pairs: pairs}, nil
}
}
+ // Trying to decode as map[int64]cbor.RawMessage to support negative integer keys.
+ {
+ var m map[int64]cbor.RawMessage
+ if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 {
+ pairs := make([]MetaPair, 0, len(m))
+ for k, rv := range m {
+ val, err := DecodeMetadatumRaw(rv)
+ if err != nil {
+ return nil, fmt.Errorf("decode map(int) value: %w", err)
+ }
+ pairs = append(pairs, MetaPair{
+ Key: MetaInt{Value: k},
+ Value: val,
+ })
+ }
+ return MetaMap{Pairs: pairs}, nil
+ }
+ }
// Trying to decode as map[string]cbor.RawMessage first.For byte-string keys, decoding via map[[]byte]... isn’t possible in Go (non-comparable key). You likely need a streaming decode using fxamacker’s decoder to iterate map pairs as cbor.RawMessage and recursively decode key/value. Recommend a follow-up change to add that path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | |
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | |
| // Trying to decode as int64 | |
| { | |
| var v int64 | |
| if _, err := cbor.Decode(b, &v); err == nil { | |
| return MetaInt{Value: v}, nil | |
| } | |
| } | |
| // Trying to decode as string | |
| { | |
| var s string | |
| if _, err := cbor.Decode(b, &s); err == nil { | |
| return MetaText{Value: s}, nil | |
| } | |
| } | |
| // Trying to decode as []bytes | |
| { | |
| var bs []byte | |
| if _, err := cbor.Decode(b, &bs); err == nil { | |
| return MetaBytes{Value: bs}, nil | |
| } | |
| } | |
| // Trying to decode as cbor.RawMessage first then recursively decode each value | |
| { | |
| var arr []cbor.RawMessage | |
| if _, err := cbor.Decode(b, &arr); err == nil { | |
| items := make([]TransactionMetadatum, 0, len(arr)) | |
| for _, it := range arr { | |
| md, err := DecodeMetadatumRaw(it) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode list item: %w", err) | |
| } | |
| items = append(items, md) | |
| } | |
| return MetaList{Items: items}, nil | |
| } | |
| } | |
| // Trying to decode as map[uint64]cbor.RawMessage first. | |
| // Next trying to decode key as MetaInt and value as MetaMap | |
| { | |
| var m map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | |
| } | |
| if k > math.MaxInt64 { | |
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: int64(k)}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[string]cbor.RawMessage first. | |
| // Next trying to decode key as MetaText and value as MetaMap | |
| { | |
| var m map[string]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(text) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaText{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| return nil, errors.New("unsupported metadatum shape") | |
| } | |
| // Tries Decoding CBOR into all TransactionMetadatum variants (int, text, bytes, list, map). | |
| func DecodeMetadatumRaw(b []byte) (TransactionMetadatum, error) { | |
| // Trying to decode as int64 | |
| { | |
| var v int64 | |
| if _, err := cbor.Decode(b, &v); err == nil { | |
| return MetaInt{Value: v}, nil | |
| } | |
| } | |
| // Trying to decode as string | |
| { | |
| var s string | |
| if _, err := cbor.Decode(b, &s); err == nil { | |
| return MetaText{Value: s}, nil | |
| } | |
| } | |
| // Trying to decode as []bytes | |
| { | |
| var bs []byte | |
| if _, err := cbor.Decode(b, &bs); err == nil { | |
| return MetaBytes{Value: bs}, nil | |
| } | |
| } | |
| // Trying to decode as cbor.RawMessage first then recursively decode each value | |
| { | |
| var arr []cbor.RawMessage | |
| if _, err := cbor.Decode(b, &arr); err == nil { | |
| items := make([]TransactionMetadatum, 0, len(arr)) | |
| for _, it := range arr { | |
| md, err := DecodeMetadatumRaw(it) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode list item: %w", err) | |
| } | |
| items = append(items, md) | |
| } | |
| return MetaList{Items: items}, nil | |
| } | |
| } | |
| // Trying to decode as map[uint64]cbor.RawMessage first. | |
| // Next trying to decode key as MetaInt and value as MetaMap | |
| { | |
| var m map[uint64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(uint) value: %w", err) | |
| } | |
| if k > math.MaxInt64 { | |
| return nil, fmt.Errorf("metadata label %d exceeds int64", k) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: int64(k)}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[int64]cbor.RawMessage to support negative integer keys. | |
| { | |
| var m map[int64]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(int) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaInt{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| // Trying to decode as map[string]cbor.RawMessage first. | |
| // Next trying to decode key as MetaText and value as MetaMap | |
| { | |
| var m map[string]cbor.RawMessage | |
| if _, err := cbor.Decode(b, &m); err == nil && len(m) > 0 { | |
| pairs := make([]MetaPair, 0, len(m)) | |
| for k, rv := range m { | |
| val, err := DecodeMetadatumRaw(rv) | |
| if err != nil { | |
| return nil, fmt.Errorf("decode map(text) value: %w", err) | |
| } | |
| pairs = append(pairs, MetaPair{ | |
| Key: MetaText{Value: k}, | |
| Value: val, | |
| }) | |
| } | |
| return MetaMap{Pairs: pairs}, nil | |
| } | |
| } | |
| return nil, errors.New("unsupported metadatum shape") | |
| } |
🤖 Prompt for AI Agents
In ledger/common/tx.go around lines 308 to 389, the current map decoding only
handles map[uint64] and map[string], missing negative integer keys and
byte-string keys; update the decode logic to also try decoding map[int64] (or
map[int64]cbor.RawMessage) to capture negative keys and convert them to MetaInt,
and add a path that uses a streaming decoder (fxamacker/cbor.Decoder) to iterate
raw map key/value pairs as cbor.RawMessage so you can recursively
DecodeMetadatumRaw on both key and value (this avoids using non-comparable
[]byte as Go map keys); return MetaMap with pairs built from the recursively
decoded key/value results and preserve existing error handling for decoding
failures.
ledger/common/tx.go
Outdated
| // converting typed metadatum back into regular go values where the CBOR library can encode | ||
| func metadatumToInterface(m TransactionMetadatum) any { | ||
| switch t := m.(type) { | ||
| case MetaInt: | ||
| return t.Value | ||
| case MetaBytes: | ||
| return []byte(t.Value) | ||
| case MetaText: | ||
| return t.Value | ||
| case MetaList: | ||
| out := make([]any, 0, len(t.Items)) | ||
| for _, it := range t.Items { | ||
| out = append(out, metadatumToInterface(it)) | ||
| } | ||
| return out | ||
| case MetaMap: | ||
| mm := make(map[any]any, len(t.Pairs)) | ||
| for _, p := range t.Pairs { | ||
| mm[metadatumToInterface(p.Key)] = metadatumToInterface(p.Value) | ||
| } | ||
| return mm | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Encoding maps via map[any]any can panic for byte‑string keys.
In metadatumToInterface, MetaMap builds map[any]any. If any key is MetaBytes, the resulting []byte key is not comparable and will panic at assignment. Also mixed key types could lead to undesirable encodings.
Short‑term mitigation: only build native maps when all keys are text or all keys are integers; otherwise, return an error via a custom MarshalCBOR for MetaMap that encodes pairs using the CBOR encoder directly.
Example mitigation:
func metadatumToInterface(m TransactionMetadatum) any {
switch t := m.(type) {
@@
case MetaMap:
- mm := make(map[any]any, len(t.Pairs))
- for _, p := range t.Pairs {
- mm[metadatumToInterface(p.Key)] = metadatumToInterface(p.Value)
- }
- return mm
+ // Try all-text keys
+ allText := true
+ for _, p := range t.Pairs {
+ if _, ok := p.Key.(MetaText); !ok {
+ allText = false
+ break
+ }
+ }
+ if allText {
+ mm := make(map[string]any, len(t.Pairs))
+ for _, p := range t.Pairs {
+ mm[p.Key.(MetaText).Value] = metadatumToInterface(p.Value)
+ }
+ return mm
+ }
+ // Try all-int keys
+ allInt := true
+ for _, p := range t.Pairs {
+ if _, ok := p.Key.(MetaInt); !ok {
+ allInt = false
+ break
+ }
+ }
+ if allInt {
+ mm := make(map[int64]any, len(t.Pairs))
+ for _, p := range t.Pairs {
+ mm[p.Key.(MetaInt).Value] = metadatumToInterface(p.Value)
+ }
+ return mm
+ }
+ // Fallback is handled by a dedicated MetaMap.MarshalCBOR implementation (recommended).
+ return nil
default:
return nil
}
}Recommended follow‑up: implement func (m MetaMap) MarshalCBOR() ([]byte, error) to encode map pairs via the CBOR encoder (so byte‑string keys are supported without using Go map keys).
🤖 Prompt for AI Agents
In ledger/common/tx.go around lines 478–502, the MetaMap branch currently builds
map[any]any which can panic if a key converts to []byte (non‑comparable) or mix
types; change it to first scan all pairs and detect key kinds: if every key is
MetaText convert to map[string]any, if every key is MetaInt convert to
map[int64]any (or appropriate int type) and populate with
metadatumToInterface(values); otherwise do NOT build a Go map — return the
original MetaMap value (so it can be encoded via a custom encoder) and implement
MetaMap.MarshalCBOR() to encode pairs directly with the CBOR encoder (handling
byte-string keys safely). Ensure metadatumToInterface only returns native maps
when keys are homogeneous text or integer types to avoid panics.
Signed-off-by: Akhil Repala <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ledger/alonzo/alonzo.go (1)
763-769: Metadata CBOR encoding regressedAppending
t.TxMetadata(aMeta*struct) directly intotmpObjmeans the CBOR encoder now serializes the Go struct itself (producing a map with a"Value"field) instead of the canonical metadata item we used to append viaRawMessage. This breaks round-trips and yields wire formats that no longer match the on-chain CDDL. Please restore proper encoding by marshaling the metadatum to its raw CBOR form (e.g., via a dedicatedMarshalCBORon the metadata types or a helper that converts to the underlying CBOR primitives) before appending.ledger/shelley/shelley.go (1)
708-713: Metadata CBOR encoding regressed
ShelleyTransaction.Cbor()now feeds theMeta*structs straight into the encoder, so the wire output becomes a struct-encoded map (with"Value"keys, etc.) rather than the intended metadata CBOR. Please marshal the metadata to its canonical CBOR representation (or wrap it as aRawMessage) before appending so encoding stays compliant.ledger/allegra/allegra.go (1)
409-415: Metadata CBOR encoding regressedHere too we append the metadata interface value directly, so CBOR encoding emits Go-struct semantics instead of the metadata item defined in the CDDL. Please marshal the metadatum to raw CBOR (or convert it to primitive CBOR values) before appending to keep Allegra transactions encoding correctly.
ledger/mary/mary.go (1)
409-415: Metadata CBOR encoding regressedBy passing the
Meta*struct straight tocbor.Encode, we emit struct-shaped CBOR rather than the intended metadata payload. Please marshal the metadata value to its canonical CBOR (or wrap it in aRawMessage) before appending so Mary transactions remain wire-compatible.
🧹 Nitpick comments (1)
ledger/common/metadata_test.go (1)
12-32: Round-trip test validates CBOR fidelity correctly.The test properly validates byte-identical encoding without relying on stored CBOR, consistent with the repository's design goals. Using real on-chain metadata is excellent for ensuring spec compliance.
Consider adding more test cases to cover diverse metadata structures.
The PR objectives mention "decoding tests using metadata extracted from several on-chain transactions." Consider adding test cases that cover:
- Different metadata types (int, bytes, text, lists, maps)
- Nested structures
- Edge cases (empty maps, single-element lists, etc.)
- Samples from other eras (Shelley, Mary, Alonzo, Babbage, Conway)
Example structure:
func Test_Metadata_RoundTrip_Samples(t *testing.T) { testCases := []struct{ name string hex string }{ {"AllegraSample", allegraBlockHex}, {"ShellySample", shelleyBlockHex}, // ... more cases } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // ... test logic }) } }Optionally verify decoded structure in addition to round-trip.
While byte-identical round-trips prove encoding correctness, you could also add assertions on the decoded structure to document expected metadata shapes:
// After decode if len(set) == 0 { t.Fatal("expected non-empty metadata set") } // Verify specific labels/types are present if _, ok := set[0x19ef64]; !ok { t.Error("expected label 0x19ef64 in metadata") }This aids debugging and serves as documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ledger/allegra/allegra.go(4 hunks)ledger/alonzo/alonzo.go(4 hunks)ledger/babbage/babbage.go(4 hunks)ledger/byron/byron.go(2 hunks)ledger/common/metadata.go(1 hunks)ledger/common/metadata_test.go(1 hunks)ledger/common/tx.go(1 hunks)ledger/conway/conway.go(4 hunks)ledger/mary/mary.go(4 hunks)ledger/shelley/shelley.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
ledger/common/tx.goledger/common/metadata.goledger/common/metadata_test.go
🧬 Code graph analysis (10)
ledger/common/tx.go (1)
ledger/common/metadata.go (1)
TransactionMetadatum(40-43)
ledger/byron/byron.go (2)
ledger/common/metadata.go (1)
TransactionMetadatum(40-43)ledger/byron.go (1)
ByronTransaction(28-28)
ledger/babbage/babbage.go (1)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)
ledger/shelley/shelley.go (1)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)
ledger/alonzo/alonzo.go (1)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)
ledger/mary/mary.go (1)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)
ledger/conway/conway.go (2)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)ledger/conway.go (1)
ConwayTransaction(26-26)
ledger/common/metadata.go (4)
cbor/decode.go (1)
Decode(29-44)cbor/cbor.go (1)
RawMessage(36-36)cbor/bytestring.go (2)
ByteString(27-29)NewByteString(31-34)cbor/encode.go (1)
Encode(27-40)
ledger/common/metadata_test.go (2)
ledger/common/metadata.go (1)
TransactionMetadataSet(38-38)cbor/decode.go (1)
Decode(29-44)
ledger/allegra/allegra.go (1)
ledger/common/metadata.go (2)
TransactionMetadataSet(38-38)TransactionMetadatum(40-43)
🪛 GitHub Actions: go-test
ledger/alonzo/alonzo.go
[error] 106-106: cannot use b.TransactionMetadataSet (variable of map type "github.com/blinklabs-io/gouroboros/ledger/common".TransactionMetadataSet) as map[uint]*"github.com/blinklabs-io/gouroboros/cbor".LazyValue value in struct literal
🪛 GitHub Actions: golangci-lint
ledger/alonzo/alonzo.go
[error] 106-106: cannot use b.TransactionMetadataSet (type map["github.com/blinklabs-io/gouroboros/ledger/common".TransactionMetadataSet) as map[uint]*"github.com/blinklabs-io/gouroboros/cbor".LazyValue value in struct literal (typecheck)
🪛 GitHub Actions: nilaway
ledger/alonzo/alonzo.go
[error] 106-106: cannot use b.TransactionMetadataSet (type map["github.com/blinklabs-io/gouroboros/ledger/common".TransactionMetadataSet) as map[uint]*"github.com/blinklabs-io/gouroboros/cbor".LazyValue in struct literal. Lint/compile error encountered during 'nilaway ./...' step.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
ledger/common/tx.go (1)
31-31: Clean interface update to support structured metadata.The return type change from
*cbor.LazyValuetoTransactionMetadatumprovides a typed API for transaction metadata, replacing the raw CBOR approach. This is consistent with implementations across all era modules in this PR.ledger/byron/byron.go (2)
145-145: LGTM! Field type updated consistently.The change from
*cbor.LazyValuetocommon.TransactionMetadatumaligns with the new typed metadata model.
278-280: LGTM! Method signature matches interface.The
Metadata()implementation correctly returns theTransactionMetadatuminterface value.ledger/conway/conway.go (4)
58-58: LGTM! Block-level metadata set correctly typed.The change from
map[uint]*cbor.LazyValuetocommon.TransactionMetadataSetprovides structured metadata handling at the block level.
521-521: LGTM! Transaction metadata field updated correctly.The field now uses the typed
TransactionMetadatuminterface instead of raw CBOR.
635-637: LGTM! Method signature matches interface.The
Metadata()implementation correctly returnsTransactionMetadatum.
684-712: LGTM! CBOR encoding correctly delegates to metadata's marshaler.The change from wrapping metadata in
cbor.RawMessage(t.TxMetadata.Cbor())to appendingt.TxMetadatadirectly is correct. This delegates encoding to the metadata type'sMarshalCBORimplementation, enabling proper round-trip encoding. The consistent append behavior (always including the field, even if nil) aligns with CBOR array structure requirements.ledger/babbage/babbage.go (4)
58-58: LGTM! Block-level metadata set correctly typed.Consistent with the Conway changes, this provides structured metadata handling at the block level.
736-736: LGTM! Transaction metadata field updated correctly.The field type change is consistent with the new metadata model.
850-852: LGTM! Method signature matches interface.The
Metadata()implementation is correct and consistent across eras.
899-927: LGTM! CBOR encoding pattern consistent with Conway.The encoding correctly delegates to the metadata's
MarshalCBORimplementation and consistently includes the metadata field (even if nil) in the transaction array structure.
| type MetaInt struct{ Value int64 } | ||
| type MetaBytes struct{ Value []byte } | ||
| type MetaText struct{ Value string } | ||
| type MetaList struct { | ||
| Items []TransactionMetadatum | ||
| } | ||
| type MetaPair struct { | ||
| Key TransactionMetadatum | ||
| Value TransactionMetadatum | ||
| } | ||
| type MetaMap struct { |
There was a problem hiding this comment.
MetaInt cannot represent on-chain metadata range
MetaInt.Value int64 caps us at ±9.22e18. The metadata CDDL allows arbitrary CBOR ints (and labels are full uint64). As written, decodeMapUint now rejects any key > math.MaxInt64, so valid metadata fails to decode. Please store integers in an unbounded form (e.g., *big.Int) or at least handle the full uint64/signed range so we don’t reject legitimate metadata.
| case cborTypeTag, cborTypeFloatSim: | ||
| var x any | ||
| if _, err := cbor.Decode(b, &x); err != nil { | ||
| return nil, err | ||
| } | ||
| return MetaText{Value: fmt.Sprintf("%v", x)}, nil |
There was a problem hiding this comment.
Do not coerce unsupported CBOR major types into text
For major type 7 you’re calling fmt.Sprintf and returning MetaText. That silently reinterprets floats, bools, and nulls as strings instead of flagging them as unsupported per the metadata CDDL. We should emit an error the moment we hit a non-metadatum major type; otherwise invalid metadata will be accepted and misrepresented.
🤖 Prompt for AI Agents
In ledger/common/metadata.go around lines 124 to 129, the code decodes CBOR
major type 7 into an interface and returns MetaText via fmt.Sprintf; instead,
stop coercing unsupported major types into text and return an explicit error
when encountering cborTypeTag or cborTypeFloatSim. Replace the decode-and-printf
branch with logic that returns a clear error (including the encountered major
type or byte) indicating non-metadatum CBOR major types are unsupported per the
metadata CDDL so invalid metadata is rejected rather than misrepresented.
| tmpMap[k] = metadatumToInterface(v) | ||
| } | ||
| return cbor.Encode(&tmpMap) | ||
| } | ||
|
|
||
| func metadatumToInterface(m TransactionMetadatum) any { | ||
| switch t := m.(type) { | ||
| case MetaInt: | ||
| return t.Value | ||
| case MetaBytes: | ||
| return t.Value | ||
| case MetaText: | ||
| return t.Value | ||
| case MetaList: | ||
| out := make([]any, 0, len(t.Items)) | ||
| for _, it := range t.Items { | ||
| out = append(out, metadatumToInterface(it)) | ||
| } | ||
| return out | ||
| case MetaMap: | ||
| allText := true | ||
| for _, p := range t.Pairs { | ||
| if _, ok := p.Key.(MetaText); !ok { | ||
| allText = false | ||
| break | ||
| } | ||
| } | ||
| if allText { | ||
| mm := make(map[string]any, len(t.Pairs)) | ||
| for _, p := range t.Pairs { | ||
| mm[p.Key.(MetaText).Value] = metadatumToInterface(p.Value) | ||
| } | ||
| return mm | ||
| } | ||
| // Try all-int keys | ||
| allInt := true | ||
| for _, p := range t.Pairs { | ||
| if _, ok := p.Key.(MetaInt); !ok { | ||
| allInt = false | ||
| break | ||
| } | ||
| } | ||
| if allInt { | ||
| mm := make(map[int64]any, len(t.Pairs)) | ||
| for _, p := range t.Pairs { | ||
| mm[p.Key.(MetaInt).Value] = metadatumToInterface(p.Value) | ||
| } | ||
| return mm | ||
| } | ||
|
|
||
| allBytes := true | ||
| for _, p := range t.Pairs { | ||
| if _, ok := p.Key.(MetaBytes); !ok { | ||
| allBytes = false | ||
| break | ||
| } | ||
| } | ||
| if allBytes { | ||
| mm := make(map[cbor.ByteString]any, len(t.Pairs)) | ||
| for _, p := range t.Pairs { | ||
| bs := p.Key.(MetaBytes).Value | ||
| key := cbor.NewByteString(bs) | ||
| mm[key] = metadatumToInterface(p.Value) | ||
| } | ||
| return mm | ||
| } | ||
| return nil |
There was a problem hiding this comment.
metadatumToInterface drops mixed-key maps
When a metadata map has keys of varying types (allowed by the spec), this helper falls through the text/int/bytes branches and returns nil, so MarshalCBOR encodes the value as CBOR null—permanently losing the map. Please preserve such maps (e.g., encode as an array of key/value pairs or a generic slice) instead of collapsing them to nil.
🤖 Prompt for AI Agents
In ledger/common/metadata.go around lines 239 to 305, the metadatumToInterface
function currently returns nil for maps with mixed key types which causes
MarshalCBOR to encode them as CBOR null; instead detect the mixed-key case and
preserve the map by returning a generic slice-of-pairs representation: build and
return a []any where each element is a two-element []any {
metadatumToInterface(p.Key), metadatumToInterface(p.Value) } (preserving order),
replacing the final "return nil" with this constructed slice so mixed-type-key
maps are retained during CBOR encoding.
Closes #1136
Summary by CodeRabbit
New Features
Refactor
Tests