Add custom struct types - #76
Conversation
WalkthroughChangesThe language now supports named structs, struct literals, nested property access, fixed-size arrays, and native multi-value result structs. Compiler and validator paths recursively flatten struct fields and arrays into dotted ABI paths. Bindgen targets preserve those paths with distinct Go and TypeScript field names. Documentation, examples, and tests cover the new behavior. Struct language and type system Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ContractSource
participant Parser
participant Typechecker
participant Validator
participant Compiler
participant Bindgen
ContractSource->>Parser: parse struct definitions and struct expressions
Parser->>Typechecker: provide struct-aware AST
Typechecker->>Validator: resolve nested fields and validate layouts
Validator->>Compiler: validate bindings and flatten scalar leaves
Compiler->>Bindgen: provide contract metadata and dotted ABI fields
Bindgen->>Bindgen: generate target-specific Go and TypeScript names
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Playground PreviewA live preview of this PR's playground is available at:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
tests/features/structs.rs (1)
283-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion for local struct leaves.
The only assertion is that no
policy.placeholder is emitted. A regression that dropped the literal initialization or both assignments would still pass this test. Assert the observable stack effect as well, for example the twoOP_PICK-based reads and the equality opcode that the finalrequiremust emit.💚 Proposed additional assertion
let asm = &output.functions[0].arkade.as_ref().expect("covenant").asm; assert!(asm.iter().all(|token| !token.contains("policy."))); + // The literal pushes 4 leaves; both assignments rebind leaves in place, so + // the final require must compare two stack reads. + assert!( + asm.iter().filter(|token| token.as_str() == "OP_PICK").count() >= 2, + "local struct leaves must be read from stack depth: {asm:?}" + ); + assert!( + asm.iter().any(|token| token == OP_EQUAL), + "final require must compare the two reassigned leaves: {asm:?}" + );Confirm the exact opcodes the current codegen emits before you fix the expected values.
As per coding guidelines, "Non-trivial logic requires the smallest runnable regression check that would fail if the logic broke" and "When assembly semantics change, assert the affected spend group, covenant, leaves, witness shape, and critical opcodes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/features/structs.rs` around lines 283 - 305, Strengthen local_struct_literals_bind_and_assign_scalar_leaves by first confirming the current assembly opcodes, then assert the expected positive stack behavior for both policy leaf reads and the final equality check. Keep the existing assertion rejecting policy. placeholders, and add checks for the two OP_PICK-based reads plus the equality opcode emitted by require.Source: Coding guidelines
arkade-bindgen/src/ir.rs (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
namedoc match the emitted value.
namenow holdsleaf.emitted_name, which is the flattened dotted leaf path (for examplepolicy.primary.key). The artifact keeps one entry per source parameter (policy: Policy), so the dotted name appears in the asm placeholders and witness stack, not as an artifact input entry.♻️ Proposed doc wording
- /// Scalar placeholder name as it appears in the artifact. + /// Flattened scalar leaf path emitted in the covenant asm placeholders + /// and witness stack (e.g. "policy.primary.key", "oracles.0"). pub name: String,As per coding guidelines, "Comments must describe only what is currently true".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arkade-bindgen/src/ir.rs` at line 68, Update the documentation for the name field in the relevant IR definition to describe its current value as the flattened dotted leaf path from leaf.emitted_name, including that it is used for ASM placeholders and witness-stack entries rather than artifact input entries.Source: Coding guidelines
arkade-bindgen/src/targets/typescript.rs (1)
47-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the intentional mixed naming convention.
field_nameproduces two different conventions in the same interface. A scalarmy_fieldbecomesmyField, but a nested leafouter.my_fieldstays verbatim as"outer.my_field". That keeps dotted paths unique and prevents thea_bversusa.bcollision, so the trade-off is correct. Add a short comment that names the condition which would justify replacing it, for example an IR that carries structured path segments instead of a flat string.♻️ Proposed comment
+/// Emits a TypeScript property key for a flattened IR field. +/// +/// Dotted leaf paths are quoted verbatim so that `a_b` and `a.b` stay distinct. +/// Scalar names keep camel case, so one interface can mix both conventions. +/// Replace this with per-segment camel casing only when the IR carries +/// structured path segments instead of a single flat string. fn field_name(name: &str) -> String {As per coding guidelines, "add a short comment for an intentional simplification with a known ceiling and name the condition that would justify replacing it".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arkade-bindgen/src/targets/typescript.rs` around lines 47 - 53, Add a concise explanatory comment above or within field_name documenting that dotted names remain quoted verbatim to preserve unique nested paths and avoid collisions with flattened names, while non-dotted names use camelCase. State that this simplification should be replaced if the IR provides structured path segments instead of flat strings.Source: Coding guidelines
src/parser/expr.rs (1)
322-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the reserved property names, and consider a clearer diagnostic.
This fixed name set makes a struct field that is named
delta,assetId,numInputs,numOutputs,sumInputs,sumOutputs,hasControl,metadataHash, orisFreshunreachable. Forstruct Stats { int delta; }andcontract C(Stats s), the expressions.deltaparses asExpression::GroupProperty { group: "s", property: "delta" }. The validator then reports that the asset groupshas the wrong type, which does not describe the real cause.The parser has no scope information, so the name set is a reasonable simplification. Add a short comment that states the ceiling and the condition that would justify replacing it, for example resolving the root binding type before choosing
GroupProperty. The coding guidelines require this comment for an intentional simplification.♻️ Proposed comment
let parts = inner.iter().map(|part| part.as_str()).collect::<Vec<_>>(); + // Simplification: the parser has no scope, so a two-part path whose + // second segment matches an asset-group property is always an asset + // group access. A struct field with one of these names is therefore + // unreachable. Replace this with a scope-aware resolution once the + // parser can look up the root binding type. if parts.len() == 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parser/expr.rs` around lines 322 - 341, Add a concise comment immediately above the reserved property-name check in the expression parser, documenting that the fixed set can shadow struct fields and is an intentional simplification because the parser lacks scope/type information. State that this should be replaced with root-binding type resolution before selecting Expression::GroupProperty when such context becomes available; leave the parsing behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@arkade-bindgen/src/naming.rs`:
- Around line 55-72: Update to_go_field_name so distinct initial-case paths such
as value.a and Value.a remain distinct in generated Go field names, or reject
that collision during identifier validation/binding before generation. Add an
explicit regression test covering both paths and ensure Go bindgen cannot emit
duplicate field names.
In `@playground/codegen.js`:
- Around line 46-52: The standalone underscore source name must be rejected
before code generation, across all source contexts including constructor
parameters and tapscript inputs. Add compiler/validation handling so "_" cannot
reach toGoFieldName or goValueExpr, and report a clear error rather than
substituting a Go-safe field name only in codegen.js.
In `@src/compiler/concat.rs`:
- Around line 73-83: The four let-binding scope-seeding sites duplicate
divergent logic; extract a shared helper that selects the declared type or
inferred type, expands struct types via build_scope_with_structs, and otherwise
inserts the binding directly. Update src/compiler/concat.rs:73-83,
src/typechecker/mod.rs:267-285, and src/validator/mod.rs:452-472 to call it,
including inferred struct fields such as p.x and p.y; in
src/validator/mod.rs:905-948, replace both duplicate loops with one helper call
while preserving the BindingSource::Local wrapper.
- Around line 73-83: Update the untyped branch in the surrounding scope-building
logic to detect inferred struct types and seed their field paths, matching the
existing inferred-struct handling in the typechecker and validator. Preserve the
current direct insertion for non-struct inferred types, and ensure an inferred
ECPoint binding makes fields such as p.x and p.y available for numeric
validation in rewrite_expression_concat.
In `@src/compiler/mod.rs`:
- Around line 953-961: Update collect_struct_literal_leaves to recognize native
struct types through builtin_struct_fields before searching user-defined
structs, and bind their multi-item expressions using the same stack-order logic
as bind_native_struct. Preserve existing handling for ordinary builtins and user
structs, and add a minimal regression test covering an AssetId, Outpoint, or
ECPoint result nested in a user-defined struct.
In `@src/compiler/tapscript.rs`:
- Around line 273-276: The name_declared closure in the tapscript
name-validation flow must accept only scalar bindings. When checking
constructor_scope entries, inspect each binding’s ArkType and reject Struct or
Array container entries while preserving scalar constructor names and tapscript
inputs; ensure composite operands such as policy or nested policy.primary fail
compilation before leaf placeholder assembly.
In `@src/typechecker/mod.rs`:
- Line 626: Add a negative native-assembly regression test covering struct
equality, using an expression such as tx.inputs[0].outpoint == expected and
asserting the typechecker reports the composite comparison error. Place it
alongside the existing native-assembly tests covering outpoint, assetId, ecAdd,
and struct field reordering; do not change the ArkType mapping.
In `@src/validator/mod.rs`:
- Around line 703-713: Update validate_binding_expression handling for
Expression::ContractInstance to reject arguments containing computed property
access, including nested Expression::Property expressions such as
candidate.primary.key. Apply the rejection during contract-instance argument
validation rather than relying on later output validation, while preserving
validation for non-property arguments.
In `@tests/e2e/structs_test.go`:
- Around line 41-45: Before the loop that indexes group.Arkade.ASM, assert that
its length matches the number of names returned by expandInput for the
constructor inputs. Keep the existing per-token comparison unchanged, so
insufficient prologue tokens produce a readable test failure instead of an
index-out-of-range panic.
---
Nitpick comments:
In `@arkade-bindgen/src/ir.rs`:
- Line 68: Update the documentation for the name field in the relevant IR
definition to describe its current value as the flattened dotted leaf path from
leaf.emitted_name, including that it is used for ASM placeholders and
witness-stack entries rather than artifact input entries.
In `@arkade-bindgen/src/targets/typescript.rs`:
- Around line 47-53: Add a concise explanatory comment above or within
field_name documenting that dotted names remain quoted verbatim to preserve
unique nested paths and avoid collisions with flattened names, while non-dotted
names use camelCase. State that this simplification should be replaced if the IR
provides structured path segments instead of flat strings.
In `@src/parser/expr.rs`:
- Around line 322-341: Add a concise comment immediately above the reserved
property-name check in the expression parser, documenting that the fixed set can
shadow struct fields and is an intentional simplification because the parser
lacks scope/type information. State that this should be replaced with
root-binding type resolution before selecting Expression::GroupProperty when
such context becomes available; leave the parsing behavior unchanged.
In `@tests/features/structs.rs`:
- Around line 283-305: Strengthen
local_struct_literals_bind_and_assign_scalar_leaves by first confirming the
current assembly opcodes, then assert the expected positive stack behavior for
both policy leaf reads and the final equality check. Keep the existing assertion
rejecting policy. placeholders, and add checks for the two OP_PICK-based reads
plus the equality opcode emitted by require.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e53c6f6-7722-4772-86fa-5e3f0cc6ecfa
📒 Files selected for processing (41)
README.mdarkade-bindgen/src/ir.rsarkade-bindgen/src/naming.rsarkade-bindgen/src/targets/go.rsarkade-bindgen/src/targets/typescript.rsarkade-bindgen/tests/go_test.rsarkade-bindgen/tests/ir_test.rsarkade-bindgen/tests/typescript_test.rsexamples/struct_vault/struct_vault.arkplayground/arkade-language.jsplayground/codegen.jsplayground/main.jssrc/compiler/asset.rssrc/compiler/concat.rssrc/compiler/expr.rssrc/compiler/loops.rssrc/compiler/mod.rssrc/compiler/tapscript.rssrc/lib.rssrc/models/mod.rssrc/parser/asset.rssrc/parser/expr.rssrc/parser/grammar.pestsrc/parser/mod.rssrc/parser/tapscript.rssrc/typechecker/mod.rssrc/validator/mod.rstests/e2e/contracts/structs.arktests/e2e/static_arrays_test.gotests/e2e/structs_test.gotests/e2e/utils_test.gotests/examples/threshold_oracle.rstests/features.rstests/features/asset_introspection.rstests/features/contract_import_instantiation.rstests/features/group_properties.rstests/features/io_introspection.rstests/features/no_shadowing.rstests/features/opcode_functions.rstests/features/static_arrays.rstests/features/structs.rs
| /// Convert a placeholder path to an exported Go field name without losing | ||
| /// the distinction between path separators and source underscores. | ||
| pub fn to_go_field_name(s: &str) -> String { | ||
| if !s.contains('.') { | ||
| return to_pascal_case(s); | ||
| } | ||
|
|
||
| let mut result = String::new(); | ||
| for (index, ch) in s.chars().enumerate() { | ||
| match ch { | ||
| '.' => result.push_str("_D"), | ||
| '_' => result.push_str("_U"), | ||
| ch if index == 0 => result.push(ch.to_ascii_uppercase()), | ||
| ch => result.push(ch), | ||
| } | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether Arkade identifiers are case-sensitive and whether validation
# rejects case-only duplicate bindings before bindgen generation.
rg -n -C 4 'identifier|IDENT|duplicate|collision|case' \
src/parser/grammar.pest src/validator/mod.rs src/models/mod.rs
# Inspect Go naming call sites and existing bindgen coverage.
rg -n -C 3 'to_go_field_name|constructor_fields|user_fields' \
arkade-bindgen/src arkade-bindgen/testsRepository: arkade-os/compiler
Length of output: 48005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== arkade-bindgen/src/naming.rs =="
sed -n '1,180p' arkade-bindgen/src/naming.rs
echo
echo "== arkade-bindgen/src/targets/go.rs relevant field-name code =="
sed -n '45,145p' arkade-bindgen/src/targets/go.rs
sed -n '200,305p' arkade-bindgen/src/targets/go.rs
echo
echo "== behavioral probe for dotted Go field names =="
python3 - <<'PY'
from pathlib import Path
mod = Path('arkade-bindgen/src/naming.rs').read_text()
print('contains first-char uppercase at dots:', '_D' in mod and 'index == 0' in mod)
PY
python3 - <<'PY'
def to_pascal_case(s):
return ''.join(x.capitalize() if not i else x for i, x in enumerate(s.lower()))
def to_go_field_name(s):
if '.' not in s:
return to_pascal_case(s)
result = ''
for index, ch in enumerate(s):
if ch == '.':
result += '_D'
elif ch == '_':
result += '_U'
elif index == 0:
result += ch.upper()
else:
result += ch
return result
cases = ["value.a", "Value.a", "val_ue.A", "value.A", "Oracles.0", "oracles.0"]
for s in cases:
print(f'{s!r} -> {to_go_field_name(s)!r}')
print("collisions:", {to_go_field_name(s): s for s in cases})
PYRepository: arkade-os/compiler
Length of output: 11650
Preserve or reject initial-case duplicates before Go generation.
to_go_field_name encodes dots/underscores uniquely but uppercases the first character, so value.a and Value.a both become Value_Da. If both paths are valid Arkade identifiers, Go bindgen emits duplicate field names and generated code cannot compile. Preserve the original initial case in the Go field encoding, or reject such a collision in validation/binding before generation, and add an explicit regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@arkade-bindgen/src/naming.rs` around lines 55 - 72, Update to_go_field_name
so distinct initial-case paths such as value.a and Value.a remain distinct in
generated Go field names, or reject that collision during identifier
validation/binding before generation. Add an explicit regression test covering
both paths and ensure Go bindgen cannot emit duplicate field names.
Source: Coding guidelines
| function toGoFieldName(s) { | ||
| if (!s.includes('.')) return toPascalCase(s); | ||
| return [...s].map((ch, index) => { | ||
| if (ch === '.') return '_D'; | ||
| if (ch === '_') return '_U'; | ||
| return index === 0 ? ch.toUpperCase() : ch; | ||
| }).join(''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect identifier grammar and validation for standalone underscore names.
rg -n -C 4 'ident|identifier|IDENT' src/parser/grammar.pest
rg -n -C 4 '== "_"|!= "_"|name.*_' src/parser src/typechecker src/validator src/compiler || true
# Inspect all Go-name generation and value-access call sites.
rg -n -C 5 'toGoFieldName|toPascalCase|goValueExpr' playground/codegen.jsRepository: arkade-os/compiler
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## playground/codegen.js relevant section"
sed -n '1,130p' playground/codegen.js | cat -n
echo
echo "## all source matches for standalone underscore rejection/warning"
rg -n --hidden --glob '!node_modules' '\b["_][^"]*["]|"_"|single underscore|underscore-only|witness|constructor' src playground/**/*.rs playground/**/*.js playground/**/*.sh 2>/dev/null || true
echo
echo "## playground build script"
if [ -x playground/build.sh ]; then
cat -n playground/build.sh
else
echo "playground/build.sh not executable/missing"
fi
echo
echo "## behavioral probe for goName functions"
python3 - <<'PY'
def toPascalCase(s):
s = s.replace('.', '_').replace('-', '_')
parts = s.split('_')
return ''.join(part.capitalize() for part in parts if part)
def toGoFieldName(s):
if '.' not in s:
return toPascalCase(s)
return ''.join('_D' if ch == '.' else '_U' if ch == '_' else ch.upper() if index == 0 else ch for index, ch in enumerate(s))
print("toGoFieldName('_') =", repr(toGoFieldName('_')))
for s in ['_', 'a_', 'a.b', 'a_b']:
print(f"{s!r} -> PASCAL={toPascalCase(s)!r}, go={toGoFieldName(s)!r}")
PYRepository: arkade-os/compiler
Length of output: 32958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## validator source identifiers and source validation calls"
rg -n -C 6 'fn validate_source_identifier|validate_source_identifier\(|validate_source_name|source_name|parameters \. iter|tapscript inputs|input names|input parameters' src/validator/mod.rs
echo
echo "## codegen Go value expression and emitted Go fields"
rg -n -C 8 'function goValueExpr|const.*ConstructorParams|constructor fields|generated Go|goField' playground/codegen.js
echo
echo "## locate generated/committed playground Go bindings"
git ls-files playground | rg '\.go$|codegen\.js|contracts\.js|README\.md$' | sed -n '1,200p'Repository: arkade-os/compiler
Length of output: 5111
Reject standalone underscore source names as ABI parameters.
_ is accepted by the parser and can appear in the ABI as a constructor parameter or tapscript input. toGoFieldName('_') returns an empty string, so goValueExpr emits params. or w. and the generated Go binding does not compile. Add a compiler/validation error or reject the name in all source contexts; do not replace it with another Go-safe field name only in codegen.js.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@playground/codegen.js` around lines 46 - 52, The standalone underscore source
name must be rejected before code generation, across all source contexts
including constructor parameters and tapscript inputs. Add compiler/validation
handling so "_" cannot reach toGoFieldName or goValueExpr, and report a clear
error rather than substituting a Go-safe field name only in codegen.js.
Source: Coding guidelines
| if let Some(declared_type) = declared_type { | ||
| scope.extend(crate::typechecker::build_scope_with_structs( | ||
| &[Parameter { | ||
| name: name.clone(), | ||
| param_type: declared_type.clone(), | ||
| }], | ||
| &self.structs, | ||
| )); | ||
| } else { | ||
| scope.insert(name.clone(), t); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract one helper for let-binding scope seeding. Four sites repeat the same logic: take a let binding's declared type, otherwise its inferred type, then expand it through build_scope_with_structs when it names a struct, otherwise insert the name directly. The copies have already diverged: the src/compiler/concat.rs copy is missing the inferred-struct branch, which drops the field paths of an untyped native struct result and defeats the num2bin concat guard. Extract a single helper, for example seed_binding_scope(scope, name, declared_type, inferred, structs), and call it from all four sites.
src/compiler/concat.rs#L73-L83: add the inferred-struct branch, then replace the block with the shared helper solet p = ecAdd(...)registersp.xandp.y.src/typechecker/mod.rs#L267-L285: replace the declared-vs-inferred expansion block with the shared helper.src/validator/mod.rs#L452-L472: replace thewalk_asset_id_stmtsexpansion block with the shared helper.src/validator/mod.rs#L905-L948: collapse the two identicalbuild_scope_with_structsloops into one call to the shared helper, keeping theBindingSource::Localwrapper.
📍 Affects 3 files
src/compiler/concat.rs#L73-L83(this comment)src/typechecker/mod.rs#L267-L285src/validator/mod.rs#L452-L472src/validator/mod.rs#L905-L948
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compiler/concat.rs` around lines 73 - 83, The four let-binding
scope-seeding sites duplicate divergent logic; extract a shared helper that
selects the declared type or inferred type, expands struct types via
build_scope_with_structs, and otherwise inserts the binding directly. Update
src/compiler/concat.rs:73-83, src/typechecker/mod.rs:267-285, and
src/validator/mod.rs:452-472 to call it, including inferred struct fields such
as p.x and p.y; in src/validator/mod.rs:905-948, replace both duplicate loops
with one helper call while preserving the BindingSource::Local wrapper.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Seed struct field types for an untyped native struct initializer.
This branch expands the scope only when declared_type is present. For let p = ecAdd(x1, y1, x2, y2, curve); the declared type is absent, so the else branch inserts only p with the inferred type ArkType::Struct("ECPoint"). The field paths p.x and p.y never enter the scope.
A later expression that reads a field then resolves to ArkType::Unknown. In rewrite_expression_concat, is_numeric(&ArkType::Unknown) is false, so p.x + someBytes32 is rewritten into a Concat without the "convert it explicitly with num2bin(value, width)" error. The compiler then concatenates an integer field with bytes instead of rejecting the expression. The declared form ECPoint p = ecAdd(...) does not have this gap.
src/typechecker/mod.rs (lines 275-282) and src/validator/mod.rs (lines 924-939) already handle the inferred-struct case. Mirror that branch here.
🐛 Proposed fix
if let Some(declared_type) = declared_type {
scope.extend(crate::typechecker::build_scope_with_structs(
&[Parameter {
name: name.clone(),
param_type: declared_type.clone(),
}],
&self.structs,
));
+ } else if matches!(t, ArkType::Struct(_)) {
+ scope.extend(crate::typechecker::build_scope_with_structs(
+ &[Parameter {
+ name: name.clone(),
+ param_type: t.as_str(),
+ }],
+ &self.structs,
+ ));
} else {
scope.insert(name.clone(), t);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compiler/concat.rs` around lines 73 - 83, Update the untyped branch in
the surrounding scope-building logic to detect inferred struct types and seed
their field paths, matching the existing inferred-struct handling in the
typechecker and validator. Preserve the current direct insertion for non-struct
inferred types, and ensure an inferred ECPoint binding makes fields such as p.x
and p.y available for numeric validation in rewrite_expression_concat.
| if crate::models::is_builtin_type(declared_type) { | ||
| leaves.push((access_name.to_string(), value.clone())); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let definition = structs | ||
| .iter() | ||
| .find(|definition| definition.name == declared_type) | ||
| .ok_or_else(|| format!("unknown struct type '{declared_type}'"))?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Support native struct fields during recursive literal binding.
collect_struct_literal_leaves treats AssetId, Outpoint, and ECPoint as unknown types. is_builtin_type excludes these types, and the next lookup checks only user-defined structs.
The validator accepts a user-struct field such as AssetId id when its value is an assetId expression. Compilation then fails with unknown struct type 'AssetId'.
Extend the recursive binding path to use builtin_struct_fields. Bind the multi-item expression with the same stack-order logic as bind_native_struct. Add a regression test with a native result nested in a user struct.
As per coding guidelines, “treat security and compiler correctness as requirements” and “non-trivial logic requires the smallest runnable regression check that would fail if the logic broke.” <coding_guidelines>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compiler/mod.rs` around lines 953 - 961, Update
collect_struct_literal_leaves to recognize native struct types through
builtin_struct_fields before searching user-defined structs, and bind their
multi-item expressions using the same stack-order logic as bind_native_struct.
Preserve existing handling for ordinary builtins and user structs, and add a
minimal regression test covering an AssetId, Outpoint, or ECPoint result nested
in a user-defined struct.
Source: Coding guidelines
| // Any declared name (constructor param or tapscript input), any type. | ||
| let name_declared = |name: &str| -> bool { | ||
| contract.parameters.iter().any(|p| p.name == name) | ||
| || ts.inputs.iter().any(|p| p.name == name) | ||
| constructor_scope.contains_key(name) || ts.inputs.iter().any(|p| p.name == name) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict name_declared to scalar names.
constructor_scope now contains composite entries. insert_type_bindings inserts the container name with ArkType::Struct(..) or ArkType::Array(..) in addition to each scalar leaf. contains_key accepts those container entries, so older(policy) or hash160(preimage) == policy.primary passes this check when policy is a struct parameter and policy.primary is a nested struct.
The leaf assembly then emits <policy> or <policy.primary>. resolve_constructor_field_placeholders has no leaf whose access_name matches a container, so the token stays unresolved, and validate_output does not inspect leaf placeholders other than the signature-name heuristic. The result is an artifact with a placeholder that no caller can satisfy.
Filter out composite types so a composite operand fails at compile time.
🛡️ Proposed fix
// Any declared name (constructor param or tapscript input), any type.
let name_declared = |name: &str| -> bool {
- constructor_scope.contains_key(name) || ts.inputs.iter().any(|p| p.name == name)
+ matches!(
+ constructor_scope.get(name),
+ Some(binding_type)
+ if !matches!(binding_type, ArkType::Array(..) | ArkType::Struct(..))
+ ) || ts.inputs.iter().any(|p| p.name == name)
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Any declared name (constructor param or tapscript input), any type. | |
| let name_declared = |name: &str| -> bool { | |
| contract.parameters.iter().any(|p| p.name == name) | |
| || ts.inputs.iter().any(|p| p.name == name) | |
| constructor_scope.contains_key(name) || ts.inputs.iter().any(|p| p.name == name) | |
| }; | |
| // Any declared name (constructor param or tapscript input), any type. | |
| let name_declared = |name: &str| -> bool { | |
| matches!( | |
| constructor_scope.get(name), | |
| Some(binding_type) | |
| if !matches!(binding_type, ArkType::Array(..) | ArkType::Struct(..)) | |
| ) || ts.inputs.iter().any(|p| p.name == name) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compiler/tapscript.rs` around lines 273 - 276, The name_declared closure
in the tapscript name-validation flow must accept only scalar bindings. When
checking constructor_scope entries, inspect each binding’s ArkType and reject
Struct or Array container entries while preserving scalar constructor names and
tapscript inputs; ensure composite operands such as policy or nested
policy.primary fail compilation before leaf placeholder assembly.
| Some("scriptPubKey") => ArkType::Bytes, | ||
| Some("sequence") => ArkType::Int, | ||
| Some("outpoint") => ArkType::Bytes32, | ||
| Some("outpoint") => ArkType::Struct("Outpoint".to_string()), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate assertions for native struct results and their opcodes.
set -euo pipefail
fd -t f -e rs . tests | xargs rg -n -C 6 'outpoint|assetId|ECPoint|ecAdd|ecMul' || true
# Check for negative coverage of a direct composite comparison.
rg -n -C 4 'composite comparison' --glob '*.rs' || echo "no negative test for composite comparison"Repository: arkade-os/compiler
Length of output: 19004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== changed files/stat =="
git diff --stat || true
echo "== relevant typechecker excerpt =="
sed -n '580,690p' src/typechecker/mod.rs
echo "== comparison support implementation =="
rg -n -C 8 'composite comparison|require_local_types|require.*==' src tests --glob '*.rs' || true
echo "== feature assembly assertions around native result tests =="
for f in tests/features/io_introspection.rs tests/features/asset_introspection.rs tests/features/opcode_functions.rs; do
echo "--- $f"
sed -n '/fn test_input_outpoint_returns_struct/,$p' "$f" | sed -n '1,160p'
sed -n '/fn test_asset_at_assetid_returns_struct/,$p' "$f" | sed -n '1,160p'
sed -n '/fn test_ec_add_returns_typed_point/,$p' "$f" | sed -n '1,140p'
doneRepository: arkade-os/compiler
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact native result test bodies =="
sed -n '89,110p' tests/features/io_introspection.rs
sed -n '73,91p' tests/features/asset_introspection.rs
sed -n '241,259p' tests/features/opcode_functions.rs
sed -n '282,300p' tests/features/opcode_functions.rs
echo "== direct composite comparison rejection coverage =="
sed -n '342,360p' tests/features/general_comparisons.rs || true
rg -n -C 5 'composite comparison|direct .*comparison|array bindings cannot|struct' tests/features --glob '*.rs' || true
echo "== implementation of native struct assembly normalization =="
rg -n -C 12 'bind_native_struct|normalize.*struct|native result|AssetId|Outpoint|ECPoint|OP_SWAP|OP_0 OP_PICK' src --glob '*.rs' || true
echo "== structural comparison of tests =="
python3 - <<'PY'
from pathlib import Path
p = {'io_introspection':'tests/features/io_introspection.rs','asset_introspection':'tests/features/asset_introspection.rs','opcode_functions':'tests/features/opcode_functions.rs'}
for name, path in p.items():
text = Path(path).read_text()
print(f'--- {path}')
cases = []
import re
for m in re.finditer(r'fn test_([A-Za-z0-9_]+)\(([^)]*)\).*?^\}', text, re.S|re.M):
body = m.group(0)
cases.append((m.group(1), body))
for name_, body in cases:
print(name_, "contains OP_SWAP=", "OP_SWAP" in body, "contains OP_0 OP_PICK=", "OP_0 OP_PICK" in body)
PYRepository: arkade-os/compiler
Length of output: 50374
Add a negative case for native struct comparisons.
outpoint, assetId, and ecAdd are covered by native-assembly tests, and EC operations cover OP_0 OP_PICK struct field reordering. Add a regression for struct-native equality, such as require(tx.inputs[0].outpoint == expected), so the new composite comparison error is part of the feature coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/typechecker/mod.rs` at line 626, Add a negative native-assembly
regression test covering struct equality, using an expression such as
tx.inputs[0].outpoint == expected and asserting the typechecker reports the
composite comparison error. Place it alongside the existing native-assembly
tests covering outpoint, assetId, ecAdd, and struct field reordering; do not
change the ArkType mapping.
Source: Coding guidelines
| structs: &[crate::models::StructDefinition], | ||
| ) { | ||
| for parameter in parameters { | ||
| if let Some((element_type, length)) = crate::models::array_type_parts(¶meter.param_type) | ||
| { | ||
| let element_type = ArkType::parse(element_type); | ||
| frame.insert( | ||
| parameter.name.clone(), | ||
| BindingInfo { | ||
| binding_type: ArkType::Array(Box::new(element_type.clone()), length), | ||
| source, | ||
| }, | ||
| ); | ||
| for index in 0..length { | ||
| frame.insert( | ||
| format!("{}[{}]", parameter.name, index), | ||
| BindingInfo { | ||
| binding_type: element_type.clone(), | ||
| source, | ||
| }, | ||
| ); | ||
| } | ||
| } else { | ||
| frame.insert( | ||
| parameter.name.clone(), | ||
| BindingInfo { | ||
| binding_type: ArkType::parse(¶meter.param_type), | ||
| source, | ||
| }, | ||
| ); | ||
| } | ||
| let types = build_scope_with_structs(parameters, structs); | ||
| for (name, binding_type) in types { | ||
| frame.insert( | ||
| name, | ||
| BindingInfo { | ||
| binding_type, | ||
| source, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect contract-instance argument emission and its accepted expression forms.
set -euo pipefail
fd -t f -e rs . src | xargs ast-grep outline --match 'emit_contract_instance_asm' 2>/dev/null || true
rg -n -C 25 'fn emit_contract_instance_asm' --glob 'src/**/*.rs'
# Look for tests that pass a struct field as a constructor argument.
rg -n -C 6 'new \w+\(\w+\.' --glob 'tests/**/*.rs' || echo "no struct-field contract argument test found"Repository: arkade-os/compiler
Length of output: 1018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/compiler/expr.rs outline around emit_contract_instance_asm =="
wc -l src/compiler/expr.rs src/validator/mod.rs
sed -n '340,520p' src/compiler/expr.rs
echo
echo "== src/validator/mod.rs relevant expressions =="
rg -n -C 20 'fn validate_binding_expression|BindingSource::|ContractInstance|struct_literal|insert_parameters|build_scope_with_structs' src/validator/mod.rs
echo
echo "== model definitions =="
rg -n -C 5 'ContractInstance|StructLiteral|BindingSource|validate_output|Expression' src/models src/parser src/typechecker 2>/dev/null | head -n 300Repository: arkade-os/compiler
Length of output: 46954
Reject Property access inside Expression::ContractInstance arguments.
validate_binding_expression checks Expression::Variable source and passes value_position = false for Expression::ContractInstance. A function-input struct field such as candidate.primary.key is emitted as <VTXO:Foo(<candidate.primary.key>)> and can reach <VTXO:...> output validation, but candidate.primary.key itself is valid to the output validator and is skipped in post-checks. Reject computed contract-instance arguments here, or explicitly reject nested Expression::Property access under contract-instance arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/validator/mod.rs` around lines 703 - 713, Update
validate_binding_expression handling for Expression::ContractInstance to reject
arguments containing computed property access, including nested
Expression::Property expressions such as candidate.primary.key. Apply the
rejection during contract-instance argument validation rather than relying on
later output validation, while preserving validation for non-property arguments.
| for index, name := range expandInput(contract.ConstructorInputs[0], contract.Structs) { | ||
| if got, want := group.Arkade.ASM[index], "<"+name+">"; got != want { | ||
| t.Fatalf("constructor prologue token %d = %q, want %q", index, got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert the prologue length before indexing group.Arkade.ASM.
The loop indexes group.Arkade.ASM[index] for every expanded leaf name. If the compiler emits fewer prologue tokens than the flattened leaf count, this panics with an index-out-of-range error instead of reporting a readable assertion failure. Check the length first.
🛡️ Proposed fix
group := covenantGroup(t, contract, "spend")
- for index, name := range expandInput(contract.ConstructorInputs[0], contract.Structs) {
+ prologue := expandInput(contract.ConstructorInputs[0], contract.Structs)
+ if len(group.Arkade.ASM) < len(prologue) {
+ t.Fatalf("covenant asm has %d tokens, want at least %d constructor placeholders",
+ len(group.Arkade.ASM), len(prologue))
+ }
+ for index, name := range prologue {
if got, want := group.Arkade.ASM[index], "<"+name+">"; got != want {
t.Fatalf("constructor prologue token %d = %q, want %q", index, got, want)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for index, name := range expandInput(contract.ConstructorInputs[0], contract.Structs) { | |
| if got, want := group.Arkade.ASM[index], "<"+name+">"; got != want { | |
| t.Fatalf("constructor prologue token %d = %q, want %q", index, got, want) | |
| } | |
| } | |
| prologue := expandInput(contract.ConstructorInputs[0], contract.Structs) | |
| if len(group.Arkade.ASM) < len(prologue) { | |
| t.Fatalf("covenant asm has %d tokens, want at least %d constructor placeholders", | |
| len(group.Arkade.ASM), len(prologue)) | |
| } | |
| for index, name := range prologue { | |
| if got, want := group.Arkade.ASM[index], "<"+name+">"; got != want { | |
| t.Fatalf("constructor prologue token %d = %q, want %q", index, got, want) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/structs_test.go` around lines 41 - 45, Before the loop that indexes
group.Arkade.ASM, assert that its length matches the number of names returned by
expandInput for the constructor inputs. Keep the existing per-token comparison
unchanged, so insufficient prologue tokens produce a readable test failure
instead of an index-out-of-range panic.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
This PR adds named struct types, native multi-value builtin results (AssetId, Outpoint, ECPoint), and recursive scalar-leaf stack layouts. The design is largely sound and the test suite is broad. One correctness bug requires a fix before merge; several medium-severity issues are flagged below.
Must-fix
[BUG] Group-property keyword shadowing in parser — src/parser/expr.rs (identifier_property_access handler)
parse_primary_expr disambiguates a two-part dotted path from a struct field access by matching the second segment against a hardcoded keyword list:
if parts.len() == 2
&& matches!(parts[1], "numInputs" | ... | "assetId" | "isFresh" | ...)
{
return Ok(Expression::GroupProperty { ... });
}
Ok(Expression::Property(text))A struct with a field named any of these keywords — assetId, isFresh, hasControl, metadataHash, numInputs, numOutputs, sumInputs, sumOutputs, delta — will have that field misclassified as a GroupProperty for any exactly-two-segment access. assetId in particular is a completely natural field name for asset-aware structs, and the README already uses it as a concept.
Validator gap: check_struct_definitions in src/validator/mod.rs (the new validate_struct_fields / validate_declared_type path) validates field types and recursion but does not validate field names against the group-property keyword list. No validation error is produced for:
struct Token { int assetId; }
contract C(Token token) {
function f() { require(token.assetId >= 0); }
}
Runtime behaviour: validate_binding_expression for GroupProperty { group: "token", ... } finds "token" in scope (the struct root is tracked in the typechecker's scope map) and raises no issue. The compiler then emits <token> followed by OP_INSPECTASSETGROUPASSETID. lower_raw_token tries read_static_binding("token"), but the struct root is never pushed to the generator stack — only its leaf bindings are — so binding_index("token") returns None and the compiler fails with "undefined binding 'token'". No silent wrong-ASM is emitted, but the error is deliberately misleading.
Fix options (choose one):
- In
check_struct_definitions, add a check that field names do not equal any group-property keyword. Rejectstruct Token { int assetId; }with a clear message. - Eliminate the keyword list from
parse_primary_exprentirely and perform the disambiguation after the parser using scope information in the type-checker (riskier refactor, but architecturally cleaner).
Option 1 is the minimal safe fix.
High
bind_native_struct hardcoded OP_SWAP — src/compiler/mod.rs (~line 460)
if fields.len() != 2 {
return Err(format!("native struct '{declared_type}' has unsupported width {}", fields.len()));
}
self.emit_expression_items(expression, fields.len())?;
self.swap()?;
let start = self.stack.len() - fields.len();
for ((field, _), item) in fields.iter().rev().zip(&mut self.stack[start..]) {
*item = StackItem::Binding { name: ..., kind: BindingKind::Local };
}The guard against non-2-item structs is correct; the tests confirm the swap is correct for all four current native opcodes (ECADD, ECMUL, INSPECTINPUTOUTPOINT, INSPECTOUTASSETAT+DROP). The correctness invariant is: every native opcode must push its fields in forward declaration order (first declared field deeper, last on top) so that the single OP_SWAP puts the second field deeper and the first on top before the reversed-iteration assignment.
This invariant is not enforced anywhere and is not stated in the builtin_struct_fields doc comment in src/models/mod.rs. Any future opcode that pushes fields in reverse order (top-of-stack = first field, as some opcodes do) would silently compile with swapped field names and no test breakage until values are inspected at runtime.
Recommendation: add a doc comment to builtin_struct_fields (and to bind_native_struct) stating the required opcode-output order, and add a compile-time assertion or test for each native struct binding.
Medium
Removed check_expanded_namespace without replacement — src/validator/mod.rs
The function is deleted (it was the only check for placeholder-namespace collisions after array expansion). The rationale — that switching from _ to . as separator makes cross-parameter collisions impossible — is sound: source identifiers cannot contain ., so a.b.c is unambiguous regardless of how a, b, c are individually named. No bug here, but the removal is non-obvious; a brief comment in validate_ast would aid future maintainers.
Duplicate literal-shape validation — src/validator/mod.rs:validate_struct_literal vs src/compiler/mod.rs:collect_struct_literal_leaves
Both sites re-implement the "field must be present exactly once, array must match declared size" constraints. If they diverge, a contract could pass validation and then fail (or vice versa) at compile time with an opaque internal error. Consider making the compiler unconditionally defer to the validator by running validation early enough to be fatal, or extracting the canonical shape check into a single shared function in src/models/mod.rs.
Breaking ABI change: _N → .N placeholder suffix
Array element placeholders change from oracles_0 to oracles.0 in emitted ASM and all bindgen output. This is a well-scoped breaking change internal to the compiler and arkade-bindgen crates (no consumed JSON artifacts with hardcoded underscore-indexed names were found in ts-sdk, go-sdk, rust-sdk, or dotnet-sdk). Confirm all downstream systems that snapshot or embed compiler artifact JSON are updated before deploying.
Low / Notes
playground/codegen.js:expandFields — no depth guard (playground/codegen.js ~line 82)
function expandFields(name, typeStr, isInjected, structs = []) {
...
const definition = structs.find(definition => definition.name === typeStr);
if (definition) {
return definition.fields.flatMap(field =>
expandFields(`${name}.${field.name}`, field.type, isInjected, structs)
);
}
...
}The JS playground client-side expansion has no cycle guard. Valid compiled artifacts never contain recursive structs (the validator rejects them), but a hand-crafted artifact JSON submitted to the playground would cause unbounded recursion and a stack-overflow crash. The Rust flatten_parameter has the stack-based cycle check; mirror it here with a visitedTypes set parameter.
validate_local_types does not walk VarAssign — src/validator/mod.rs
validate_local_types only checks LetBinding { declared_type: Some(...) } branches; VarAssign to a struct field (e.g., local.primary.weight = x) is not inspected here. This is safe because validate_binding_statements covers VarAssign fully, but the split responsibility is fragile.
Forward struct reference test — tests/features/structs.rs:forward_struct_references_are_allowed
Confirmed: struct Outer { Inner inner; } declared before struct Inner { int value; } correctly compiles because flatten_parameter resolves against the collected definitions, not declaration order. This is good language behaviour; the test is minimal and should be kept.
Size warning
2871 changed lines across grammar, parser, type-checker, compiler, validator, bindgen, playground, and E2E tests is large for a single review. The changes are logically coherent, but a future split (grammar+parser / type system / compiler+validator / bindgen) would reduce review risk.
Summary
Request changes for the group-property keyword shadowing bug (must be caught by the validator before reaching the parser disambiguation). All other findings are improvements rather than blockers, but the bind_native_struct invariant documentation and the playground depth guard are strongly recommended before shipping.
2 participants
Summary
AssetId,Outpoint, andECPointstructsImpact
Contracts can group related values into named, nestable types while retaining the existing one-stack-item-per-scalar ABI. Artifact consumers can recursively flatten these values from the published schema, and fixed-width builtin results can now be handled as typed values instead of loose stack outputs.
Validation
cargo test --workspacecargo clippy --workspace --all-targets -- -D warningscargo fmt --check./playground/build.sh./scripts/e2e.shSummary by CodeRabbit