Repository navigation
[2.x] Adapt update visitors to Codama v2 - #1178
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Ports the update*Visitors and fillDefaultPdaSeedValuesVisitor to the v2 node model. The core of the change is updateHelpers.ts: a shared getUpdateVisitor that wraps each visitor's transformers with recordLinkablesOnFirstVisitVisitor and a set of reference-repointing transformers driven by a RenamePlan. References are resolved through the LinkableDictionary against the original tree (the NodeStack in bottomUpTransformerVisitor holds pre-transform nodes, so stack.getPath(...) + linkables.getPath(...) always land on original nodes), which is what makes cross-program links and program-scoped selectors work correctly. createPathResolver rewrites config.fee-style paths segment by segment while walking the original types and following defined type links, resetting to top-level-only renames once it crosses into a defined type — consistent with updateDefinedTypesVisitor only renaming top-level members.
The design holds together well and the test coverage is genuinely thorough (cross-program link resolution, tuple indices, self-referencing types, text-node intents, enum values scoped to the matching enum, PDA upserts in other programs). Error codes are appended to the registry without touching existing ones. I checked the node factories (accountLinkNode(newIdentifier, { ...node }), providedNode(identifier, value, { ...merged[index] }), etc.) — they take the identifier positionally and only read known option keys, so the spread-the-old-node pattern is safe.
Things to watch out for
Two correctness edge cases, both in updateInstructionsVisitor and both stemming from the same root: the instruction-level transform runs after the rename transformers have already processed the instruction's children, so anything the instruction transform introduces (or looks up by the old name) is not reconciled with the renames in the same update.
-
Parent + nested data field renamed in one update (
applyInstructionUpdates, data reduce). Updates are applied sequentially to the progressively-transformed type using the user's old paths, so{ args: { identifier: 'params' }, 'args.amount': { identifier: 'lamports' } }silently skips the second update (noargsfield anymore), whilecreatePathResolver— which builds prefixes from original identifiers — does rewrite references toparams.lamports. Result: field isparams.amount, references sayparams.lamports. Details inline. -
Auto-filled PDA seeds use pre-rename identifiers (
applyAccountUpdates).fillDefaultPdaSeedValuesVisitoris fed the original instruction path, so a filledaccountValueNode('owner')dangles ifowneris renamed in the same update. Details inline.
Related, lower priority: when several entries match the same instruction (e.g. transfer and myProgram.transfer, or a [instructionNode] wildcard), the second entry's accounts[oldName] lookup runs against the already-renamed node and silently no-ops, even though assertUpdatedInputsExist passed on the original. Probably rare enough to leave, but worth knowing about.
Also worth stating in the README/docblock: user-supplied defaultValues in the same update are not repointed, so they must use the new identifiers (e.g. accounts: { owner: { identifier: 'authority' }, vault: { defaultValue: pdaValueNode('vault', { seeds: [pdaSeedValueNode('owner', accountValueNode('authority'))] }) } }). Reasonable behaviour, but it's a trap without a note.
Notes for subsequent reviewers
- Changeset: none in the PR.
CONTRIBUTING.mdasks for one per user-facing change, but this is a[2.x]PR and the release notes say the seeded major changeset covers all public packages during the cut. Worth confirming that's the convention for this branch series and not an omission — thearguments→dataandname→identifierkey changes are the kind of thing users will grep changelogs for. updateAccountsVisitor'spdasrename rule renames any PDA in the same program sharing the account's identifier, even if the account'spdalink points elsewhere. Matches the description and v1 semantics, just flagging it's identifier-based rather than link-based (unlike everything else in this PR).getResolvedInstructionInputsVisitornow callsstack.getPath('instructionNode')instead of passingnode— equivalent givenrecordNodeStackVisitorhas pushed the instruction, but if anyone reshuffles that visitor's pipe order it'll break subtly.updateErrorsVisitorintentionally skipsgetUpdateVisitorsince errors have no link nodes; nothing to repoint.
3bb9769 to
67734b2
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
All three points from my first pass are fixed, each with a dedicated test, and I re-verified them against the current head rather than the description:
- Parent + nested field renamed together —
applyDataUpdatesnow does a single walk and recurses intofield.typewith the original path before the field itself is renamed, so{ args: …, 'args.amount': … }works in either order.unusedPathsfeedsINSTRUCTION_DATA_FIELD_NOT_FOUND, which also closes the silent-miss gap I flagged. Covered inupdateHelpers.test.tsandupdateInstructionsVisitor.test.ts. - Auto-filled PDA seeds using pre-rename identifiers —
fillNewDefaultValuesruns against[...path.slice(0, -1), updated], and defaults are keyed byidentifier ?? accountNameso they land on the renamed account. Both directions are tested (staleowneris not filled;authorityis). TheLinkableDictionarylookups inside still resolve through the original program node in the path, so links keep working. - Several entries matching the same node —
createUpdateResolvermerges in declaration order against the original node,deletewins, results cached per node. Tested for instructions and defined types. - Bonus:
getInstructionDataFieldscycle guard is now by node reference, with a same-name-across-programs test.
The updateAccountsVisitor restructure is also cleaner than before: PDA upserts are computed from linkables.getRecordedPathsOfKind('accountNode') per program, so cross-program upserts need no shared mutable state, and the pdas rename rule + programTransformer both use post-rename identifiers consistently (bottom-up order guarantees pdaTransformer has already run when the program is transformed).
Minor notes (non-blocking)
Two small edge cases inline — one on createUpdateResolver's cache identity, one on type + nested-path interplay in applyDataUpdates — plus a readability nit on getRecordedPathsOfKind's default branch. None of these warrant holding the PR.
For subsequent reviewers
- Changeset: still none in the PR. I'm assuming the
[2.x]series relies on a seeded major changeset as per the release notes; worth a one-line confirmation sincearguments→dataandname→identifierare exactly the kind of change people grep changelogs for. getRecordedPathsOfKindis new public API on@codama/visitors-core(README updated). It only lists what was recorded — the docblock says so, but callers that construct aLinkableDictionarymanually should be aware.getResolvedInstructionInputsVisitornow relies onstack.getPath('instructionNode')rather than the passed node — equivalent given therecordNodeStackVisitorwrapping, and the inline comment now calls this out.
67734b2 to
46aec0a
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review (3rd pass)
Small delta since my approval, addressing the three non-blocking nits from last round. Re-fetched all changed files and verified each against the head:
createUpdateResolvercache identity — now keyed on the path (NodePath) rather than the node reference, so a frozen node instance shared between two programs resolves independently per location. Test added inupdateHelpers.test.tsfor the shared-instance case.type+ nested path interplay inapplyDataUpdates— replacing a field'stypewhile also targeting a path underneath it now throwsINSTRUCTION_DATA_FIELD_NOT_FOUNDfor the nested path instead of silently discarding it. Tested in bothupdateHelpers.test.tsandupdateInstructionsVisitor.test.ts.getRecordedPathsOfKinddefaultbranch — the switch now spells out everyLINKABLE_NODESkind explicitly, with an exhaustiveness guard so a future addition toLINKABLE_NODESfails to compile rather than falling through. The +45/-1 inLinkableDictionary.tsis this plus the docblock.
Nothing else moved in the non-test source. Everything I raised across three passes is now resolved and covered by tests; approval stands.
For subsequent reviewers
- Changeset: still absent — flagging one last time so it's a conscious decision on the
[2.x]series rather than an oversight. If the seeded major changeset covers it, all good. - No further concerns from me. The
updateHelpers.tsmodule is the one to read closely if you're coming in fresh:getUpdateVisitor(orchestration),createUpdateResolver(merging multiple matching entries),createPathResolver(dotted-path rewriting through defined type links), andapplyDataUpdates(the recursive walk) are the four load-bearing pieces.
|
One correction to my review above: I described the |
46aec0a to
9bb6ed0
Compare
68455c3 to
599fa34
Compare
9bb6ed0 to
f74457a
Compare
82d679f to
e48e185
Compare
f74457a to
0e134a7
Compare
e48e185 to
66f05b8
Compare
0e134a7 to
0159613
Compare
66f05b8 to
c4f8eac
Compare
0d6f7f5 to
f0cba6f
Compare
ae160c2 to
be4576d
Compare
f0cba6f to
4c2d13f
Compare
ae160c2 to
be4576d
Compare
4c2d13f to
8d4e2b1
Compare

