Skip to content

Adapt errors to Codama v2 - #1167

Draft
lorisleiva wants to merge 1 commit into
09-18-rename_node_name_attribute_to_identifierfrom
09-21-adapt_errors_to_codama_v2
Draft

lorisleiva wants to merge 1 commit into
09-18-rename_node_name_attribute_to_identifierfrom
09-21-adapt_errors_to_codama_v2

Conversation

@lorisleiva

@lorisleiva lorisleiva commented Sep 21, 2026

Copy link
Copy Markdown
Member

Phase D, per-package adaptation (dependency order). Adapts @codama/errors to the v2 node model. Stacked on #1162.

Changes

src/context.ts:

  • CamelCaseStringIdentifierString across the ~42 error-context fields that hold node identifiers (v2 identifiers are IdentifierString, not casing-branded).
  • Resolver error contexts now span accounts and data. In v2, instruction arguments are struct fields of instructionNode.data, and a data field can carry a contextual default via the inject/provide pattern: structFieldTypeNode.defaultValue may be an injectedValueNode whose key is fulfilled by instructionNode.providesprovidedNode.node (a contextual value such as dataValueNode, accountValueNode, accountBumpValueNode, …). So a default-value dependency graph — and therefore a cycle — can involve accounts, data fields, or both. defaultValue lives on InstructionAccountNode and StructFieldTypeNode only, so those are the participant types.
    • CYCLIC_DEPENDENCY_DETECTED_WHEN_RESOLVING_INSTRUCTION_DEFAULT_VALUES: cycle retyped (InstructionAccountNode | InstructionArgumentNode)[](InstructionAccountNode | StructFieldTypeNode)[].
    • INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCY: dependencyAccountValueNode | DataValueNode, dependencyKind'accountValueNode' | 'dataValueNode' (dataValueNode replaces the removed argumentValueNode); dependencyNameIdentifierString | PathString (a dataValueNode carries a path); parentInstructionAccountNode | StructFieldTypeNode, parentKind'instructionAccountNode' | 'structFieldTypeNode', parentNameIdentifierString | PathString.
  • Imports: dropped ArgumentValueNode, InstructionArgumentNode, CamelCaseString; added DataValueNode, IdentifierString, PathString, StructFieldTypeNode.

src/messages.ts:

  • CYCLIC_... reworded to drop the participant qualifier (was "accounts and arguments' default values"), since a cycle may run through accounts, data, or both: "resolving the default values of the [$instructionName] instruction".
  • INVALID_... keeps the generic $dependencyName/$dependencyKind/$parentName/$parentKind/$instructionName form — no hardcoded participant kind.

test/error.typetest.ts: 'numberTypeNode''integerTypeNode' in example node-kind arrays (removed kind → v2 successor).

Follow-ups for the visitors-core resolver PR

These two errors are thrown only by getResolvedInstructionInputsVisitor (still v1, rewritten later in the stack). When adapted, its throw sites must build these v2 context shapes: an account input → InstructionAccountNode (parentName = identifier); a data-field input → StructFieldTypeNode (parentName = its PathString); dependencyKind/Name from the accountValueNode | dataValueNode union. The graph walks instructionNode.provides + injectedValueNode + dataValueNode + account defaults.

The old v1 isPda detection (which scanned instruction arguments for an accountBumpValueNode default) maps onto this same model in v2: a data-field bump default is providedNode('bump', accountBumpValueNode(...)) feeding a structFieldTypeNode via injectedValueNode.

Verification

tsc clean (src + test), pnpm build success, 27/27 tests pass, lint clean.

@changeset-bot

changeset-bot Bot commented Sep 21, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 34df2cc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Copy Markdown
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@lorisleiva
lorisleiva force-pushed the 09-21-adapt_errors_to_codama_v2 branch 2 times, most recently from 068dda2 to 9d5c372 Compare September 21, 2026 10:48
@lorisleiva

Copy link
Copy Markdown
Member Author

@trevor-cortex

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adapts @codama/errors to the v2 node model: swaps CamelCaseStringIdentifierString across the identifier-bearing context fields, narrows the two resolver-only error contexts (CYCLIC_DEPENDENCY_... and INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCY) to the accounts-only / accountValueNode | dataValueNode shapes, and updates the typetest's example node kinds (numberTypeNodeintegerTypeNode).

I verified the imported types against @codama/node-types at head: IdentifierString and PathString exist in brands.ts, DataValueNode.path is a PathString, AccountValueNode.identifier is an IdentifierString, and StructFieldTypeNode.defaultValue is ValueNode | undefined (so the reasoning that only account defaults can carry contextual references, hence accounts-only cycle/parent, holds). The mechanical CamelCaseStringIdentifierString sweep is correct throughout, and CamelCaseString still exists in brands.ts so nothing else breaks from dropping the import here.

