Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Codama’s node model and default processing pipeline to support event nodes, including schema/JSON updates and visitor + macro support to populate ProgramNode.events.
Changes:
- Add
EventNodetocodama-nodes, wire it intoNode/ProgramNode, and update serialization expectations. - Introduce
SetEventsVisitorand run it in the default plugin so events get collected into program/root nodes and merged across modules. - Add
CodamaEvent/CodamaEventsderive entrypoints and add/extend tests across crates to cover event extraction, directives, skipping, and merging.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| codama/tests/system/mod.rs | Updates system test IDL JSON to include an events array. |
| codama/tests/membership/mod.rs | Updates membership test IDL JSON to include an events array. |
| codama-plugin-core/src/default_plugin.rs | Runs SetEventsVisitor as part of the default program item pipeline. |
| codama-nodes/src/root_node.rs | Updates root-node JSON expectations to include program events. |
| codama-nodes/src/program_node.rs | Adds events: Vec<EventNode> and an add_event helper; updates tests/JSON. |
| codama-nodes/src/node.rs | Adds Node::Event(EventNode) and wires it into HasKind. |
| codama-nodes/src/lib.rs | Registers and re-exports the new event_node module. |
| codama-nodes/src/event_node.rs | Introduces the EventNode definition with JSON roundtrip tests. |
| codama-macros/src/lib.rs | Adds proc-macro derives CodamaEvent and CodamaEvents. |
| codama-macros/tests/codama_events_derive/_pass.rs | Adds a passing trybuild case for CodamaEvents. |
| codama-macros/tests/codama_event_derive/_pass.rs | Adds a passing trybuild case for CodamaEvent. |
| codama-macros/tests/codama_event_derive/multiple_types.fail.rs | Adds a failing trybuild case for conflicting #[codama(type=...)] on events. |
| codama-macros/tests/codama_event_derive/multiple_types.fail.stderr | Adds expected compiler output for the failing event derive case. |
| codama-korok-visitors/src/set_events_visitor.rs | Implements event extraction into EventNode / ProgramNode.events (incl. directives + discriminators). |
| codama-korok-visitors/src/lib.rs | Exposes SetEventsVisitor. |
| codama-korok-visitors/src/combine_modules_visitor.rs | Merges/dedupes events when combining modules and scraps. |
| codama-korok-visitors/tests/lib.rs | Registers the new set_events_visitor test module. |
| codama-korok-visitors/tests/set_events_visitor/* | Adds coverage for CodamaEvent(s), program directive behavior, and skip semantics. |
| codama-korok-visitors/tests/combine_modules_visitor/* | Extends combine-modules tests to include event merging/deduplication. |
| codama-attributes/src/codama_directives/program_directive.rs | Allows #[codama(program(...))] to wrap Node::Event into a ProgramNode. |
| codama-attributes/src/attributes.rs | Treats CodamaEvent / CodamaEvents as “program items” for attribute filtering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds first-class EventNode support across the stack: a new node type in codama-nodes, a SetEventsVisitor to collect events from #[derive(CodamaEvent)] structs and #[derive(CodamaEvents)] enums, wiring into ProgramNode.events + CombineModulesVisitor merging/dedup, the program(...) directive wrapping events into programs, and test coverage that mirrors accounts/instructions/errors. The implementation closely follows the SetAccountsVisitor pattern, which is the right base to port from.
Overall this looks solid and the test coverage is thorough. A few things worth looking at before merging:
Key things to watch out for
EventNode.datafield type — typed as plainTypeNode, but every other struct-shaped top-level node (e.g.AccountNode.data) is typed asNestedTypeNode<StructTypeNode>. The visitor already operates onNestedTypeNode<StructTypeNode>internally (seeparse_struct/parse_enum_variant/map_nested_type_node) and then widens toTypeNodeonly when callingEventNode::new. This inconsistency is worth resolving — see inline comment.- Tuple enum variants are rejected for
CodamaEvents—SetInstructionsVisitorhandles tuple variants, butSetEventsVisitor'sparse_enum_variantonly matchesStructandEmpty. Intentional? If so, it's worth a doc comment; if not, it's inconsistent with instructions. - Merge-order change in
merge_program_nodes—errorsandpdaswere swapped. Cosmetic (each vector is merged independently), but worth confirming the ordering matches the struct field order on purpose.
Notes for subsequent reviewers
SetEventsVisitoris essentiallySetAccountsVisitorminus the PDA/seed handling. Good portability, but theEventNode.datatype divergence is a real deviation from that pattern._pass.rsincodama_event_deriveaccepts both a struct and an (empty) enum, and thefrom_enumvisitor test comment ("No visitor error because there is already a compilation error.") is copy-pasted from the accounts tests — there is no actual compilation error, derivingCodamaEventon an enum is a silent no-op. This is pre-existing convention acrossCodamaAccount/CodamaEventso I'm not flagging it, but worth being aware of.- The
skip_preserves_sibling_discriminator_countingtest is good — confirms that#[codama(skip)]still advances the discriminator counter, matchingSetInstructionsVisitor/SetErrorsVisitorsemantics. - No tests for merging two enums annotated with
CodamaEventsand#[codama(program(...))]pointing at the same external program — theprogram_directive.rstests only cover a single enum/struct each. Not strictly needed since this goes through the sharedmerge_program_nodescode path, but worth verifying manually once.
|
|
||
| // Children. | ||
| pub data: TypeNode, | ||
| #[serde(default, skip_serializing_if = "crate::is_default")] |
There was a problem hiding this comment.
Consider typing data as NestedTypeNode<StructTypeNode> instead of TypeNode.
AccountNode.data is declared as NestedTypeNode<StructTypeNode>, which guarantees at the type level that the event's data is always a struct (optionally wrapped in FixedSize/SizePrefix/HiddenPrefix/etc). The visitor already operates on that type:
parse_structreturnsCodamaResult<(CamelCaseString, NestedTypeNode<StructTypeNode>)>parse_enum_variantreturns the samemap_nested_type_node(...)is used to insert the enum discriminator into the innermost struct
...and only widens to TypeNode at the very last moment when calling EventNode::new(name, data) (because U: Into<TypeNode>). Downstream consumers of EventNode.data now have to re-validate that the data is struct-shaped, whereas AccountNode consumers get it for free.
Suggested change:
#[node]
pub struct EventNode {
pub name: CamelCaseString,
#[serde(default, skip_serializing_if = "crate::is_default")]
pub docs: Docs,
// Children.
pub data: NestedTypeNode<StructTypeNode>,
#[serde(default, skip_serializing_if = "crate::is_default")]
pub discriminators: Vec<DiscriminatorNode>,
}
impl EventNode {
pub fn new<T, U>(name: T, data: U) -> Self
where
T: Into<CamelCaseString>,
U: Into<NestedTypeNode<StructTypeNode>>,
{ ... }
}This would make the visitor's EventNode::new(name, data) calls work unchanged (since NestedTypeNode<StructTypeNode>: Into<NestedTypeNode<StructTypeNode>> and StructTypeNode: Into<NestedTypeNode<StructTypeNode>> both hold), and bring EventNode in line with the rest of the node model. If there's an intentional reason to allow non-struct event payloads here (e.g. to mirror a specific TS shape), it'd be worth calling out in a doc comment.
There was a problem hiding this comment.
@trevor-cortex No, this is based on a new node definition that has already happened on the codama-idl/codama repository (the JS counterpart). The event must technically accept any TypeNode and therefore could even accept tuple structs. Can you adjust your suggestions accordingly?
There was a problem hiding this comment.
Ah, got it — if the canonical JS EventNode takes any TypeNode (including tuple structs), then EventNode.data: TypeNode is correct and the node shouldn't change. In that case the issue flips: it's the visitor that's overly restrictive. Right now parse_struct / parse_enum_variant both return NestedTypeNode<StructTypeNode> and hard-error on anything else, so tuple structs and non-struct payloads that the node type is supposed to allow can never actually reach it.
A few adjustments to bring the visitor in line with the relaxed node:
-
parse_structshould return(CamelCaseString, TypeNode)and accept anyTypeNodefrom theDefinedTypeNode, not justNestedTypeNode<StructTypeNode>. The error branch ("its type is not aNestedTypeNode<StructTypeNode>") then goes away entirely — any defined type is valid. -
parse_enum_variantneeds aTuplearm. Currently it only matchesEnumVariantTypeNode::StructandEnumVariantTypeNode::Empty;EnumVariantTypeNode::Tuplefalls through to the error. For consistency with the node type allowing tuple payloads, it should return the tuple'sTupleTypeNode(wrapped in whateverNestedTypeNodelayers are present) as a plainTypeNode. -
Discriminator injection needs rethinking for the enum path. The current code does:
let data = data.map_nested_type_node(|node| { let mut fields = node.fields; fields.insert(0, discriminator); StructTypeNode { fields } });
This only works when
dataisNestedTypeNode<StructTypeNode>. Once tuple/empty variants are allowed, there's a design question: where does the discriminator field go when the payload is a tuple or a non-struct type? Options:- (a) Wrap non-struct payloads in a synthetic struct with
{ discriminator, data: <payload> }— clean but changes the shape. - (b) Only inject the discriminator field when the payload is struct-shaped, and for tuple/empty variants rely purely on the
FieldDiscriminatorNodeindiscriminatorswithout a corresponding struct field (which would then need a different discriminator node kind, sinceFieldDiscriminatorNodepoints at a field that doesn't exist). - (c) Mirror exactly whatever the JS
codamarepo does forCodamaEventson tuple variants — if there's a reference implementation there, that's the safest bet.
Do you know which direction the JS side went? That'd settle it. If the JS
SetEventsVisitorequivalent just doesn't support tuple variants insideCodamaEventsenums (and only the standalone struct path accepts arbitraryTypeNode), then the minimum change is just (1) — relaxingparse_struct— and leavingparse_enum_variantstruct/empty-only with a doc comment explaining why. - (a) Wrap non-struct payloads in a synthetic struct with
-
Error message in
parse_structshould be rewritten to match whatever constraint actually remains (likely "could not resolve aDefinedTypeNodefor this struct" rather than a struct-shape requirement).
Happy to narrow this down once you confirm which of (a)/(b)/(c) matches the JS behavior — my guess is it's closest to just supporting the struct path for now and the tuple-variant case is a future extension, in which case this reduces to a small relaxation of parse_struct + a doc comment on parse_enum_variant.
| pub struct StructTest; | ||
|
|
||
| #[derive(CodamaEvent)] | ||
| pub enum EnumTest {} |
There was a problem hiding this comment.
Worth double-checking: this pass test asserts that #[derive(CodamaEvent)] on an enum EnumTest {} compiles, but in set_events_visitor/from_codama_event.rs::from_enum, the test expects korok.node to be None after running the visitor (with the comment "No visitor error because there is already a compilation error"). Those two statements contradict each other — the derive does compile, and the visitor silently no-ops.
This same pattern exists for CodamaAccount/set_accounts_visitor (the comment was clearly copied from there), so it's not a regression introduced by this PR. But if the intent is actually "deriving CodamaEvent on an enum should be an error", the macro needs a validation step; if the intent is "permissive, the visitor decides", the misleading comment should be removed. No action strictly required for this PR — flagging for awareness.
lorisleiva
left a comment
There was a problem hiding this comment.
Looks good to me! @stegaBOB are you happy with it?
Following up on jup-ag/codama@6487af5
This PR adds support for event nodes
Hopefully that helps 🙏