This PR adapts the
update*visitors of@codama/visitorsto the Codama v2 node model, along withfillDefaultPdaSeedValuesVisitor, which they depend on. Renames now repoint every reference to the renamed nodes, and update maps fail loudly on unknown keys or targets.Update visitors
identifierupdates are matched and applied as is, without camelCasing.CODAMA_ERROR__VISITORS__UNRECOGNIZED_UPDATE_KEYSerror when the visitor is created, so keys such as v1'snameorargumentsno longer silently do nothing.updateInstructionsVisitordatareplacesargumentsand updates the fields of the instruction's inlinedata, keyed by path (e.g.amountorconfig.fee, including tuple indices). Default values must beValueNodes: contextual defaults are expressed with aninjectedValueNodeand a matchingprovidesentry.providesis a record merged by identifier with the instruction's provided nodes;nullremoves an entry.defaultValue: nullremoves a default value.INSTRUCTION_ACCOUNT_NOT_FOUNDorINSTRUCTION_DATA_FIELD_NOT_FOUND. Fields behind adefinedTypeLinkNodecannot be updated since the type may be shared.updateAccountsVisitor: renaming a missing data field throwsACCOUNT_FIELD_NOT_FOUND. Updating the seeds of an existing PDA keeps its docs and program ID, and apdalink to another program creates the PDA in that program.updateDefinedTypesVisitor: renaming a missing field or variant throws a newDEFINED_TYPE_MEMBER_NOT_FOUNDerror. Renamed variants keep their discriminator and data, and renamed structs keep their transforms.updateErrorsVisitorandupdateProgramsVisitor.transferandmyProgram.transfer), their updates are merged and applied at once against the original node, so they all refer to its original accounts, fields and members. Adeleteentry wins over any other update.Rename propagation
References are resolved through the
LinkableDictionaryto the node they point to, rather than matched by identifier, so links from other programs are handled correctly and links inprogramortransformsattributes are preserved.accountValueNode,accountBumpValueNode,accountDataValueNode.account,instructionAccountLinkNode,${accounts.…}placeholdersdataValueNode,accountDataValueNode.path,fieldDiscriminatorNodeand${data.…}placeholders going through it, following defined type linksenumValueNode.variantof that enumOther changes
fillDefaultPdaSeedValuesVisitorfills seeds matching top-level data fields withdataValueNodes, following linked data, and keeps theprogramIdandpluginsof PDA values.@codama/visitors-coreexports a newgetInstructionDataFieldshelper listing an instruction's addressable data fields with their paths, now shared withgetResolvedInstructionInputsVisitor. It follows same-named defined types from different programs.LinkableDictionarygains agetRecordedPathsOfKind(kind)method listing the paths of every recorded linkable node of a given kind.UNRECOGNIZED_UPDATE_KEYS(1200015),INSTRUCTION_DATA_FIELD_NOT_FOUND(1200016),INSTRUCTION_ACCOUNT_NOT_FOUND(1200017) andDEFINED_TYPE_MEMBER_NOT_FOUND(1200018).@codama/visitorsand@codama/cliare updated accordingly.Tests
The existing tests are ported to v2 nodes, new test files cover
updateProgramsVisitor,updateErrorsVisitor,updateDefinedTypesVisitor, the internal update helpers andgetInstructionDataFields, and every rename propagation case above is covered.