fix(dynamic-codecs): decode scalar enums as discriminated unions - #1029
fix(dynamic-codecs): decode scalar enums as discriminated unions#1029plutohan wants to merge 1 commit into
Conversation
Scalar enums went through getEnumCodec and decoded to number indices
while data enums decoded to { __kind } objects. All enums now use
getDiscriminatedUnionCodec, so empty variants decode the same way in
both. Wire bytes are unchanged.
The codec input transformer still accepts bare empty-variant names and
indices and resolves them to the union shape, so instruction argument
inputs keep working. Display label lookup matches the new shape.
🦋 Changeset detectedLatest commit: 79ed936 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Removes the scalar-enum special case in @codama/dynamic-codecs so every enum — scalar or data — flows through getDiscriminatedUnionCodec, producing the { __kind: 'PascalVariant' } shape on both encode and decode. Wire bytes are unchanged (both codec paths write the variant index with the same size prefix; empty variants encode via getUnitCodec → zero payload bytes). The follow-on cleanup drops the mirrored isScalarEnum branch in the enumValueNode visitor, updates the display-label matcher in dynamic-instructions to the new shape, and — to keep instruction call sites ergonomic — teaches the dynamic-address-resolution input transformer to accept bare variant names and numeric indices for empty variants, resolving them to { __kind }.
The changesets look right: @codama/dynamic-codecs is a minor with a bolded breaking note (matching the big-pens-make.md precedent), and the two dependent packages get patches. The decision to defer __discriminant until the wire format can honor variant.discriminator is the right call — advertising a field the codec doesn't respect would be worse than not having it.
What to watch for
- Roundtrip correctness on the wire. The dynamic-client integration test (
nested-example-ix.test.ts) exercises real send/decode against SVM with scalar enums nested inside structs, arrays, and data-enum variants, so the wire-format claim is well covered end-to-end. - Ergonomic input surface. The transformer accepts three input shapes for scalar enums now: bare string (
'arm'), bare index (0), and{ __kind: 'Arm' }. The first two hit the new empty-variant branch cleanly; the third goes through the pre-existing object branch, which has a subtle case-sensitivity issue worth calling out (see inline). enumValueNodeconsumers.visitEnumValueinvalues.tsis used indirectly bygetConstantCodec, sentinels, andzeroableOptionnoneValues. Any IDL that used a scalar-enum value node in one of those slots will now encode via the discriminated-union codec — which is consistent, since the codec side changed in lockstep, so the encoded bytes stay the same. Nothing to fix, just worth being aware of for subsequent reviewers.
Notes for subsequent reviewers
- The
pascalCase(v.name) === pascalCase(input)string lookup invisitEnumTypeis nicely tolerant of case variations. Worth confirming the transformer's object-input branch (unchanged in this PR) plays well with the newly-idiomatic{ __kind: 'Buy' }shape when variants are camelCase — see inline. - README table entry is updated; no other docs seem to reference the old scalar-enum shape.
| : (node.variants ?? []).find(v => pascalCase(v.name) === pascalCase(input)); | ||
| if (variantNode && isNode(variantNode, 'enumEmptyVariantTypeNode')) { | ||
| return { __kind: pascalCase(variantNode.name) }; | ||
| } |
There was a problem hiding this comment.
Nice touch supporting both numeric indices and bare names here, and using pascalCase on both sides so 'arm', 'Arm', 'ARM' all resolve.
One inconsistency worth flagging (pre-existing, but this PR makes it easier to hit): the object-input branch a few lines down still does variants.find(v => v.name === __kind) — a raw case-sensitive match. With this PR advertising { __kind: 'Buy' } as the canonical shape for scalar enums, users who follow the docs and pass { __kind: 'Arm' } against an IDL variant named arm will fall through to the CODAMA_ERROR__DYNAMIC_CLIENT__UNEXPECTED_ARGUMENT_TYPE throw, even though the same string via the bare-name branch works.
Could be aligned by matching on pascalCase(v.name) === pascalCase(String(__kind)) in the object branch too. Not a blocker — happy to punt to a follow-up if you'd rather keep this PR tight.
| } | ||
| return input; | ||
| } | ||
|
|
There was a problem hiding this comment.
Minor: an out-of-range numeric index (e.g. input === 99) silently falls through and returns the raw number, which then fails deeper in the codec with a less specific error. Same behavior as before this PR, but now that indices are a documented input shape for scalar enums, a targeted CODAMA_ERROR__DYNAMIC_CLIENT__UNEXPECTED_ARGUMENT_TYPE here with the valid variant list would give users a much cleaner message. Optional.
| }); | ||
|
|
||
| test('it decodes empty variants the same way in scalar and data enums', () => { | ||
| const scalar = getNodeCodec([enumTypeNode([enumEmptyVariantTypeNode('quit'), enumEmptyVariantTypeNode('stay')])]); |
There was a problem hiding this comment.
Good addition — this test locks in the key invariant ("empty variants decode identically whether the enum is scalar or mixed"), which is the whole point of the PR. Consider one more assertion for the mirror direction: expect(scalar.encode({ __kind: 'Quit' })).toStrictEqual(data.encode({ __kind: 'Quit' })) would nail down that encode is symmetric too.
| /** SeedEnum enum is stored as a number. */ | ||
| export function seedEnumToNumber(enumValue: string) { | ||
| switch (enumValue.toLowerCase()) { | ||
| case 'arm': | ||
| return 0; | ||
| case 'bar': | ||
| return 1; | ||
| case 'car': | ||
| return 2; | ||
| default: | ||
| throw new Error(`Unknown enum value: ${enumValue}`); | ||
| export function seedEnumVariant(enumValue: string) { | ||
| const kind = enumValue.charAt(0).toUpperCase() + enumValue.slice(1).toLowerCase(); | ||
| if (!['Arm', 'Bar', 'Car'].includes(kind)) { | ||
| throw new Error(`Unknown enum value: ${enumValue}`); | ||
| } | ||
| return { __kind: kind }; | ||
| } |
There was a problem hiding this comment.
The renamed helper is fine, but the leftover /** SeedEnum enum is stored as a number. */ doc comment on line 639 is now inaccurate — after this PR the helper produces the discriminated-union shape, not a number. Worth updating (e.g. "SeedEnum variants are represented as { __kind } objects at the codec boundary.") while you're touching this function.
|
Would love @mikhd's input on that one since I expect this breaking change will shake a few things in the dynamic package. |
3 participants
As discussed in #584, scalar enums took a different codec path (
getEnumCodec, decoding to number indices) than data enums (getDiscriminatedUnionCodec, decoding to{ __kind }objects). This removes the special case so every enum goes throughgetDiscriminatedUnionCodec, following @unek's suggestion.2{ __kind: 'Down' }2or'down'{ __kind: 'Down' }enumValueNoderesolution2{ __kind: 'Down' }Wire bytes are unchanged in every case; only the JavaScript value shape changes. To keep instruction inputs ergonomic, the codec input transformer in dynamic-address-resolution now resolves bare empty-variant names and indices to the union shape, so existing
encodeInstructionArgumentscall sites keep working (the dynamic-instructions argument tests pass unmodified). The display label lookup in dynamic-instructions matches the new decoded shape.On the open question of exposing the discriminant: I left it out of this PR on purpose. Kit's
getDiscriminatedUnionCodecwrites the array index on the wire and nothing currently readsvariant.discriminator, so a__discriminantfield here would advertise a value the codec does not actually honor precisely in the custom-discriminant case that motivates it. If you want, I can follow up with a PR that makes the wire format honorvariant.discriminator(viagetUnionCodecwith explicit index maps) and adds__discriminantto the decoded output there, where it becomes truthful, which would also cover the incrementalErrorLevelcomparison use case.Changesets:
@codama/dynamic-codecsminor with a bolded breaking note (following the.changeset/big-pens-make.mdprecedent), patches for the two dependent packages. Happy to bump to major instead if you prefer.Fixes #584