Things to watch

  • Description vs. code mismatch on parentKind / messages.ts. The description says parentKind was dropped and the INVALID_... message template was reworded to remove $parentKind, but the diff only touches two files: context.ts still has parentKind: 'instructionAccountNode' (narrowed to a literal, not removed) and messages.ts at head still interpolates [$parentKind]. The code as it stands is internally consistent, and keeping the now-constant field arguably respects the "Don't change or remove members of an error's context" note in context.ts — so this is fine to land as-is. Just reconcile one way or the other: either update the description, or amend to actually drop the field + reword the template so the resolver PR builds against what the description promises. Inline comment below.
  • Cyclic-dependency message wording (nit, not in diff). messages.ts still says "resolving the accounts and arguments' default values" for CYCLIC_DEPENDENCY_.... Since the cycle is now accounts-only by construction, "arguments" is slightly misleading. Not blocking — could be folded into the resolver PR if you prefer to keep this one type-only.
  • Changeset. @codama/errors is in the fixed core group and this is a user-facing type change, but I'm assuming the v2 stack is covered by the seeded major changeset described in CONTRIBUTING.md rather than a per-PR one. Flagging only so a subsequent reviewer doesn't need to re-check.

Notes for subsequent reviewers

  • The dependencyName: IdentifierString | PathString union is intentionally not correlated with dependencyKind at the type level (context types are flat records). The resolver PR's throw sites will need to pick the right branch; worth a glance there.
  • The open question in the description about where instruction bump defaults live in v2 (accountBumpValueNode can't be a struct-field default anymore) is real and belongs to the resolver rewrite, not this PR.

Comment thread packages/errors/src/context.ts Outdated
Comment thread packages/errors/src/context.ts Outdated
@lorisleiva
lorisleiva force-pushed the 09-21-adapt_errors_to_codama_v2 branch from 9d5c372 to 34df2cc Compare September 21, 2026 15:51

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review

Re-fetched the diff and node-types at head. This revision supersedes my earlier pass and, importantly, corrects a gap in my own reasoning: I previously said struct-field defaults "can't hold contextual references" because StructFieldTypeNode.defaultValue is ValueNode | undefined. That's true on the surface, but InjectedValueNode is a member of StandaloneValueNode (hence ValueNode), and InstructionNode.providesProvidedNode.node: Node is a transparent pipe. So a data field can indeed reach a dataValueNode / accountValueNode / accountBumpValueNode via injectedValueNode.keyprovides. The widened (InstructionAccountNode | StructFieldTypeNode)[] cycle and the structFieldTypeNode parent branch are the right model, and the "where do bump defaults live" question from the previous description is answered by the same mechanism.

What I verified this round:

  • InjectedValueNode ∈ StandaloneValueNode, InstructionNode.provides?: ProvidedNode[], ProvidedNode.node: Node — the inject/provide path described in the PR body exists in node-types as-is.
  • messages.ts: the CYCLIC_... rewording drops the now-inaccurate "accounts and arguments'" qualifier while keeping the message's meaning (per the "Don't change the meaning" note). The INVALID_... template is untouched and still lines up with the context fields ($dependencyName/$dependencyKind/$parentName/$parentKind/$instructionName), so the description/code mismatch I flagged earlier is resolved — the union parentKind was the right call.
  • The CamelCaseStringIdentifierString sweep is unchanged from last time and still correct; PathString and IdentifierString both exist in brands.ts.
  • Typetest numberTypeNodeintegerTypeNode is straightforward.

Notes for the resolver PR (not blocking here)

  • A StructFieldTypeNode in cycle / parent doesn't carry its own PathStringidentifier is just the leaf segment. The resolver will need to thread the full path from the instructionNode.data walk to populate parentName and formattedCycle for nested fields (e.g. config.fees[0]). Inline note on the cycle line.
  • Since data may be a definedTypeLinkNode, the resolver's graph walk has to follow links before it can enumerate struct fields, and lexical shadowing in provides (sub-instructions) means the same field type can resolve differently per instruction. Both are resolver concerns; the error shapes here accommodate them fine.

};
[CODAMA_ERROR__VISITORS__CYCLIC_DEPENDENCY_DETECTED_WHEN_RESOLVING_INSTRUCTION_DEFAULT_VALUES]: {
cycle: (InstructionAccountNode | InstructionArgumentNode)[];
cycle: (InstructionAccountNode | StructFieldTypeNode)[];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widening to StructFieldTypeNode checks out — InjectedValueNode is part of StandaloneValueNode, so a field default can reach contextual nodes through instructionNode.provides. (My earlier accounts-only reasoning missed that indirection.)

One thing for the resolver PR: a StructFieldTypeNode here only carries its leaf identifier, not the PathString relative to instructionNode.data. For nested fields the resolver will have to track the path itself when it builds formattedCycle / parentName, since the node in cycle won't be enough on its own to disambiguate e.g. two bump fields in different sub-structs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants