Repository navigation
[2.x] Add validator rules for spec 2.0.0-rc.2 - #1226
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds the validator rules for spec 2.0.0-rc.2 plus the shared helpers they need:
parseInterpolatedIntent/printInterpolatedIntentin@codama/nodes, turning${data.…}/${accounts.x}/${accounts.x.data…}placeholders into the contextual value nodes they denote (and back), with a newCODAMA_ERROR__INVALID_INTERPOLATED_INTENT_PLACEHOLDERfor anything off-grammar.resolveTypePathgainsfollowAccountLinks, andisValueNodeOfTypechecks a concrete value against a type structurally (following defined-type links, detecting alias cycles).- New validation items: ticks-per-second, fixed-point scale, value labels (concrete + type-matching), field discriminator paths (must land on a struct field, must not cross account links), missing account links, and the account/path checks for
dataValueNode,accountDataValueNodeand intent placeholders via a sharedgetContextualValueItems. - The rename transformers in
updateHelpersare refactored intorenameAccountValue/renameDataValue/renameAccountDataValue, which are reused to repoint intent placeholders — including${accounts.x.data.field}, which the old regex didn't handle.
The code is clean and the test coverage is thorough; I traced the parser grammar, the account-link following, and the dedupe logic against the resolver and they hold up. Two things worth a look before merging:
Watch out for
-
Double-reporting gap for
accountDataValueNodedefaults. The PR dedupesINVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCYonly when the dependency is adataValueNode. ButgetDependencies()turns anaccountDataValueNode('mint', …)into a synthesizedaccountValueNode('mint'), so an account default likeconditionalValueNode({ condition: accountDataValueNode('mint', { path: 'decimals' }), … })with nomintaccount gets reported twice: once by the resolver ("invalid dependency") and once byvisitAccountDataValue("Account "mint" is not an account of instruction…"). See the inline comment for a suggested approach. -
accountValueNodehas no contextual check.getContextualValueItemshandlesaccountValueNode(used for intent placeholders), but there's novisitAccountValue, soprovidedNode('x', accountValueNode('missing'))that is never injected into a default goes unreported, while the same node in an intent is reported. AddingvisitAccountValuewould close that and also make the dedupe in (1) uniform: "skip the resolver error whenever the dependency reports itself".
Minor / optional
isValueNodeOfTypeforenumTypeNodematches onvalue.variantonly;value.enum(the link on the value node) isn't checked against the type being matched. Given the doc says "structural", that's probably intended, but worth a line in the docblock so nobody files it as a bug.FixedPointTypeNode.scaleis documented as "must be non-zero" while the validator requires a positive integer. Likely the spec intends positive — just flagging the wording mismatch in case the spec text should be tightened.- No changeset in the PR. Per
CONTRIBUTING.mduser-facing changes need one — unless rc work on this branch rolls into the seeded major changeset, in which case ignore.
Notes for other reviewers
- The
followedAccountscycle guard inresolveContaineris per-segment by design:next.next.valueon a self-linking list legitimately re-follows the same account; only an account whosedatais itself a self-linking public key (no type to apply the segment to) throws. The tests cover both. visitInjectedValuenow treatsvalueLabelDisplayNodeas a nesting ancestor so injected labels are reported once (byvisitValueLabelDisplay) rather than as an unprovided injection — intentional.- Intent renaming returns the original string when no part changed, so formatting is only normalised through
printInterpolatedIntentwhen a rename actually occurred; the parse→print round-trip is tested as identity for valid templates.
af8dcb4 to
824a1b4
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Both points from the first pass are addressed:
visitAccountValueis in, andvisitAccountBumpValuechecks the bump's account the same way — so bump values pointing at unknown accounts (including inside PDA seeds, which the resolver never tracked) are now reported.- The resolver dedupe is now uniform: the
INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCYerror is skipped whenevergetContextualValueItemswould report the dependency itself. Covered by the new "account data value defaults of unknown accounts once" test. isValueNodeOfTypedocblock now states the enum link isn't compared, and thevisitFieldDiscriminatorearly return is in.
I re-traced the dedupe against getResolvedInstructionInputsVisitor.getDependencies / resolveDependencies and it holds for all the self-contained cases: accountValueNode, accountBumpValueNode, accountDataValueNode (synthesized to accountValueNode), dataValueNode, PDA seeds and conditionals. The cases where resolveTypePath succeeds but findFieldByPath doesn't (tuple data with [0], or a linked data type with no program in the path) still fall through to the resolver error, which is the right fallback.
One edge case worth a decision (inline): a dependency that comes from a parent instruction's provides is checked against the parent's accounts by visitAccountValue/visitDataValue, but the resolver checks it against the sub-instruction's. If it's valid in the parent and invalid in the sub-instruction, the new dedupe drops the resolver error and nothing else reports it. Not blocking — it's a corner of the provides/sub-instruction model — but it's a silent regression versus the previous behaviour, so worth either guarding or documenting.
Still open from last time, both minor: FixedPointTypeNode.scale doc says "non-zero" while the validator requires positive; and no changeset (ignore if rc work rolls into the seeded major one).
Approving — the remaining point is a judgement call rather than a correctness issue in the common path.
824a1b4 to
157675b
Compare

This PR adds validator rules for the features of spec
2.0.0-rc.2, and the helpers they share with the rest of Codama.parseInterpolatedIntentandprintInterpolatedIntentturn placeholders into the contextual value nodes they are equivalent to, and back. Invalid placeholders throwCODAMA_ERROR__INVALID_INTERPOLATED_INTENT_PLACEHOLDER.resolveTypePathcan follow account links withfollowAccountLinks, andisValueNodeOfTypechecks a value against a type structurally.${accounts.x.data…}placeholders.