feat: add toindefarray struct tag option for CBOR indefinite-length arrays#770
Conversation
982d383 to
e1e8080
Compare
There was a problem hiding this comment.
Thanks for opening this PR with a detailed description and many tests.
I left some suggestions focused on reducing code duplication and preventing future code divergence/inconsistency.
The separate code path is a good approach to reduce coupling and it helps avoid modifying existing functions.
But in this case, the duplicate code is almost entirely the same. So I think we can refactor some existing functions to incorporate a few lines of differences, which will help us reuse code in a single path instead of maintaining mostly identical blocks used by two paths.
e1e8080 to
d589f29
Compare
|
Thanks for the careful review — you're right that the duplicate code was a maintenance trap waiting to happen. All suggestions applied in d589f29: Refactor (code dedup)
Tests
Net diff: -69 lines (the refactor removed more than it added). Coverage held at 97.2% (up from 97.0% before refactor — the merged path is now exercised by both |
fxamacker
left a comment
There was a problem hiding this comment.
Thanks for updating this PR. It looks good. I left some minor suggestions.
- decouple
encodingStructType.toArrayandencodingStructType.toIndefArray - remove redundant hex comments in tests
- add roundtrip decoding in new tests when they are feasible
BTW, there are some conflicts with main branch that need to be resolved.
Thanks again for opening this PR and the updates!
| fields: encFlds, | ||
| toArray: true, | ||
| toIndefArray: toIndefArray, |
There was a problem hiding this comment.
Maybe add toArray function parameter to getEncodingStructToArrayType(), and only set toArray to true if it is actually specified by the user.
| fields: encFlds, | |
| toArray: true, | |
| toIndefArray: toIndefArray, | |
| fields: encFlds, | |
| toArray: toArray, | |
| toIndefArray: toIndefArray, |
There was a problem hiding this comment.
Done in 988e0fd — getEncodingStructToArrayType() now takes toArray, toIndefArray bool and each flag reflects the user's literal struct tag input. Updated the field comments on encodingStructType accordingly. Note: isEmptyStruct at encode.go:2209 still uses structType.toArray only; under the new semantics a toindefarray-only struct now falls through to the map-shaped path there, which happens to give the same answer because array-shaped structs never populate omitEmptyFieldsIdx. Let me know if you'd prefer that check to be toArray || toIndefArray for explicitness.
There was a problem hiding this comment.
Good catch! 👍 Yes, please update isEmptyStruct() to check for structType.toArray || structType.toIndefArray for the fast path.
Even though the current code produces the correct CBOR data, updating the check is more future-proof.
There was a problem hiding this comment.
Done in 7b123cf — isEmptyStruct() now checks structType.toArray || structType.toIndefArray for the fast path.
| omitEmptyFieldsIdx []int // Only populated if toArray is false | ||
| err error | ||
| toArray bool | ||
| toIndefArray bool // Only set when toArray is also true. |
There was a problem hiding this comment.
We can decouple encodingStructType.toArray and encodingStructType.toIndefArray here because these two options are mutually exclusive.
Decoupling will be more consistent with the user-specified struct tag options:
encodingStructType.toArrayis true only iftoarrayis specified in struct tag;encodingStructType.toIndefArrayis true only iftoindefarrayis specified in struct tag.
| toIndefArray bool // Only set when toArray is also true. | |
| toIndefArray bool // Only set when toindefarray is specified. |
There was a problem hiding this comment.
Decoupled in 988e0fd — the field comments now state toArray and toIndefArray independently reflect the user-specified option:\n\ngo\ntoArray bool // True iff the struct declares the `toarray` option\ntoIndefArray bool // True iff the struct declares the `toindefarray` option\n
988e0fd to
851cf6c
Compare
fxamacker
left a comment
There was a problem hiding this comment.
Thank you for rebasing and updating the PR. I just left one comment to reply about the isEmptyStruct() check that you identified and suggested. 👍
| fields: encFlds, | ||
| toArray: true, | ||
| toIndefArray: toIndefArray, |
There was a problem hiding this comment.
Good catch! 👍 Yes, please update isEmptyStruct() to check for structType.toArray || structType.toIndefArray for the fast path.
Even though the current code produces the correct CBOR data, updating the check is more future-proof.
Encodes a Go struct as a CBOR indefinite-length array (0x9f ... 0xff) when tagged `cbor:",toindefarray"`. Gated by EncOptions.ToIndefArrayStructTag, default ToIndefArrayStructTagForbidden. Mutually exclusive with `toarray`. Decoder is unchanged. Motivated by Cardano Plutus Data interoperability; proposed in fxamacker#762. Signed-off-by: tdnguyenND <tdnguyen.uet@gmail.com>
851cf6c to
7b123cf
Compare
fxamacker
left a comment
There was a problem hiding this comment.
Nice! 👍 Thanks for contributing this PR!
Description
Adds the
toindefarraystruct tag option, gated by a newEncOptions.ToIndefArrayStructTagfield. When enabled, a tagged struct encodes as a CBOR indefinite-length array (head0x9f, break0xff) instead of a definite-length array. This was discussed and welcomed in issue #762.Motivation: Cardano Plutus Data interoperability. The reference
plutus-coredata encoder emitsConstrfield lists as indefinite-length arrays for non-empty payloads, and consumers in that ecosystem accept only that exact byte layout.Public API additions (encode-side only):
Struct usage:
Behavior contract:
ToIndefArrayStructTagForbiddenrejects encoding a struct tagged withtoindefarrayand returns a clear error pointing at the option name. Existing users see no change.ToIndefArrayStructTag = ToIndefArrayStructTagAllowedtogether withIndefLength = IndefLengthForbiddenis rejected atEncMode()time as a contradictory combination.toarrayandtoindefarrayon the same struct are mutually exclusive and rejected at type-cache time, on both encoder and decoder side, with the same error message.toindefarray-tagged structs (same as it does fortoarray).0x9f 0xff.(*Encoder).StartIndefiniteArray) continues to be governed byIndefLength.Files changed:
cache.go—hasToIndefArrayOption,getEncodingStructToIndefArrayType, mutual-exclusion check on both encoder and decoder cache, routetoindefarraythrough the existing array decode path.encode.go—ToIndefArrayStructTagModeenum,EncOptionsfield,encModefield, validation inencMode()(including the cross-option check againstIndefLengthForbidden),encodeStructToIndefArraywith runtime gate, dispatch ingetEncodeFuncInternal.toindefarray_test.go— new tests (see below).encode_test.go— extendedTestEncOptionsto skipToIndefArrayStructTagfor the non-zero invariant (its non-zero value conflicts withIndefLengthForbidden, mirroring the existing escape hatch forTagsMd).Test coverage:
TestEncodeStructToIndefArrayBasic— header/break/roundtrip on a typical struct.TestEncodeStructToIndefArrayEmpty— empty struct emits exactly0x9f 0xff.TestEncodeStructToIndefArrayNested— nestedtoindefarraystructs roundtrip.TestEncodeStructToIndefArrayEmbeddedField— fields promoted from an embedded struct are encoded in order.TestEncodeStructToIndefArrayParentIndefChildArray— parenttoindefarraycontaining childtoarrayproduces nested9f 82 ... ff.TestEncodeStructToIndefArrayParentArrayChildIndef— parenttoarraycontaining childtoindefarrayproduces nested82 9f ... ff ....TestEncodeStructToIndefArrayForbiddenByDefault— defaultEncOptions{}rejects with an error mentioningToIndefArrayStructTag.TestEncodeStructToIndefArrayMutuallyExclusive— struct with bothtoarrayandtoindefarrayis rejected.TestToIndefArrayStructTagOptionsRoundTrip— non-zero value of the option round-trips throughEncOptions -> EncMode -> EncOptions.TestInvalidToIndefArrayStructTagMode— out-of-range mode values are rejected byEncMode().TestToIndefArrayStructTagRejectsIndefLengthForbidden— combination withIndefLengthForbiddenis rejected atEncMode()time.TestEncodeStructToIndefArrayWithCBORTag— verifies a struct registered with a CBOR tag throughTagSetand taggedtoindefarrayproduces the tag header followed by an indefinite-length array.PR Was Proposed and Welcomed in Currently Open Issue
Checklist (for code PR only, ignore for docs PR)
go test ./...andgo test -race -short ./...; coverage held at 97.0%)(see next section).
Certify the Developer's Certificate of Origin 1.1
the Developer Certificate of Origin 1.1.