fix(dynamic-codecs): decode scalar enums as discriminated unions - #1029
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: 381fa42 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
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.
|
Would love @mikhd's input on that one since I expect this breaking change will shake a few things in the dynamic package. |
|
Done, brought the branch up to date with main. Two notes from the update: the display-side special case I had removed in format-argument-value.ts has since been superseded by the richer variant matching on main, so my change there reduces to the added tests; and the README enum row now carries the new spec links with the updated example shape. All dynamic package tests are green locally. |
|
@plutohan Unfortunately, lots of tests are failing still. |
…d unions
The default-value encoder passed raw variant names to the enum codec,
which the union codec rejects. Resolve them to the { __kind } shape and
update the input-transformer tests to the resolved expectations.
This reverts commit 4052ec2.
|
Sorry about that, my local run had missed the dynamic-address-resolution suite. Fixed in a3ca61f: the default-value encoder now resolves enum variants to the |
|
Hi guys @plutohan @lorisleiva , Currently dynamic-instructions validates arguments before building an instruction (encoding args, resolve accounts, etc). And I think one test case could fail - when we decode and then encode ix, e.g.: So I'm wondering if we should also align enums And also align enum matching in objects branch in codec-input-transformer.ts What do you think? Here are tests examples for better understanding of what I meant: import { createCodecInputTransformer } from '@codama/dynamic-address-resolution';
import { getNodeCodec } from '@codama/dynamic-codecs';
import type { InstructionNode } from 'codama';
import {
definedTypeLinkNode,
definedTypeNode,
enumEmptyVariantTypeNode,
enumStructVariantTypeNode,
enumTypeNode,
instructionArgumentNode,
instructionNode,
numberTypeNode,
programNode,
rootNode,
structFieldTypeNode,
structTypeNode,
} from 'codama';
import { describe, expect, test } from 'vitest';
import { createArgumentsInputValidator } from
'../../src/arguments/validate-arguments-input';
const PROGRAM_KEY = '11111111111111111111111111111111';
// the IDL declares `Arm | Bar | Car`, the node constructor camelCases them.
const seedEnum = definedTypeNode({
name: 'seedEnum',
type: enumTypeNode([
enumEmptyVariantTypeNode('Arm'),
enumEmptyVariantTypeNode('Bar'),
enumEmptyVariantTypeNode('Car'),
]),
});
// A mixed enum, to cover the payload-validator lookup.
const command = definedTypeNode({
name: 'command',
type: enumTypeNode([
enumEmptyVariantTypeNode('Quit'),
enumStructVariantTypeNode(
'Move',
structTypeNode([structFieldTypeNode({ name: 'x', type: numberTypeNode('u8')
})]),
),
]),
});
const ix: InstructionNode = instructionNode({
arguments: [
instructionArgumentNode({ name: 'seedEnum', type:
definedTypeLinkNode('seedEnum') }),
instructionArgumentNode({ name: 'command', type: definedTypeLinkNode('command')
}),
],
name: 'nestedExampleIx',
});
const root = rootNode(
programNode({ definedTypes: [seedEnum, command], instructions: [ix], name: 'test',
publicKey: PROGRAM_KEY }),
);
const validate = createArgumentsInputValidator(root, ix);
// Shapes that already work today, so each test below isolates exactly one failure.
const validSeed = 'arm';
const validCommand = { __kind: 'quit' };
describe('enum inputs accept the shape the codec decodes', () => {
test('what the codec decodes must be accepted back as input', () => {
const codec = getNodeCodec([root, root.program, ix, (ix.arguments ?? [])[0]]);
const decoded = codec.decode(new Uint8Array([0]));
expect(() => validate({ command: validCommand, seedEnum: decoded
})).not.toThrow();
});
test('validator accepts an empty variant as a PascalCase __kind object', () => {
expect(() => validate({ command: validCommand, seedEnum: { __kind: 'Arm' }
})).not.toThrow();
});
test('validator accepts an empty variant as a PascalCase bare name', () => {
expect(() => validate({ command: validCommand, seedEnum: 'Arm'
})).not.toThrow();
});
test('validator accepts a struct variant as a PascalCase __kind object', () => {
expect(() => validate({ command: { __kind: 'Move', x: 12 }, seedEnum: validSeed
})).not.toThrow();
});
test('validator still rejects an invalid payload under a PascalCase __kind', () => {
expect(() => validate({ command: { __kind: 'Move', x: 'oops' }, seedEnum:
validSeed })).toThrow(
/Enum variant "Move" has invalid "x"/,
);
});
test('validator still rejects an unknown variant', () => {
expect(() => validate({ command: validCommand, seedEnum: { __kind: 'Leg' }
})).toThrow(
/Invalid enum variant "Leg"/,
);
});
test('transformer resolves a PascalCase __kind object', () => {
const transform = createCodecInputTransformer(definedTypeLinkNode('seedEnum'),
root);
expect(transform({ __kind: 'Arm' })).toStrictEqual({ __kind: 'Arm' });
});
}); |
EnumVariantValidator and the input transformer's object branch matched __kind against raw node names, so decoded values failed the decode-then-build round trip. Both now match through pascalCase, with round-trip tests from the review discussion.
|
Good catch, the decode-then-build round trip was exactly the gap. Done in 381fa42:
I adapted your test cases into |
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Still approved. The changes since my last pass address the substantive points I raised and extend the normalization further downstream:
- Transformer object branch case-sensitivity — fixed.
codec-input-transformer.tsnow comparespascalCase(v.name) === pascalCase(String(__kind)), and the newenumTypeNode.test.tscase ({ __kind: 'arm' }→{ __kind: 'Arm' }) covers exactly the camelCase-IDL scenario I flagged. - Validator normalization is a welcome addition.
EnumVariantValidatorindynamic-instructionsnow keys the variant map by PascalCase, so'Arm',{ __kind: 'Arm' }, and the raw camelCase shapes all validate consistently — and the payload validators are looked up through the same normalization, so a PascalCase__kindwith an invalid payload still fails correctly. The newvalidate-arguments-input.test.tssuite covers all of these paths including the negative cases. default-value-encoder.tsnow emits the union shape for enum default values, which is required for the codec change to work. One informational flag inline aboutnode.valuepayloads.
Notes:
- The
format-argument-value.tssource change from the earlier revision is no longer in the diff (the display matcher onmainalready tolerates the__kindshapes via PascalCase comparison), but the added display tests remain and pin the new decoded shape — good. - Leftover from my previous review: the doc comment on
seedEnumVariantinnested-example-ix.test.ts(~L639) still reads "SeedEnum enum is stored as a number", which is now describing the old shape. It's on an unchanged line so I can't anchor there — trivial, but worth fixing while you're in the file. - Tiny changeset nit:
.changeset/plain-taxis-transform.mddescribes the display-label matching (which is no longer part of this PR) but not the validator normalization that now justifies thedynamic-instructionspatch bump. Consider rewording so the release notes match what actually shipped.
The wire-format invariant remains well covered by the dynamic-client SVM integration test, and the decode-symmetry test in EnumTypeNode.test.ts pins scalar and data enums to the same decoded shape for empty variants.
|
Thank you for your work on this and for your patience! 🙏 |
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