Implement symbolic stack management - #72
Conversation
WalkthroughThe PR replaces direct ASM emission with symbolic stack compilation. It adds dynamic array indexing and explicit multisig signatures, strengthens binding and ABI validation, updates covenant interfaces, disables unsupported Taproot transitions, and expands compiler and end-to-end tests. ChangesCompiler and covenant behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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:
|
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 introduces a correct and well-reasoned symbolic stack model. The core Generator design, scope lifecycle, and finish() cleanup are sound. However, several issues require changes before merge, with two being protocol-level blockers.
BLOCKER 1 — repayment_pool.ark: five more functions silently emit wrong assembly
Only issue is commented out (examples/bonds/repayment_pool.ark:78–137 pr-side). The other five functions — acceptRepayment, rollOut, rollIn, liquidate, acceptAuction — all reconstruct contracts using runtime values (function inputs such as borrowerPk, auctioneerPk, or local variables such as usdtNext, creditNext):
repayment_pool.ark:143,148—acceptRepaymentpassesborrowerPk(function input) intonew BondMint(...)andnew SingleSig(borrowerPk, exit).repayment_pool.ark:185,193—rollOutsame pattern.repayment_pool.ark:226—rollInsame pattern.repayment_pool.ark:289,344,363—liquidatepassesauctioneerPk(function input) intonew SingleSig(auctioneerPk, exit)andnew BondMint(...).repayment_pool.ark:368,423,442—acceptAuctionsame.
Under the new model, lower_raw_token accepts any <VTXO:…> string as a temporary (compiler/mod.rs new lower_raw_token VTXO branch). The VTXO token is generated by emit_contract_instance_asm and will embed the literal name of the runtime binding (e.g. <VTXO:BondMint:borrowerPk:…>). That name cannot be resolved at instantiation time — the README's own new text says "Function inputs and computed locals exist only at spend time and cannot be resolved inside an opaque <VTXO:…> placeholder." Compilation succeeds, spending fails silently.
Every test guarding these functions is #[ignore = "dynamic contract reconstruction is temporarily disabled"] (tests/examples/repayment_pool.rs:16,55,125,…). CI will not catch this.
Required action: comment out or gate all six functions, or add a compile-time error when a VTXO placeholder references a non-constructor binding, before merging.
BLOCKER 2 — Protocol mismatch: auctioneer output constraint in co-spent liquidate/acceptAuction
bond_mint.ark:126,145 now enforces tx.outputs[1].scriptPubKey == auctioneerScriptPubKey (bytes parameter — any script). repayment_pool.ark:344,363,423,442 still enforces tx.outputs[1].scriptPubKey == new SingleSig(auctioneerPk, exit) with pubkey auctioneerPk. These two functions are co-spent in the same transaction. The effective constraint is still SingleSig, but the two contracts now speak different languages at the API boundary. bond_mint.ark advertises "caller picks any destination" while repayment_pool.ark imposes SingleSig. Any client serialising the witness will use the BondMint ABI (bytes) without realising the pool side requires a pubkey-derivable address.
If the intent is to move both sides to opaque bytes, update repayment_pool.ark:liquidate and acceptAuction to match. If the intent is to keep SingleSig, revert the BondMint change. Either way, document the invariant in a comment.
SERIOUS — normalized_name divergence between compiler and validator
src/validator/mod.rs:553–565 (normalized_name): when the bracket content is not a literal integer (e.g. arr[i]) it falls back to "arr_0". The compiler's normalize_binding_name (src/compiler/mod.rs new code, normalize_binding_name fn) uses the content verbatim, so arr[i] → arr_i. A contract that passes the validator with arr[i] (mapped to the in-scope arr_0) will then fail at codegen with undefined binding 'arr_i'. The compiler error message will be cryptic. Either both should use "0" as the canonical fallback, or both should reject non-literal indices at parse/validation time.
SERIOUS — CheckSig/CheckSigFromStack argument order reversal is a breaking ASM change
src/compiler/expr.rs:149 (old): CheckSigExpr pushed <pubkey> then <signature> → stack [pubkey, sig] (sig on top). New code pushes <signature> then <pubkey> → [sig, pubkey] (pubkey on top).
src/compiler/expr.rs:157–165 (old): CheckSigFromStackExpr pushed [message, pubkey, signature] (sig on top). New code pushes [signature, message, pubkey] (pubkey on top).
The new orderings align with Elements-style opcodes (CHECKSIG: pubkey on top; CHECKSIGFROMSTACK: pubkey on top over message over sig). This is almost certainly a bug-fix, not a regression. But it is a breaking change to emitted ASM for every existing contract that uses checkSig or checkSigFromStack as an expression (not just as a require). Any previously cached ASM artifacts or clients that rely on the prior opcode operand order will break silently. Confirm against the ArkadeScript opcode spec, and add a BREAKING note to the PR description.
MODERATE — 34 new #[ignore] annotations across protocol-critical tests
Ignoring tests that enforce:
- RepaymentPool burn-equality invariant (
test_all_burn_checks_are_strict_equality) - Phase disjointness of liquidate/acceptAuction (
test_liquidate_and_accept_auction_are_phase_disjoint) - CoveredCall and CashSecuredPut reconstruction tests
roundtrip_repayment_pool
These are the guards against solvency-breaking regressions. A follow-up issue or PR with a concrete timeline for re-enabling them is needed before this lands in main.
MINOR — assign alt-stack counter is global
src/compiler/mod.rs new Generator::assign: self.alt_depth is a single counter on Generator. The final if self.alt_depth != 0 { return Err(…) } check assumes no other path pushes to the alt-stack. If a future opcode emitter (e.g. a multi-return introspection) also uses OP_TOALTSTACK, this check will trip spuriously. Consider scoping the counter to the assign call (a local accumulator compared to the pre-call value) rather than asserting the global counter is zero.
MINOR — Missing validation that CheckMultisig key/sig arrays have equal length
The src/parser/checksig.rs parser (new) requires a sig array to be present but does not check pubkeys.len() == signatures.len(). The check happens later in codegen (src/compiler/mod.rs new CheckMultisig arm: if pubkeys.len() != signatures.len()). The validator's check_binding_semantics does not check this. A mismatch produces a codegen-phase error with no source location. Move the check to the validator so it can report a line-attributed error.
OBSERVATION — repayment_pool comment understates the scope of disabled functions
examples/bonds/repayment_pool.ark:72: "Pool transitions need dynamic taproot reconstruction." Only issue is shown as disabled, but acceptRepayment, rollOut, rollIn, liquidate, and acceptAuction have the same blocker. Update the comment to list all affected functions so the next reader does not assume the rest compile correctly.
|
@arkana-ai-bot ignores all the contract examples being merged till now, that will get fixed in future PR once Compiler work ended |
|
Understood, thanks @tiero. To be clear on what that context changes vs. what stands: What it changes: What it does not change:
Happy to re-review once the follow-up scope is scoped out. |
2 is fixed. Other issues deferred. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/bonds/bond_mint.ark (1)
40-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe stated pairing guarantees no longer hold.
Lines 43-57 describe how each vault function is paired with a pool function that constrains the same output. Lines 47-50 state that
pool.liquidatepinsoutput[1]=auctioneerand "health-gates via oracle, so a healthy vault cannot be liquidated".
examples/bonds/repayment_pool.arknow comments outliquidate,acceptAuction,acceptRepayment,issue,rollOut,rollIn, andredeem. The only enabled pool function isdisabled(), which always fails.The safety argument in this header now references pool functions that do not exist. The vault paths fail closed today because no pool input can be spent, so this is a documentation-versus-code divergence rather than an exploitable hole. Update the header to state that the pool counterparts are temporarily disabled, and reference the restoration note in
examples/bonds/repayment_pool.arkLines 72-74.🤖 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 `@examples/bonds/bond_mint.ark` around lines 40 - 59, Update the pairing-guarantee header in the bond mint example to state that the referenced repayment-pool counterparts are temporarily disabled and vault paths currently fail closed. Remove or qualify claims about active pool functions such as pool.liquidate and rollOut, and point readers to the restoration note around the repayment pool’s disabled-function section.
🧹 Nitpick comments (11)
src/compiler/mod.rs (1)
241-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate operand kinds in
nipandswap.
nipandswapcheck only the stack height. They do not check that the affected items areStackItem::Temporary.nipremoves the item below the top. If that item is aBinding, the binding disappears from the symbolic stack and from the real stack, and every later depth computation for the remaining bindings is wrong.No current caller reaches this state: the expression paths that emit
OP_NIPoperate on two results of the same expression, andfinishremoves bindings on purpose. The gap is thatnipcannot distinguish these two cases, so a future emission path can corrupt the stack model without any error.Add a temporary-operand check to
nipandswap, and givefinisha separate frame-collapse helper.♻️ Proposed separation of the two `nip` roles
fn nip(&mut self) -> Result<(), String> { if self.stack.len() < 2 { return Err("internal compiler error: OP_NIP stack underflow".to_string()); } let index = self.stack.len() - 2; + if !matches!(self.stack[index], StackItem::Temporary) { + return Err( + "internal compiler error: OP_NIP would discard a binding".to_string(), + ); + } self.stack.remove(index); self.asm.push(OP_NIP.to_string()); Ok(()) } + /// Collapse the covenant frame: drop everything under the final result. + fn nip_frame_slot(&mut self) -> Result<(), String> { + if self.stack.len() < 2 { + return Err("internal compiler error: OP_NIP stack underflow".to_string()); + } + let index = self.stack.len() - 2; + self.stack.remove(index); + self.asm.push(OP_NIP.to_string()); + Ok(()) + } + fn swap(&mut self) -> Result<(), String> { if self.stack.len() < 2 { return Err("internal compiler error: OP_SWAP stack underflow".to_string()); } let len = self.stack.len(); + if self.stack[len - 2..] + .iter() + .any(|item| !matches!(item, StackItem::Temporary)) + { + return Err( + "internal compiler error: OP_SWAP expected two temporary operands".to_string(), + ); + } self.stack.swap(len - 2, len - 1); self.asm.push(OP_SWAP.to_string()); Ok(()) }Then use
nip_frame_slotinfinish:self.push_temporary(OP_1); while self.stack.len() > 1 { - self.nip()?; + self.nip_frame_slot()?; }🤖 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 241 - 259, Update nip and swap to validate that all affected stack entries are StackItem::Temporary before mutating the symbolic stack, returning an internal compiler error otherwise. Add a separate nip_frame_slot helper for intentional frame-slot removal, and update finish to use it so bindings can still be collapsed without weakening nip’s operand validation.tests/features/opcode_functions.rs (1)
8-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the duplicated
contains_tokenshelper into a shared test module. Both files define an identical privatecontains_tokensfunction for ordered ASM-token subsequence matching. Extract it once intotests/common/mod.rs, alongside the existingarkade_asm_tokenshelper, and import it from both test modules.
tests/features/opcode_functions.rs#L8-L15: remove the localcontains_tokensdefinition and import the shared helper fromtests/common.tests/features/symbolic_stack.rs#L15-L22: remove the localcontains_tokensdefinition and import the shared helper fromtests/common.🤖 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/opcode_functions.rs` around lines 8 - 15, The duplicated contains_tokens helper should be centralized in tests/common/mod.rs alongside arkade_asm_tokens. In tests/features/opcode_functions.rs lines 8-15 and tests/features/symbolic_stack.rs lines 15-22, remove the local definitions and import the shared contains_tokens helper from tests/common.src/validator/mod.rs (4)
216-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also reserving
EMULATOR_KEY.
validate_source_identifierrejects onlySERVER_KEY. The compiler also emits anEMULATOR_KEYplaceholder role for synthesized collaborative leaves. A source identifier namedEMULATOR_KEYis accepted today and emits<EMULATOR_KEY>into the ABI namespace, which is confusable with the reserved<EMULATOR_KEY:funcName>token.♻️ Proposed extension
fn validate_source_identifier(name: &str, context: &str, issues: &mut Vec<ValidationIssue>) { - if name == "SERVER_KEY" { + if name == "SERVER_KEY" || name == "EMULATOR_KEY" { issues.push(ValidationIssue::error(format!( "{context} '{name}' uses a compiler-reserved placeholder name" ))); } }As per coding guidelines: "use
<SERVER_KEY>and<EMULATOR_KEY:funcName>for the reserved co-signer roles".🤖 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 216 - 222, Update validate_source_identifier to reject both SERVER_KEY and EMULATOR_KEY as compiler-reserved placeholder names, preserving the existing ValidationIssue error behavior and message context for either identifier.Source: Coding guidelines
1468-1479: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist
expected_prologueout of the group loop.
expected_prologuedepends only onoutput.parameters. The loop at Line 1454 iterates over spend groups, so this rebuilds the vector and allocates oneStringper constructor parameter on every iteration. Compute it once before the loop.♻️ Proposed refactor
+ let expected_prologue = output + .parameters + .iter() + .rev() + .map(|parameter| format!("<{}>", parameter.name)) + .collect::<Vec<_>>(); + for group in &output.functions {- let expected_prologue = output - .parameters - .iter() - .rev() - .map(|parameter| format!("<{}>", parameter.name)) - .collect::<Vec<_>>(); if !arkade.asm.starts_with(&expected_prologue) {🤖 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 1468 - 1479, Move the expected_prologue construction out of the spend-group loop and compute it once before the loop, since it depends only on output.parameters. Keep the existing reverse iteration, formatting, and starts_with validation unchanged, and reuse the hoisted value for each group.
1649-1673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit test covers the new prologue validation.
make_outputsetsparameters: vec![]andarkade: None. The new constructor-prologue check and the new placeholder-outside-prologue check at Lines 1468-1490 are therefore never reached byvalidate_outputunit tests. The existing tests only cover empty ASM and the leaf signature leak.Add cases for a valid reversed prologue, a wrong-order prologue, a bare
<name>placeholder after the prologue, and an exempt<VTXO:...>placeholder after the prologue.Do you want me to generate these test cases?
🤖 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 1649 - 1673, Add unit-test coverage in the validator tests using make_output and validate_output for the new prologue rules: configure parameters and arkade as needed to exercise a valid reversed constructor prologue and a wrong-order prologue, then add cases for a bare <name> placeholder after the prologue and an exempt <VTXO:...> placeholder after it. Assert the valid and exempt cases pass, while wrong ordering and bare placeholders are rejected.
926-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the composite-value checks.
Five sequential
if value_position && matches!(...)blocks push four near-identical messages. Three of them produce the same "produces 2 stack items; composite values are not supported" text.Two of these strings are also duplicated verbatim in
emit_expressioninsrc/compiler/mod.rs(Lines 368-408): the outpoint message and the asset-group input message. Tests assert on the exact text, so a future edit in one module silently diverges from the other.Consider one helper that maps an expression to an optional rejection reason, and share the message constants with the compiler.
🤖 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 926 - 1003, The sequential composite-value checks around expression validation should be consolidated into a single helper that returns an optional rejection reason for each unsupported expression, preserving the existing precedence and exact messages. Define shared message constants for the duplicated outpoint and asset-group input diagnostics, and update both this validation logic and emit_expression to use them so test-visible text cannot diverge.examples/arkade_kitties/arkade_kitties.ark (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
single_sig.arkis no longer used.
transferwas the last user ofSingleSig. Line 88 now compares the output script directly. No remaining statement referencesSingleSig.
examples/controlled_mint/controlled_mint.arkremoved itssingle_sig.arkimport in the same change for the same reason. Remove it here for consistency.-import "single_sig.ark"; -🤖 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 `@examples/arkade_kitties/arkade_kitties.ark` at line 15, Remove the unused single_sig.ark import from the arkade_kitties example. Update only the import section, leaving the direct output-script comparison and remaining contract logic unchanged.examples/fuji_safe/fuji_safe.ark (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe P2TR verification logic is now duplicated.
verifyTreasuryFujiBurninghardcodestreasuryPk.redeeminlines the same three statements withborrowerPk. Lines 30-31 and Lines 71-72 are identical apart from the key.The previous parameterized helper served all three call sites. Replacing it with a key-specific helper plus one inlined copy means a future change to the burning rule must be applied in two places.
If the symbolic stack model cannot pass a
pubkeyargument to an internal function, state that constraint in a comment above Line 25 so the duplication is not later "cleaned up" into a broken form.examples/bonds/repayment_pool.arkLines 72-74 records the same private-function limitation.Note one latent hazard: internal functions are inlined, so the helper's local
p2trScriptenters each caller's scope.redeemdeclares its ownp2trScriptat Line 70. Ifredeemever also calls the helper, the two declarations collide and the new shadowing check insrc/validator/mod.rsLine 617 rejects the contract.Also applies to: 70-72
🤖 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 `@examples/fuji_safe/fuji_safe.ark` around lines 24 - 32, Restore a single parameterized helper for the shared P2TR output verification used by verifyTreasuryFujiBurning and redeem, passing the applicable treasuryPk or borrowerPk instead of duplicating the three statements. If the symbolic stack model cannot pass a pubkey to an internal function, document that limitation above verifyTreasuryFujiBurning and retain the duplication explicitly; otherwise ensure the helper’s inlined local p2trScript cannot collide with redeem’s local declaration.examples/bonds/repayment_pool.ark (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the restoration note to a tracking issue.
The note states the restoration condition: private functions plus EC return decomposition, or a generic builtin. It does not give an owner, an issue number, or a timeline.
Roughly 400 lines of solvency-critical logic are now commented out across Lines 82-491. The PR objectives also record a reviewer request for "a concrete re-enable timeline" on the ignored solvency tests.
Add the tracking issue reference here.
Do you want me to open an issue that captures the restoration condition and lists every disabled function?
🤖 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 `@examples/bonds/repayment_pool.ark` around lines 72 - 75, Update the restoration note near the disabled pool transition functions to include a reference to the tracking issue that owns re-enabling them. Use the issue identifier for the solvency logic and ignored tests, while preserving the stated restoration conditions and avoiding changes to the commented-out functions.examples/layerzero/endpoint.ark (1)
196-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the documented parameter name with the code.
Line 198 names the value
oappReceiveScriptHash. The constructor field added at Line 100 and passed at Line 206 isreceiveMarkerScriptHash. The comment describes theReceiveMarkerside and the code shows theEndpointside, so both names are defensible, but a reader must infer that they are the same value.State the mapping explicitly, for example: "pins itself to the OApp.receive() closure via its constructor parameter
oappReceiveScriptHash, supplied here as the Endpoint'sreceiveMarkerScriptHash, which is fixed when the Endpoint is deployed."🤖 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 `@examples/layerzero/endpoint.ark` around lines 196 - 207, Update the comment above the receive-invocation marker validation to explicitly state that the ReceiveMarker constructor parameter oappReceiveScriptHash is supplied by the Endpoint’s receiveMarkerScriptHash value, while preserving the deployment-time fixed-value description.examples/options/cash_secured_put.ark (1)
87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the functions disabled by the missing dynamic taproot reconstruction. Three example contracts comment out the functions that rebuild a contract script with
new <Contract>(...), and their tests are ignored with the reason "dynamic contract reconstruction is temporarily disabled". The restore condition appears only as prose in each file, with no shared tracking reference.
examples/options/cash_secured_put.ark#L87-L89: link a tracking issue fortransferSellerandtransferBuyer, and reference the ignored tests intests/examples/cash_secured_put.rs.examples/options/covered_call.ark#L90-L92: link the same tracking issue fortransferSellerandtransferBuyer.examples/stability/stability_offer.ark#L38-L41: link the same tracking issue fortake, and note that disabling it removes the only path that opens aStabilityVault.Do you want me to open one tracking issue that lists every disabled function and every ignored test across these files?
🤖 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 `@examples/options/cash_secured_put.ark` around lines 87 - 89, Track the missing dynamic taproot reconstruction work with one shared issue reference. In examples/options/cash_secured_put.ark lines 87-89, reference that issue for transferSeller and transferBuyer and link the ignored tests in tests/examples/cash_secured_put.rs; in examples/options/covered_call.ark lines 90-92, reference it for transferSeller and transferBuyer; and in examples/stability/stability_offer.ark lines 38-41, reference it for take and note that disabling it removes the only path opening a StabilityVault.
🤖 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 `@examples/arkade_kitties/arkade_kitties.ark`:
- Line 75: The transfer function’s owner signature is verified against
caller-supplied keys and destination data, so ownership is not authenticated.
Update transfer to bind authorization to on-chain ownership, preferably by
deriving the expected owner key or script from the spent input’s current
scriptPubKey, and use that bound value for signature and destination validation
instead of ownerPk/newOwnerScriptPubKey; if this example intentionally remains
incomplete, document the limitation in the contract.
In `@examples/bonds/bond_mint.ark`:
- Around line 128-129: Replace the auctioneer output value checks in both
settlement paths, near the existing output 1 script checks, with the Taproot
dust-floor condition requiring at least 330 sats. Update both occurrences
consistently while preserving the existing auctioneer script validation.
In `@examples/bonds/repayment_pool.ark`:
- Around line 493-495: The RepaymentPool example is permanently unspendable
because disabled() is its only enabled function. Add a prominent warning at the
top of the contract identifying it as a non-deployable placeholder, and add a
standalone unilateral(signature ownerSig) tapscript using the existing exit and
owner symbols with CSV enforcement and signature validation.
In `@examples/stability/stability_vault.ark`:
- Around line 46-49: Update the disabled-function comment block in
StabilityVault to explicitly state that the contract must not be deployed while
settlement functions remain disabled, since unilateral releases all collateral
to seekerPk. Replace the vague “unsupported runtime bindings” wording with the
specific runtime bindings that must be supported before restoring providerExit,
removeCapital, and seekerExit.
In `@examples/threshold_oracle/threshold_oracle.ark`:
- Line 25: Bind the oracle attestation to the payout in the threshold validation
flow around attest: require messageHash to equal the hash of the encoded amount
concatenated with recipientScriptPubKey before accepting the quorum. Apply the
same commitment at the corresponding payout path so callers cannot substitute
either destination or amount.
In `@src/compiler/mod.rs`:
- Around line 399-406: Update the expected error strings in the
ecMulScalarVerify and tweakVerify tests to match emit_expression’s actual
message, “expression produces {n} stack items; exactly one is required,” while
preserving the existing test scenarios and assertions.
In `@src/validator/mod.rs`:
- Around line 617-621: Remove the duplicate shadowing diagnostics from
check_binding_semantics, including both binding and loop-variable cases, while
preserving insertion of each binding into the scope for downstream lookups. Keep
the corresponding diagnostics exclusively in walk_scope, matching the existing
VarAssign delegation pattern.
- Around line 896-911: Update the `Requirement::After` and
`Requirement::HashEqual` validation arms to skip `validate_named_binding` when
`timelock_var`, `preimage`, or `hash` is an integer literal, while retaining the
existing binding validation for named operands and the `After` no-binding case.
- Around line 674-681: Update the if-condition validation around
resolved_expression_type so ArkType::Unknown is accepted alongside ArkType::Bool
and does not produce a diagnostic; continue reporting errors for all other
inferred types.
---
Outside diff comments:
In `@examples/bonds/bond_mint.ark`:
- Around line 40-59: Update the pairing-guarantee header in the bond mint
example to state that the referenced repayment-pool counterparts are temporarily
disabled and vault paths currently fail closed. Remove or qualify claims about
active pool functions such as pool.liquidate and rollOut, and point readers to
the restoration note around the repayment pool’s disabled-function section.
---
Nitpick comments:
In `@examples/arkade_kitties/arkade_kitties.ark`:
- Line 15: Remove the unused single_sig.ark import from the arkade_kitties
example. Update only the import section, leaving the direct output-script
comparison and remaining contract logic unchanged.
In `@examples/bonds/repayment_pool.ark`:
- Around line 72-75: Update the restoration note near the disabled pool
transition functions to include a reference to the tracking issue that owns
re-enabling them. Use the issue identifier for the solvency logic and ignored
tests, while preserving the stated restoration conditions and avoiding changes
to the commented-out functions.
In `@examples/fuji_safe/fuji_safe.ark`:
- Around line 24-32: Restore a single parameterized helper for the shared P2TR
output verification used by verifyTreasuryFujiBurning and redeem, passing the
applicable treasuryPk or borrowerPk instead of duplicating the three statements.
If the symbolic stack model cannot pass a pubkey to an internal function,
document that limitation above verifyTreasuryFujiBurning and retain the
duplication explicitly; otherwise ensure the helper’s inlined local p2trScript
cannot collide with redeem’s local declaration.
In `@examples/layerzero/endpoint.ark`:
- Around line 196-207: Update the comment above the receive-invocation marker
validation to explicitly state that the ReceiveMarker constructor parameter
oappReceiveScriptHash is supplied by the Endpoint’s receiveMarkerScriptHash
value, while preserving the deployment-time fixed-value description.
In `@examples/options/cash_secured_put.ark`:
- Around line 87-89: Track the missing dynamic taproot reconstruction work with
one shared issue reference. In examples/options/cash_secured_put.ark lines
87-89, reference that issue for transferSeller and transferBuyer and link the
ignored tests in tests/examples/cash_secured_put.rs; in
examples/options/covered_call.ark lines 90-92, reference it for transferSeller
and transferBuyer; and in examples/stability/stability_offer.ark lines 38-41,
reference it for take and note that disabling it removes the only path opening a
StabilityVault.
In `@src/compiler/mod.rs`:
- Around line 241-259: Update nip and swap to validate that all affected stack
entries are StackItem::Temporary before mutating the symbolic stack, returning
an internal compiler error otherwise. Add a separate nip_frame_slot helper for
intentional frame-slot removal, and update finish to use it so bindings can
still be collapsed without weakening nip’s operand validation.
In `@src/validator/mod.rs`:
- Around line 216-222: Update validate_source_identifier to reject both
SERVER_KEY and EMULATOR_KEY as compiler-reserved placeholder names, preserving
the existing ValidationIssue error behavior and message context for either
identifier.
- Around line 1468-1479: Move the expected_prologue construction out of the
spend-group loop and compute it once before the loop, since it depends only on
output.parameters. Keep the existing reverse iteration, formatting, and
starts_with validation unchanged, and reuse the hoisted value for each group.
- Around line 1649-1673: Add unit-test coverage in the validator tests using
make_output and validate_output for the new prologue rules: configure parameters
and arkade as needed to exercise a valid reversed constructor prologue and a
wrong-order prologue, then add cases for a bare <name> placeholder after the
prologue and an exempt <VTXO:...> placeholder after it. Assert the valid and
exempt cases pass, while wrong ordering and bare placeholders are rejected.
- Around line 926-1003: The sequential composite-value checks around expression
validation should be consolidated into a single helper that returns an optional
rejection reason for each unsupported expression, preserving the existing
precedence and exact messages. Define shared message constants for the
duplicated outpoint and asset-group input diagnostics, and update both this
validation logic and emit_expression to use them so test-visible text cannot
diverge.
In `@tests/features/opcode_functions.rs`:
- Around line 8-15: The duplicated contains_tokens helper should be centralized
in tests/common/mod.rs alongside arkade_asm_tokens. In
tests/features/opcode_functions.rs lines 8-15 and
tests/features/symbolic_stack.rs lines 15-22, remove the local definitions and
import the shared contains_tokens helper from tests/common.
🪄 Autofix (Beta)
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: ab0efd10-612e-4a01-b7e8-6a9e07848a0c
📒 Files selected for processing (56)
README.mdexamples/arkade_kitties/arkade_kitties.arkexamples/bonds/bond_mint.arkexamples/bonds/repayment_pool.arkexamples/controlled_mint/controlled_mint.arkexamples/fuji_safe/fuji_safe.arkexamples/layerzero/endpoint.arkexamples/layerzero/oapp.arkexamples/nft_mint/nft_mint.arkexamples/non_interactive_swap/non_interactive_swap.arkexamples/options/cash_secured_put.arkexamples/options/covered_call.arkexamples/stability/stability_offer.arkexamples/stability/stability_vault.arkexamples/threshold_oracle/threshold_oracle.arksrc/compiler/concat.rssrc/compiler/expr.rssrc/compiler/loops.rssrc/compiler/mod.rssrc/models/mod.rssrc/opcodes/mod.rssrc/parser/checksig.rssrc/parser/expr.rssrc/parser/grammar.pestsrc/parser/mod.rssrc/typechecker/mod.rssrc/validator/mod.rstests/e2e/contracts/symbolic_stack.arktests/e2e/htlc_test.gotests/e2e/symbolic_stack_test.gotests/e2e/utils_test.gotests/examples/arkade_kitties.rstests/examples/asm_structural.rstests/examples/bond_mint.rstests/examples/cash_secured_put.rstests/examples/compilation_roundtrip.rstests/examples/controlled_mint.rstests/examples/covered_call.rstests/examples/repayment_pool.rstests/examples/stability_vault.rstests/examples/threshold_oracle.rstests/features.rstests/features/asset_id_explicit.rstests/features/asset_introspection.rstests/features/beacon.rstests/features/concat_op.rstests/features/contract_import_instantiation.rstests/features/general_comparisons.rstests/features/group_properties.rstests/features/io_introspection.rstests/features/no_shadowing.rstests/features/opcode_functions.rstests/features/symbolic_stack.rstests/features/threshold_multisig.rstests/features/type_system.rstests/features/validation_error.rs
|
|
||
| // Transfer a kitty to a new owner | ||
| function transfer(bytes32 kittyIdTxid, int kittyIdGidx, pubkey newOwnerPk, signature ownerSig, pubkey ownerPk) { | ||
| function transfer(bytes32 kittyIdTxid, int kittyIdGidx, bytes newOwnerScriptPubKey, signature ownerSig, pubkey ownerPk) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
The owner check does not authenticate the owner.
transfer accepts ownerPk as a function parameter (Line 75). Line 89 verifies ownerSig against that same caller-supplied ownerPk. A caller supplies any keypair and passes the check.
Before this change, the destination was pinned to new SingleSig(newOwnerPk, exit), which at least constrained the output shape. Line 88 now compares against a fully caller-supplied newOwnerScriptPubKey. A caller can therefore transfer any kitty to any script.
The kitty owner must be bound to on-chain state. Two options:
- Derive the authorized key from the spent input, for example by pinning
tx.input.current.scriptPubKey. - Make the owner key a constructor parameter so it is fixed at deployment.
The test test_transfer_uses_owner_authorized_destination_script in tests/examples/arkade_kitties.rs asserts OP_CHECKSIG is present and describes the input as "owner-authorized". That assertion does not verify the key is authenticated.
If the example contracts are intentionally incomplete, track this separately and add a comment in the file that states the limitation.
Also applies to: 88-89
🤖 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 `@examples/arkade_kitties/arkade_kitties.ark` at line 75, The transfer
function’s owner signature is verified against caller-supplied keys and
destination data, so ownership is not authenticated. Update transfer to bind
authorization to on-chain ownership, preferably by deriving the expected owner
key or script from the spent input’s current scriptPubKey, and use that bound
value for signature and destination validation instead of
ownerPk/newOwnerScriptPubKey; if this example intentionally remains incomplete,
document the limitation in the contract.
| require(tx.outputs[1].value > 0, "auctioneer output empty"); | ||
| require(tx.outputs[1].scriptPubKey == new SingleSig(auctioneerPk, exit), "output 1 not auctioneer"); | ||
| require(tx.outputs[1].scriptPubKey == auctioneerScriptPubKey, "output 1 not auctioneer"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the 330-sat dust threshold for the auctioneer output.
Both settlement paths check tx.outputs[1].value > 0. A Taproot output below 330 sats is not viable. The current check accepts a 1-sat auctioneer output, which is the exact siphon shape the comment at Lines 114-119 tries to block.
Change both checks to the Taproot dust floor.
🐛 Proposed fix
- require(tx.outputs[1].value > 0, "auctioneer output empty");
+ require(tx.outputs[1].value >= 330, "auctioneer output below dust");Apply the same change at Line 147.
As per coding guidelines: "Use 330 sats for Taproot dust checks: >= 330 for viable outputs and > 330 for dust-branch decisions."
Also applies to: 147-148
🤖 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 `@examples/bonds/bond_mint.ark` around lines 128 - 129, Replace the auctioneer
output value checks in both settlement paths, near the existing output 1 script
checks, with the Taproot dust-floor condition requiring at least 330 sats.
Update both occurrences consistently while preserving the existing auctioneer
script validation.
Source: Coding guidelines
| function disabled() { | ||
| require(0 == 1, "temporarily disabled"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
disabled() makes the contract permanently unspendable.
require(0 == 1, ...) never passes. disabled() is the only enabled function in RepaymentPool. Any value locked to this contract is unrecoverable, because the contract also declares no unilateral-exit tapscript.
This file is an example that users copy. Add a prominent warning at the top of the contract that states the contract is a non-deployable placeholder. Consider also adding the standard unilateral CSV tapscript so the shape stays deployable and recoverable:
// Unilateral exit (CSV).
function unilateral(signature ownerSig) tapscript {
require(older(exit));
require(checkSig(ownerSig, owner));
}
The contract already declares int exit at Line 69, so the parameter is available.
As per coding guidelines: "Unilateral exits must use a standalone CSV tapscript with require(older(exit)); require(checkSig(ownerSig, owner));".
🤖 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 `@examples/bonds/repayment_pool.ark` around lines 493 - 495, The RepaymentPool
example is permanently unspendable because disabled() is its only enabled
function. Add a prominent warning at the top of the contract identifying it as a
non-deployable placeholder, and add a standalone unilateral(signature ownerSig)
tapscript using the existing exit and owner symbols with CSV enforcement and
signature validation.
Source: Coding guidelines
| // State transitions need dynamic taproot reconstruction, and settlement | ||
| // currently hits unsupported runtime bindings. Restore the disabled functions | ||
| // once private functions plus EC return decomposition can implement them, or | ||
| // once a generic builtin provides the same verification. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
State that this vault is not deployable while the settlement functions are disabled.
Every covenant function in StabilityVault is now commented out. The only active function is unilateral at Lines 356-359, and it releases the whole UTXO under seekerPk after the exit delay.
totalCollateral holds both the seeker's claim backing and the provider's added collateral. providerExit, removeCapital, and seekerExit are all disabled, so the provider has no path to recover their share. A deployed vault would pay the full collateral to the seeker.
Add an explicit note in this comment block that the contract must not be deployed in this state. Also name the "unsupported runtime bindings" so the restore condition is actionable.
📝 Proposed comment update
+ // WARNING: Do not deploy this contract in its current state. Every covenant
+ // function is disabled, so the only spend path is the seeker's `unilateral`
+ // CSV exit. That path pays the full `totalCollateral` to `seekerPk` and
+ // leaves the provider with no recovery path.
+ //
// State transitions need dynamic taproot reconstruction, and settlement
// currently hits unsupported runtime bindings. Restore the disabled functions
// once private functions plus EC return decomposition can implement them, or
// once a generic builtin provides the same verification.🤖 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 `@examples/stability/stability_vault.ark` around lines 46 - 49, Update the
disabled-function comment block in StabilityVault to explicitly state that the
contract must not be deployed while settlement functions remain disabled, since
unilateral releases all collateral to seekerPk. Replace the vague “unsupported
runtime bindings” wording with the specific runtime bindings that must be
supported before restoring providerExit, removeCapital, and seekerExit.
| int amount, | ||
| bytes32 messageHash, | ||
| pubkey recipientPk, | ||
| bytes recipientScriptPubKey, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The oracle quorum does not commit to the payout destination.
attest verifies the quorum against messageHash at Line 32. It never binds messageHash to amount or to recipientScriptPubKey. Both values stay free caller inputs. Any caller who can present a valid quorum over any messageHash can therefore direct amount tokens at output 1 to a script of their choice.
This weakness is pre-existing. The previous code pinned the destination to new SingleSig(recipientPk, exit), but recipientPk was equally uncommitted, so this change does not introduce it.
The contract is a documented example of threshold attestation, so readers will copy this shape. Bind the attested message to the payout, for example by requiring messageHash == sha256(num2bin(amount, 8) + recipientScriptPubKey).
Also applies to: 40-40
🤖 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 `@examples/threshold_oracle/threshold_oracle.ark` at line 25, Bind the oracle
attestation to the payout in the threshold validation flow around attest:
require messageHash to equal the hash of the encoded amount concatenated with
recipientScriptPubKey before accepting the quorum. Apply the same commitment at
the corresponding payout path so callers cannot substitute either destination or
amount.
| if self.stack.len() != before + 1 | ||
| || !matches!(self.stack.last(), Some(StackItem::Temporary)) | ||
| { | ||
| return Err(format!( | ||
| "expression produces {} stack items; exactly one is required", | ||
| self.stack.len().saturating_sub(before) | ||
| )); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the emit_expression error text with the substrings asserted in tests.
set -euo pipefail
# Test: Show the compiler error text.
rg -n -C 3 'exactly one is required|does not produce one stack item' src/
# Test: Show every test assertion on this message.
rg -n -C 5 'does not produce one stack item|stack items' tests/Repository: arkade-os/compiler
Length of output: 6743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/compiler/mod.rs around emit_expression =="
sed -n '340,415p' src/compiler/mod.rs | cat -n
echo
echo "== src/compiler/mod.rs opcode lookup context =="
sed -n '300,335p' src/compiler/mod.rs | cat -n
echo
echo "== tests/features/opcode_functions.rs around verify assertions =="
sed -n '315,370p' tests/features/opcode_functions.rs | cat -n
echo
echo "== validator similar error context =="
sed -n '950,980p' src/validator/mod.rs | cat -n
echo
echo "== compile helper =="
sed -n '1,85p' tests/features/opcode_functions.rs | cat -nRepository: arkade-os/compiler
Length of output: 11676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all compile() definitions/usages =="
rg -n "fn compile\\(|pub fn compile\\(|use .*compile|compile\\(" src compiler tests -g '*.rs' | head -120
echo
echo "== all exact verify assertions =="
rg -n 'expression does not produce one stack item|expression produces .* stack items; exactly one is required' . -g '*.rs'Repository: arkade-os/compiler
Length of output: 9829
Update the failing assertion text.
emit_expression returns expression produces {n} stack items; exactly one is required, while the two verify tests assert on expression does not produce one stack item. The compiler message is already used by the other value-binding tests, so update the ecMulScalarVerify and tweakVerify assertions to the actual message.
🤖 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 399 - 406, Update the expected error
strings in the ecMulScalarVerify and tweakVerify tests to match
emit_expression’s actual message, “expression produces {n} stack items; exactly
one is required,” while preserving the existing test scenarios and assertions.
| let condition_type = resolved_expression_type(condition, scopes); | ||
| if condition_type != ArkType::Bool { | ||
| issues.push(ValidationIssue::error(format!( | ||
| "function '{}': if condition has type '{}', expected bool; use an explicit comparison", | ||
| function_name, | ||
| condition_type.as_str() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept ArkType::Unknown for if conditions.
This check rejects every condition whose type is not exactly Bool. resolved_expression_type returns ArkType::Unknown whenever infer_type cannot model the expression. A valid boolean condition that infer_type does not cover therefore fails compilation with "if condition has type 'unknown', expected bool".
Every other new type check in this file treats Unknown as unprovable and skips the diagnostic: the LetBinding declared-type check guards on inferred != ArkType::Unknown (Line 606), the VarAssign check guards on both sides (Lines 653-654), validate_named_binding guards on binding.binding_type != ArkType::Unknown (Line 791), and the array-index check accepts ArkType::Int | ArkType::Unknown (Line 1024). Make the if condition check consistent.
🐛 Proposed fix
let condition_type = resolved_expression_type(condition, scopes);
- if condition_type != ArkType::Bool {
+ if !matches!(condition_type, ArkType::Bool | ArkType::Unknown) {
issues.push(ValidationIssue::error(format!(
"function '{}': if condition has type '{}', expected bool; use an explicit comparison",
function_name,
condition_type.as_str()
)));
}📝 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.
| let condition_type = resolved_expression_type(condition, scopes); | |
| if condition_type != ArkType::Bool { | |
| issues.push(ValidationIssue::error(format!( | |
| "function '{}': if condition has type '{}', expected bool; use an explicit comparison", | |
| function_name, | |
| condition_type.as_str() | |
| ))); | |
| } | |
| let condition_type = resolved_expression_type(condition, scopes); | |
| if !matches!(condition_type, ArkType::Bool | ArkType::Unknown) { | |
| issues.push(ValidationIssue::error(format!( | |
| "function '{}': if condition has type '{}', expected bool; use an explicit comparison", | |
| function_name, | |
| condition_type.as_str() | |
| ))); | |
| } |
🤖 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 674 - 681, Update the if-condition
validation around resolved_expression_type so ArkType::Unknown is accepted
alongside ArkType::Bool and does not produce a diagnostic; continue reporting
errors for all other inferred types.
| Requirement::After { | ||
| timelock_var: Some(name), | ||
| .. | ||
| } => validate_named_binding( | ||
| name, | ||
| Some(ArkType::Int), | ||
| "timelock", | ||
| function_name, | ||
| scopes, | ||
| issues, | ||
| ), | ||
| Requirement::After { .. } => {} | ||
| Requirement::HashEqual { preimage, hash, .. } => { | ||
| validate_named_binding(preimage, None, "preimage", function_name, scopes, issues); | ||
| validate_named_binding(hash, None, "hash", function_name, scopes, issues); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Literal operands are rejected for After and HashEqual.
validate_named_binding requires the name to resolve through find_binding. It reports "is undefined" when the name is not an in-scope binding.
The compiler accepts literals in exactly these three positions. generate_requirement_asm in src/compiler/mod.rs calls read_binding_or_integer(var) for After.timelock_var, and read_binding_or_integer(preimage) plus read_binding_or_integer(hash) for HashEqual. The _or_integer suffix shows an integer literal is a supported operand there.
Source that passes an integer literal in these positions now fails validation before the compiler ever sees it. Skip the binding check when the operand parses as an integer.
🐛 Proposed fix
+fn validate_named_binding_or_integer(
+ name: &str,
+ expected: Option<ArkType>,
+ label: &str,
+ function_name: &str,
+ scopes: &BindingScopes,
+ issues: &mut Vec<ValidationIssue>,
+) {
+ if name.parse::<i64>().is_ok() {
+ return;
+ }
+ validate_named_binding(name, expected, label, function_name, scopes, issues);
+} Requirement::After {
timelock_var: Some(name),
..
- } => validate_named_binding(
+ } => validate_named_binding_or_integer(
name,
Some(ArkType::Int),
"timelock",
function_name,
scopes,
issues,
),
Requirement::After { .. } => {}
Requirement::HashEqual { preimage, hash, .. } => {
- validate_named_binding(preimage, None, "preimage", function_name, scopes, issues);
- validate_named_binding(hash, None, "hash", function_name, scopes, issues);
+ validate_named_binding_or_integer(
+ preimage, None, "preimage", function_name, scopes, issues,
+ );
+ validate_named_binding_or_integer(hash, None, "hash", function_name, scopes, issues);
}Run the following script to confirm the compiler-side literal support and to find affected sources:
#!/bin/bash
# Description: Confirm read_binding_or_integer semantics and find literal timelock/hash operands.
set -euo pipefail
rg -n -C 15 'fn read_binding_or_integer' src/
# Sources that may pass literals into after()/hash-equality requirements.
rg -n --glob '*.ark' -C 2 'older\(|after\(|sha256\(|hash160\(|hash256\(|ripemd160\(' examples tests🤖 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 896 - 911, Update the `Requirement::After`
and `Requirement::HashEqual` validation arms to skip `validate_named_binding`
when `timelock_var`, `preimage`, or `hash` is an integer literal, while
retaining the existing binding validation for named operands and the `After`
no-binding case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/features/contract_import_instantiation.rs`:
- Around line 432-453: Expand test_array_constructor_arg_is_flattened to assert
output.parameters contains owners_0, owners_1, and owners_2, then compare the
complete covenant ASM with arkade.asm and each tapleaf ASM with its
corresponding leaves[].asm rather than checking only one VTXO placeholder. Add a
wrong-arity constructor case that verifies compilation fails as expected.
🪄 Autofix (Beta)
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: 725f8a9b-e15a-422c-831d-feb65c44cbff
📒 Files selected for processing (6)
src/compiler/mod.rssrc/parser/grammar.pestsrc/validator/mod.rstests/features/contract_import_instantiation.rstests/features/no_shadowing.rstests/features/symbolic_stack.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/features/no_shadowing.rs
- src/validator/mod.rs
- src/parser/grammar.pest
- tests/features/symbolic_stack.rs
- src/compiler/mod.rs
| #[test] | ||
| fn test_array_constructor_arg_is_flattened() { | ||
| let code = r#" | ||
| import "threshold_oracle.ark"; | ||
|
|
||
| contract Forwarder(pubkey[] owners) { | ||
| function forward() { | ||
| require(tx.outputs[0].scriptPubKey == new ThresholdOracle(owners)); | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| let output = compile(code).expect("array constructor argument"); | ||
| let placeholder = arkade_asm_tokens(&output, "forward") | ||
| .into_iter() | ||
| .find(|token| token.starts_with("<VTXO:")) | ||
| .expect("VTXO placeholder"); | ||
| assert_eq!( | ||
| placeholder, | ||
| "<VTXO:ThresholdOracle(<owners_0>,<owners_1>,<owners_2>)>" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Assert the expanded ABI and both ASM paths.
Lines [445]-[452] verify only one VTXO placeholder. The test can pass while owners_0–owners_2 are absent from the emitted ABI or while other covenant and tapleaf instructions are incorrect. Assert the expanded output.parameters, compare complete arkade.asm and leaves[].asm, and add a wrong-arity edge case.
As per coding guidelines, tests under tests/**/*.rs must cover happy and edge paths and compare covenant ASM with arkade.asm and tapleaf ASM with leaves[].asm.
🤖 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/contract_import_instantiation.rs` around lines 432 - 453,
Expand test_array_constructor_arg_is_flattened to assert output.parameters
contains owners_0, owners_1, and owners_2, then compare the complete covenant
ASM with arkade.asm and each tapleaf ASM with its corresponding leaves[].asm
rather than checking only one VTXO placeholder. Add a wrong-arity constructor
case that verifies compilation fails as expected.
Source: Coding guidelines
3 participants
What changed
Why
Stack handling previously depended on positional assumptions spread across compiler paths. Those assumptions became fragile as expressions rearranged values and as control flow merged. A shared symbolic representation lets compilation and validation reason about the value at each stack position and reject invalid transitions consistently.
The follow-up commit also fixes dynamic contract destinations so destination expressions are handled correctly under the symbolic stack model.
Impact
Contract authors get consistent stack behavior and earlier validation errors. Existing examples and tests have been updated for the emitted assembly changes.
Validation
Regression coverage is included in
tests/features/symbolic_stack.rsandtests/e2e/symbolic_stack_test.go, alongside updated compiler, feature, and example tests.Summary by CodeRabbit
New Features
Bug Fixes
Changes