Repository navigation
[2.x] Adapt instruction visitors to Codama v2 - #1179
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Ports the last three instruction visitors of @codama/visitors to the v2 node model:
setInstructionAccountDefaultValuesVisitormoves from a hand-rolledextendVisitor+NodeStackto abottomUpTransformerVisitoron[instructionNode](so sub-instructions are covered for free), matches string rules exactly, generates camelCase + snake_case alternatives for the common rules, stops mutating the caller'srulesarray, and only swallowsINVALID_PDA_SEED_VALUESinstead of every error.setInstructionDiscriminatorsVisitornow branches on the data shape: inline struct/absent → prepend astructFieldTypeNode+fieldDiscriminatorNode; anything else → append ahiddenPrefixTransformNode+constantDiscriminatorNode. Existing discriminators are shifted by the byte size of the new one (computed withgetByteSizeVisitor), and a newCANNOT_SET_INSTRUCTION_DISCRIMINATOR(1200019) error covers the three failure modes.setNumberWrappersVisitorreplaces the v1Amount/DateTime/SolAmountwrappers with the seven v2 variants, targetsintegerTypeNode/floatTypeNodevia a|selector plus a path predicate that skips size/prefix/already-wrapped integers, and carriestransformsonto the wrapper node.
I verified the things that looked risky and they're all fine:
- The
{ ...node, ... }spreads passed as factory options (sizeDiscriminatorNode(size + n, { ...discriminator }),integerTypeNode(format, { ...number, transforms: undefined }),structTypeNode(fields, { ...node.data })) are safe — the generated factories pick named options explicitly and dropundefined, so the positional arg always wins and no stray keys leak in. NodeStack.getPath('instructionNode')in v2 asserts on the last node rather than truncating, soinstructionPathcorrectly ends at the sub-instruction when transforming one.fillDefaultPdaSeedValuesVisitorin strict mode only throwsINVALID_PDA_SEED_VALUES, so narrowing the catch to that code doesn't regress the "best-effort" bulk behaviour.bottomUpTransformerVisitorhands selector functions the stack of original nodes, so theparent.prefix === numberidentity check inisValueNumberis reliable.addTypeNodeTransformsappends outermost, so the hidden-prefix path really does put the discriminator at byte 0 and the uniform offset shift is correct there.
Things to watch
- Inline struct data that already carries transforms (see inline comment on
setInstructionDiscriminatorsVisitor.ts). Prepending a field inside a struct that has ahiddenPrefixTransformNode/sizePrefixTransformNode/preOffsetTransformNodedoesn't put the discriminator at byte 0, so the newfieldDiscriminatorNode(offset 0) and the uniform shift of existing discriminators are wrong in that case. Falling back to the hidden-prefix path whennode.data.transformsis non-empty would fix it cheaply since that path already exists. constantValueNodetypes are treated as values byisValueNumber(see inline comment onsetNumberWrappersVisitor.ts). With a broad selector like[structTypeNode]or[instructionNode], theintegerTypeNodeinside a hidden prefix/sentinel/zeroValueconstant would get wrapped, and aFixedPoint/DateTimewrapper would leave the constant'sintegerValueNodemismatched with its new type.- Changeset: none in the diff.
CONTRIBUTING.mdasks for one on user-facing changes to@codama/errors/@codama/visitors— ignore if the 2.x branch is riding the seeded major changeset.
Notes for other reviewers
- The behavioural changes to
setInstructionAccountDefaultValuesVisitor(exact string matching, no more camelCasing, rethrowing non-seed errors) are intentional per the PR description and documented in the README + docblocks — don't flag them as regressions. - The README example for
setInstructionDiscriminatorsVisitorswitching fromnametoidentifieris enforced at runtime byassertValidUpdateKeys, with a test covering the v1 key. - Test coverage is solid for the happy paths and every error branch. The one gap is the hidden-prefix path combined with pre-existing discriminators, which ties into point 1 above.
9ce770c to
1e898a4
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Both points from my previous pass are addressed:
- Inline struct with transforms —
isPlainStructnow requires an emptytransformsarray before taking the field path, so a struct with ahiddenPrefixTransformNode/sizePrefixTransformNode/ etc. falls through to the hidden-prefix path. The new test ("it adds a hidden prefix to struct data that carries transforms") checks the exact case I was worried about: the pre-existingconstantDiscriminatorNodeat offset 0 is shifted to offset 1 and the new constant sits at 0, which is correct sinceaddTypeNodeTransformsappends outermost. - Constants' types —
isValueNumbernow short-circuits when any ancestor in the path is aconstantValueNodeorconstantPdaSeedNode, so integers nested arbitrarily deep inside a constant's type are skipped. The new test covers both a direct integer constant (hidden prefix) and a struct-typed constant (hidden suffix), with[structTypeNode]as the broad selector.
Docblocks, README and PR description were updated to match. No other files changed since the last pass.
Remaining nit
packages/visitors/test/setNumberWrappersVisitor.test.tsL3–4:constantValueNodeis imported beforeassertIsNode, which the repo'ssimple-import-sortrule will flag. Lint will catch it; mentioning only so it's not a surprise in CI.
For other reviewers
Nothing new to call out beyond the previous review's notes. The changeset question from last time still stands as a soft check if the 2.x branch isn't riding a seeded major changeset.
1e898a4 to
fcc8af3
Compare
46aec0a to
9bb6ed0
Compare
fcc8af3 to
566f28f
Compare
9bb6ed0 to
f74457a
Compare
621d4b9 to
90e3656
Compare
f74457a to
0e134a7
Compare
90e3656 to
dcf7a0d
Compare
0159613 to
0d6f7f5
Compare
dcf7a0d to
015a2ca
Compare
0d6f7f5 to
f0cba6f
Compare
6ef73f4 to
b8ab0fe
Compare
f0cba6f to
4c2d13f
Compare
6ef73f4 to
b8ab0fe
Compare
4c2d13f to
8d4e2b1
Compare
b8ab0fe to
45f5d3f
Compare
45f5d3f to
f8d92f5
Compare

This PR adapts the remaining instruction visitors of
@codama/visitorsto the Codama v2 node model:setInstructionAccountDefaultValuesVisitor,setInstructionDiscriminatorsVisitorandsetNumberWrappersVisitor. With it, the whole package builds against v2.setInstructionAccountDefaultValuesVisitoraccountandinstructionrules are matched exactly rather than camelCased.getCommonInstructionAccountDefaultRulesmatches both camelCase and snake_case identifiers (e.g.systemProgramandsystem_program), since identifiers keep the casing of their program.setInstructionDiscriminatorsVisitorfieldDiscriminatorNode, as before.hiddenPrefixTransformNodeon the data with aconstantDiscriminatorNode, leaving any shared defined type untouched.namebecomesidentifier,typedefaults tointegerTypeNode('u8')and unknown keys throwUNRECOGNIZED_UPDATE_KEYS.CODAMA_ERROR__VISITORS__CANNOT_SET_INSTRUCTION_DISCRIMINATORerror (1200019) is thrown when the field identifier already exists, when theoptionalstrategy is used with a hidden prefix, or when the discriminator type does not have a fixed size.setNumberWrappersVisitorFixedPoint{ scale, base?, unit? }fixedPointTypeNodeSolAmountfixedPointTypeNodeof scale 9 inSOLDateTime/Duration{ ticksPerSecond? }dateTimeTypeNode/durationTypeNodeUnit{ unit }unitattributeAmountDisplay{ decimals, unit? }amountNumberDisplayNodedisplayUnitDisplay{ unit }unitNumberDisplayNodedisplayAmountwrapper is replaced byFixedPoint,UnitandAmountDisplay.transformsof the number they wrap, and integers used as sizes or prefixes (enum sizes, size prefixes, option prefixes, etc.), already wrapped or within the type of a constant are left untouched.CODAMA_ERROR__VISITORS__INVALID_NUMBER_WRAPPERnow reports the wrapperkindand areason. It is thrown for unknown kinds, zero-scale orshortU16fixed points, and integers that already carry a unit or display.Tests
New test files cover the three visitors, including every error case, rule precedence, PDA seed filling and the skipped integer positions. The README is updated accordingly.