Rebase asset introspection to current emulator ABI#46
Conversation
Playground PreviewA live preview of this PR's playground is available at:
|
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — PR #46: Rebase asset introspection to current emulator ABI
Reviewer: Arkana (automated, protocol-critical — requires human sign-off)
Verdict: Request changes (one deferred-but-dangerous typing issue, plus human sign-off required for protocol-critical asset introspection changes)
Summary
Well-crafted PR. The two-operand (txid, gidx) ABI migration is thorough — grammar, parser, compiler emission, type checker, validator, loop unrolling, all examples, and 15 new tests. The opcode sequences are correct, the stack analysis is sound, and the cross-repo surface is clean (no downstream consumers of the old single-arg API were found).
The new validator additions (shadowing, expanded-namespace collision, scope-aware operand type checking with 0..65535 range enforcement) are a significant quality improvement.
🟢 Correct — verified by stack trace
| Construct | Stack sequence | Result |
|---|---|---|
lookup(txid, gidx) |
[amount, flag] → OP_VERIFY |
Leaves amount, aborts if absent ✓ |
has(txid, gidx) |
[amount, flag] → OP_NIP |
Leaves flag (Bool), drops amount ✓ |
find(txid, gidx) |
[k, flag] → OP_VERIFY |
Leaves k, aborts if absent ✓ |
assetGroups.has(txid, gidx) |
[k, flag] → OP_NIP |
Leaves flag, drops k ✓ |
controlIs(txid, gidx) |
[ctrl_txid, ctrl_gidx, flag] → OP_DROP → compare gidx → OP_SWAP → compare txid → OP_BOOLAND |
Bool, no abort on mismatch ✓ |
hasControl |
[ctrl_txid, ctrl_gidx, flag] → OP_NIP OP_NIP |
Leaves flag ✓ |
bare find |
find → OP_DROP OP_1 |
Correctly handles k==0 ✓ |
The sentinel guard removal (OP_DUP OP_1NEGATE OP_EQUAL OP_NOT OP_VERIFY → just OP_VERIFY) is correct for the new (result, success_flag) ABI.
controlIs correctly returns false (not abort) when control is absent because empty_bytes ≠ valid_bytes32_txid.
🟡 Issue: .assetId is a silent footgun (deferred but dangerous)
Files: src/compiler/mod.rs:1985-1988, src/compiler/mod.rs:2205-2213, src/typechecker/mod.rs:459, src/typechecker/mod.rs:475
Both asset_at.assetId and group.assetId leave TWO stack items [txid, gidx] but are typed as ArkType::Bytes32 (a single item). This means:
require(group.assetId == someBytes32);
…would compile without error but only compare gidx (top of stack) against the bytes32, leaving txid orphaned on the stack. This is silently wrong — it doesn't abort, it just produces incorrect results.
The TODO comments are present and the issue is explicitly deferred, but:
- There is no compile-time guard preventing someone from writing this comparison today. The validator should reject
== someBytes32on.assetIduntil theAssetIdstruct type lands. - The orphaned
txidon the stack would corrupt subsequent opcode operations.
Recommendation: Add a validator rule that emits a fatal error if .assetId appears in a comparison (Requirement::Comparison) until TODO(asset-id-struct) is resolved. This is a one-line check and prevents a class of silent bugs that could cause real fund loss. If this is too much for this PR, at minimum add a test that asserts .assetId == x is rejected.
🟢 Good: Validator additions
check_asset_id_operands: scope-aware, handleslet/var/for/if-elsecorrectly, range-checks literal gidx0..=65535✓check_shadowing: lexical scope stack with frame push/pop for blocks ✓check_expanded_namespace: catchesint[] xsvsint xs_0collisions ✓check_ctor_assignment: immutability enforcement for constructor params ✓reject_malformed_asset_call: catches legacy single-arg forms that fall through the grammar ✓
🟢 Good: Grammar & parser
- PEG rules correctly enforce two-arg syntax; single-arg and three-arg forms fail to parse ✓
identifierfor txid (prevents numeric literals as txid) ✓(identifier | number_literal)for gidx (allows both) ✓parse_asset_group_id_operandshandles bothasset_group_accessandtx_property_accesspaths ✓
🟢 Good: Loop unrolling
substitute_expression correctly recurses all three operands (index, asset_txid, asset_gidx) for AssetLookup, AssetHas, GroupFind, GroupHas, and GroupControlIs — including the group name substitution for GroupControlIs. ✓
🟢 Good: Test coverage
15 new tests covering: flag consume, bare-find k==0 handling, has/controlIs/hasControl ASM verification, minimal ScriptNum encoding, parser rejection (legacy single-arg, extra arg), fatal validation (swapped operands, out-of-range gidx), loop index as gidx. All critical paths are tested. ✓
🟡 Note: Dependency on emulator PR#97
PR description correctly notes this is an ABI-breaking change tracking arkade-os/emulator#97 (still OPEN as of this review). The merge ordering matters — emulator first, then compiler. The "deployed-VTXO-descriptor inventory" check mentioned in the PR description should be documented as a merge gate.
🟢 Cross-repo impact: None
Searched all repos under /root/arkana/repos/ for assets.lookup, assetGroups.find, .control ==, collect_lookup_asset_ids, decompose_constructor_params, and the compiler crate as a dependency. No downstream consumers found. The old API surface is fully internal to this repo.
Verdict
The code is high quality and the migration is comprehensive. The opcode sequences are correct. However, this is protocol-critical code (asset introspection directly governs VTXO handling and exit paths), so:
- Requesting changes for the
.assetIdtyping footgun — add a validator guard or a test proving it's rejected. - Human sign-off required per protocol-critical review rules, independent of code quality.
- Ensure
emulator#97merges first.
cc @msinkec
WalkthroughThe PR migrates the Arkade Script compiler from single ChangesAsset ID (txid, gidx) migration and success-flag lookup semantics
Sequence Diagram(s)sequenceDiagram
participant Source as .ark Source
participant Parser as Parser (grammar.pest + mod.rs)
participant TypeCheck as Typechecker
participant Validator as Validator
participant Compiler as Compiler (mod.rs)
participant JSON as Compiled JSON/ASM
Source->>Parser: lookup(txid, gidx) / find(txid, gidx) / controlIs(txid, gidx)
Parser->>Parser: parse_asset_id_txid + parse_asset_id_gidx operands
Parser-->>TypeCheck: Expression::AssetLookup{asset_txid, asset_gidx}
TypeCheck-->>Validator: AssetHas → Bool, GroupControlIs → Bool
Validator->>Validator: check_asset_id_operands (txid=Bytes32, gidx=Int 0..65535)
Validator->>Validator: check_shadowing + check_expanded_namespace
Validator-->>Compiler: validated AST
Compiler->>Compiler: emit_asset_lookup_asm → INSPECTINASSETLOOKUP + OP_VERIFY
Compiler->>Compiler: emit_group_control_is_asm → INSPECTGROUPCONTROL + OP_BOOLAND
Compiler-->>JSON: ASM with camelCase placeholders, raw-32/scriptnum witness encodings
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Arkana Incremental Review — commit 3e08aa17
Scope: "Remove unused const + update doc" — cleanup commit pushed after initial review.
Changes reviewed
-
src/opcodes/mod.rs: RemovesOP_1NEGATEandOP_DUPconstants. Verified no remaining references in any.rsfile on the PR branch — these were already eliminated fromcompiler/mod.rsand both test files in earlier commits. ✅ -
docs/arkade-primitives-spec.md: Two changes:- Stack depth comments (line ~339): Replaced the verbose/confused "Recount: …Wait, mintAmt IS top" annotation with a clean 2-line comment. Correct —
mintAmt(0), ctrlIn(1), msg(2), amt(3), soOP_3 OP_PICKcopiesamt. ✅ - Opcode count table (line ~501): Phases 6/7/9 reduced by 4 opcodes each (total 102→90, script size 427→415). Math checks out: removing the old 5-opcode sentinel guard
OP_DUP OP_1NEGATE OP_EQUAL OP_NOT OP_VERIFYand replacing with 1-opcodeOP_VERIFY= -4 per phase × 3 phases = -12. ✅
- Stack depth comments (line ~339): Replaced the verbose/confused "Recount: …Wait, mintAmt IS top" annotation with a clean 2-line comment. Correct —
Previous issue status
The .assetId typing footgun flagged in my initial review (.assetId leaves 2 stack items but is typed Bytes32) is not addressed in this commit. That remains the outstanding requested change before approval.
No new issues found in this commit.
There was a problem hiding this comment.
Arkana Incremental Review — commit d048dbed
Scope: "remove redundant comments and asserts" — cleanup commit.
Changes reviewed
| File | What changed |
|---|---|
src/compiler/mod.rs |
Shortened comments on decompose_constructor_params (asset ID context removed, now just "array flattening") |
src/parser/grammar.pest |
Shortened group_property comment re: .control removal |
src/validator/mod.rs |
Shortened check_expanded_namespace comment |
tests/asset_id_explicit_test.rs |
Removed OP_1NEGATE absence assertion; simplified error message string |
tests/bond_mint_test.rs |
Shortened comment |
tests/cash_secured_put_test.rs |
Removed _txid/_gidx absence assertions |
tests/covered_call_test.rs |
Same as above |
tests/fee_adapter_test.rs |
Shortened comment |
tests/no_shadowing_test.rs |
Removed NOTE comment about deleted collision test |
Verification
- Confirmed
OP_1NEGATEandcollect_lookup_asset_idsare completely absent fromsrc/— the removed negative assertions were testing for absence of already-deleted code. ✅ - Confirmed the old implicit
_txid/_gidxgenerated-name pattern is gone fromsrc/— only the new explicitasset_txid/asset_gidxoperand fields exist. ✅ - No functional code changes. No behavioral changes.
Outstanding item (unchanged)
The .assetId typing footgun flagged in my initial review remains tracked as TODO(asset-id-struct) and is explicitly out-of-scope per the PR description. No objection to deferring it.
No new issues found. Previous review stance holds — protocol-critical, requires human sign-off before merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
examples/fee_adapter.ark (1)
11-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
int exitconstructor parameter per coding guidelines.As per coding guidelines: "All contracts must use
options { server = server; exit = exit; }... and includeint exit... as constructor parameters so the playground can parameterize them".🛠️ Suggested fix
options { server = server; - exit = 144; + exit = exit; } contract FeeAdapter( pubkey senderPk, pubkey operatorPk, pubkey recipientPk, bytes32 paymentAssetIdTxid, int paymentAssetIdGidx, - int minFee + int minFee, + int exit ) {🤖 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/fee_adapter.ark` around lines 11 - 23, The FeeAdapter contract is missing the int exit constructor parameter required by coding guidelines. Add int exit as a parameter to the FeeAdapter constructor definition (after the minFee parameter). Additionally, update the options block to change exit = 144; to exit = exit; so that the exit value is parameterized from the constructor argument instead of being hardcoded.Source: Coding guidelines
examples/controlled_mint.ark (1)
12-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
int exitconstructor parameter per coding guidelines.Similar to other example contracts, this should include
int exitas a constructor parameter so the playground can parameterize the exit timelock.As per coding guidelines: "All contracts must use
options { server = server; exit = exit; }... and includeint exit... as constructor parameters so the playground can parameterize them".🛠️ Suggested fix
options { server = server; - exit = 288; + exit = exit; } contract ControlledMint( bytes32 tokenAssetIdTxid, int tokenAssetIdGidx, bytes32 ctrlAssetIdTxid, int ctrlAssetIdGidx, - pubkey issuerPk + pubkey issuerPk, + int exit ) {🤖 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/controlled_mint.ark` around lines 12 - 23, The ControlledMint contract constructor is missing the int exit parameter and the options block contains a hardcoded exit value instead of a parameterized one. Add int exit as a constructor parameter to the ControlledMint function (after the issuerPk parameter), and update the options block to use exit = exit; instead of the hardcoded exit = 288; to allow the playground to parameterize the exit timelock per coding guidelines.Source: Coding guidelines
examples/arkade_kitties.ark (1)
15-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
int exitconstructor parameter per coding guidelines.The guidelines require example contracts to include
int exitas a constructor parameter so the playground can parameterize the exit timelock. Currently, the exit timelock is hardcoded to 576.As per coding guidelines: "All contracts must use
options { server = server; exit = exit; }... and includeint exit... as constructor parameters so the playground can parameterize them".🛠️ Suggested fix
options { server = server; - exit = 576; // ~4 days for breeding timeout + exit = exit; } contract ArkadeKitties( bytes32 speciesControlIdTxid, int speciesControlIdGidx, - pubkey oraclePk + pubkey oraclePk, + int exit ) {🤖 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.ark` around lines 15 - 24, Add the missing `int exit` constructor parameter to the ArkadeKitties contract constructor signature alongside the existing parameters speciesControlIdTxid, speciesControlIdGidx, and oraclePk. Then update the options block to use `exit = exit;` instead of the hardcoded `exit = 576;` so the exit timelock value can be parameterized by the playground as required by the coding guidelines.Source: Coding guidelines
examples/non_interactive_swap.ark (1)
14-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
int exitconstructor parameter per coding guidelines.As per coding guidelines: "All contracts must use
options { server = server; exit = exit; }... and includeint exit... as constructor parameters so the playground can parameterize them".🛠️ Suggested fix
options { // Server key is injected from getInfo(), not passed in constructor server = server; // Exit delay: 144 blocks (~24 hours) for unilateral recovery - exit = 144; + exit = exit; } contract NonInteractiveSwap( // Maker's public key (creator of the swap offer) pubkey makerPk, // Asset ID the maker is offering (locked in this contract) bytes32 offerAssetIdTxid, int offerAssetIdGidx, // Amount of offer asset locked int offerAmount, // Asset ID the maker wants to receive bytes32 wantAssetIdTxid, int wantAssetIdGidx, // Amount of want asset required for the swap int wantAmount, // Expiration timestamp (block height) after which maker can cancel - int expirationTime + int expirationTime, + int exit ) {🤖 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/non_interactive_swap.ark` around lines 14 - 36, The NonInteractiveSwap contract constructor is missing the required `int exit` parameter as specified in the coding guidelines. Add `int exit` as a constructor parameter (positioned after the expirationTime parameter) to allow the playground to parameterize the exit delay, and update the options block to use the passed `exit` parameter value instead of the hardcoded value 144.Source: Coding guidelines
examples/token_vault.ark (1)
11-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOptions block uses hardcoded exit value instead of parameterized
exit.Per coding guidelines, contracts must use
options { server = server; exit = exit; }and includeint exitas a constructor parameter. This contract hardcodesexit = 144.Proposed fix
options { server = server; - exit = 144; + exit = exit; } contract TokenVault( pubkey ownerPk, bytes32 tokenAssetIdTxid, int tokenAssetIdGidx, bytes32 ctrlAssetIdTxid, - int ctrlAssetIdGidx + int ctrlAssetIdGidx, + int exit ) {🤖 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/token_vault.ark` around lines 11 - 14, The options block in the token_vault.ark contract has a hardcoded exit value of 144 instead of using a parameterized variable. Change the exit assignment in the options block from `exit = 144;` to `exit = exit;` to use the parameter value, and add `int exit` as a constructor parameter to the contract so the exit value can be dynamically passed in rather than being hardcoded.Source: Coding guidelines
examples/threshold_oracle.ark (1)
13-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOptions block uses hardcoded exit value instead of parameterized
exit.Per coding guidelines for
.arkcontracts, all contracts must useoptions { server = server; exit = exit; }and includeint exitas a constructor parameter so the playground can parameterize them. This contract hardcodesexit = 288and lacks theint exitconstructor parameter.Proposed fix
options { server = server; - exit = 288; + exit = exit; } contract ThresholdOracle( bytes32 tokenAssetIdTxid, int tokenAssetIdGidx, bytes32 ctrlAssetIdTxid, int ctrlAssetIdGidx, pubkey[] oracles, - int threshold + int threshold, + int exit ) {Also update the continuation constructor at line 45:
- require(tx.outputs[0].scriptPubKey == new ThresholdOracle(tokenAssetIdTxid, tokenAssetIdGidx, ctrlAssetIdTxid, ctrlAssetIdGidx, oracles, threshold), "broken"); + require(tx.outputs[0].scriptPubKey == new ThresholdOracle(tokenAssetIdTxid, tokenAssetIdGidx, ctrlAssetIdTxid, ctrlAssetIdGidx, oracles, threshold, exit), "broken");🤖 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.ark` around lines 13 - 16, The options block in this contract hardcodes the exit value instead of using a parameterized variable, and the constructor is missing the int exit parameter required by the coding guidelines. Change the exit assignment in the options block from the hardcoded value of 288 to use the parameterized exit variable. Add int exit as a constructor parameter to the contract. Additionally, update the continuation constructor (referenced around line 45) to also use the parameterized exit value instead of any hardcoded value, ensuring consistency throughout the contract.Source: Coding guidelines
🧹 Nitpick comments (3)
src/compiler/mod.rs (3)
1032-1105: ⚖️ Poor tradeoffNear-duplicate functions
generate_expression_asmandemit_expression_asm.Both functions contain overlapping logic for emitting ASM from expressions. This duplication increases maintenance burden and risks divergence over time. Consider consolidating into a single implementation.
Also applies to: 1587-1643
🤖 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 1032 - 1105, The functions generate_expression_asm and emit_expression_asm contain overlapping logic for emitting assembly from expressions, creating maintenance burden and risk of divergence. Consolidate these two functions into a single implementation by identifying the common logic (expression matching, opcode emission, conversion handling) and refactoring to eliminate duplication. Consider extracting shared helper functions or merging one function into the other to establish a single source of truth for expression assembly generation.
172-176: 💤 Low valueMissing recursion into
asset_txidandasset_gidxfields.
AssetLookupandAssetHasnow carryasset_txidandasset_gidxsub-expressions, but this arm only recurses intoindex. If a contract ever passes an expression (rather than a plain variable) for the txid/gidx operands, any pubkey references inside would be missed—potentially omitting required exit-path signatures.♻️ Suggested fix
Expression::AssetLookup { index, .. } - | Expression::AssetHas { index, .. } - | Expression::AssetCount { index, .. } => { + | Expression::AssetCount { index, .. } => { collect_pubkey_usage_in_expr(index, tx_sigs, data_sigs); } + Expression::AssetLookup { + index, + asset_txid, + asset_gidx, + .. + } + | Expression::AssetHas { + index, + asset_txid, + asset_gidx, + .. + } => { + collect_pubkey_usage_in_expr(index, tx_sigs, data_sigs); + collect_pubkey_usage_in_expr(asset_txid, tx_sigs, data_sigs); + collect_pubkey_usage_in_expr(asset_gidx, tx_sigs, data_sigs); + }🤖 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 172 - 176, The pattern match arm for Expression::AssetLookup and Expression::AssetHas is missing recursion into the asset_txid and asset_gidx sub-expression fields. Currently, the code only calls collect_pubkey_usage_in_expr on the index field, but AssetLookup and AssetHas variants also contain asset_txid and asset_gidx expressions that may themselves contain pubkey references. Add additional collect_pubkey_usage_in_expr calls for the asset_txid and asset_gidx fields when handling these variants to ensure all nested pubkey usages are captured, while keeping the AssetCount variant's behavior unchanged since it does not have these fields.
260-265: 💤 Low value
GroupFind,GroupHas, andGroupControlIsshould recurse into theirasset_txid/asset_gidxsub-expressions.These variants are currently classified as leaves, but they now carry boxed expressions for the Asset ID operands. Any pubkey usage nested inside those sub-expressions would be silently ignored, potentially breaking exit-signature collection.
♻️ Suggested fix
- | Expression::GroupFind { .. } - | Expression::GroupHas { .. } - | Expression::GroupProperty { .. } - | Expression::GroupControlIs { .. } + | Expression::GroupProperty { .. } | Expression::AssetGroupsLength | Expression::ArrayLength(_) => {} + Expression::GroupFind { + asset_txid, + asset_gidx, + } + | Expression::GroupHas { + asset_txid, + asset_gidx, + } => { + collect_pubkey_usage_in_expr(asset_txid, tx_sigs, data_sigs); + collect_pubkey_usage_in_expr(asset_gidx, tx_sigs, data_sigs); + } + Expression::GroupControlIs { + asset_txid, + asset_gidx, + .. + } => { + collect_pubkey_usage_in_expr(asset_txid, tx_sigs, data_sigs); + collect_pubkey_usage_in_expr(asset_gidx, tx_sigs, data_sigs); + }🤖 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 260 - 265, The variants GroupFind, GroupHas, and GroupControlIs in the match expression are currently being treated as leaf nodes with empty blocks, but they now contain boxed sub-expressions for asset_txid and asset_gidx operands that must be recursed into. Remove these three variants from the current catch-all pattern (the line with `=> {}`), and add separate match arms for each of them that extract and recursively traverse their boxed expression sub-expressions, similar to how other non-leaf variants in this match statement handle recursion. This ensures pubkey usage nested within these Asset ID operands is properly collected for exit-signature analysis.
🤖 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 `@docs/arkade-primitives-spec.md`:
- Line 108: The `u64le` entry in the specification table on line 108 incorrectly
describes the format as "Signed LE" when it should be "Unsigned LE" to match the
unsigned integer type naming convention. Change the "Signed LE" text to
"Unsigned LE" in the table row for the `u64le` type to correct this
inconsistency.
In `@docs/arkade-script-with-assets.md`:
- Line 15: The documentation incorrectly groups controlIs with lookup and find
as opcodes that "assert presence," but controlIs actually evaluates to a boolean
on absence or mismatch rather than aborting. Update the sentence at line 15 to
move controlIs from the assertion group (lookup/find) to the boolean exposure
group with has and hasControl, so it accurately reflects that controlIs returns
false for absence/mismatch instead of aborting.
In `@docs/ArkadeKitties.md`:
- Around line 171-174: Remove the obsolete null-guard checks that follow the
find(txid, gidx) method calls in the documentation examples. The find method
under the new success-flag semantics already verifies asset group presence, so
the != null comparisons in the require statements at lines 171, 246, and 281 are
unnecessary. Delete these null checks from the require statements while keeping
the rest of the validation logic intact (such as the controlIs checks that
follow them).
In `@examples/nft_mint.ark`:
- Around line 10-19: The NFTMint contract is missing the int exit constructor
parameter and has a hardcoded exit value in the options block instead of using a
parameterized one. Add int exit as a constructor parameter to the NFTMint
function (after the existing pubkey issuerPk parameter), and update the options
block to change exit = 288; to exit = exit; so the exit value can be
parameterized when the contract is instantiated.
In `@examples/non_interactive_swap.json`:
- Around line 269-273: The swap function with serverVariant=false has an
unresolved placeholder binding for takerPk that appears in the ASM code
(referenced at line 168) but is not available in either the witnessSchema or
constructorInputs, making this variant unconstructable. Resolve this by either
adding takerPk to the schema or constructor inputs so it can be properly bound,
or alternatively remove the takerPk placeholder usage from the ASM code if it is
not needed. After making either change, regenerate the artifacts to eliminate
the output-invariant warning.
In `@src/parser/grammar.pest`:
- Around line 263-267: The asset_has_comparison rule currently uses
binary_operator which includes arithmetic operators, allowing invalid
expressions like assets.has(...) + 1 to parse. Replace binary_operator in the
asset_has_comparison rule with a more restrictive rule that only allows
comparison operators such as ==, !=, <, >, <=, >=. Additionally, apply the same
fix to the similar rule referenced at lines 350-353 to ensure all boolean
predicate comparisons are restricted to comparison operators only.
In `@src/typechecker/mod.rs`:
- Around line 453-457: The type checking for assetId is currently incorrectly
typed as a single Bytes32 when it should be a composite type (asset_txid,
asset_gidx). Replace the "assetId" match arms that return ArkType::Bytes32 with
a hard error instead, to prevent incorrect typechecking until the proper
composite AssetId type or explicit accessor methods are implemented. This fix
needs to be applied at multiple locations where "assetId" is matched and typed,
specifically where the pattern appears in the match statement around line 453
and the secondary location mentioned in the review.
In `@src/validator/mod.rs`:
- Around line 301-319: The match statement in the check_asset_id_expr function
is not exhaustively handling all expression wrapper types that can contain
nested expressions. Add explicit match arms for wrapper expression types like
ContractInstance, Sha256, conversions, ArrayIndex, and group index expressions.
For each wrapper type, extract and recursively call check_asset_id_expr on the
nested expressions they contain, rather than allowing them to fall through to
the catch-all pattern. This ensures that invalid asset-ID lookups nested within
these wrapper expressions are properly validated instead of bypassing the checks
and reaching codegen.
- Around line 570-574: The code currently reserves the serverSig parameter name
when has_server_key is true, but fails to reserve the SERVER_KEY placeholder
that is also injected by the operator. Add a second record_name call after the
existing serverSig reservation (in the same has_server_key conditional block) to
reserve the SERVER_KEY placeholder name to prevent parameter name collisions
with the injected placeholder.
In `@tests/asset_id_explicit_test.rs`:
- Around line 10-18: The function server_asm only validates the server variant
path of non-internal functions, leaving the non-server variant
(serverVariant=false) untested for the new (txid, gidx) and success-flag
semantics. Create a complementary helper function (similar to server_asm) that
extracts and returns assembly for the non-server variant by searching for
functions where server_variant is false, then add paired test assertions that
validate both variants for all new syntax/opcode paths, ensuring both
serverVariant=true and serverVariant=false paths are verified per coding
guidelines.
In `@tests/beacon_test.rs`:
- Around line 52-56: The test fixture contains multiple calls to the PriceBeacon
constructor using undefined identifiers ticker and clock with only 4 arguments,
but the constructor definition requires 6 parameters: tickerTxid, tickerGidx,
clockTxid, clockGidx, oraclePk, and exit. Locate all calls to new PriceBeacon
throughout the fixture and replace each call with the correct 6-argument
version, substituting the undefined ticker and clock identifiers with their
corresponding parameter pairs tickerTxid and tickerGidx (for ticker) and
clockTxid and clockGidx (for clock), while keeping oraclePk and exit unchanged.
This will resolve the compilation errors across all three affected constructor
invocations.
In `@tests/no_shadowing_test.rs`:
- Around line 252-267: The all_examples_still_compile function uses a
non-recursive fs::read_dir() loop that only checks files in the top-level
examples directory, causing it to skip .ark files in subdirectories like
examples/bonds/ and examples/options/. Replace the current for loop that
iterates over fs::read_dir() with a recursive directory walk that traverses all
subdirectories and collects all .ark files at any depth, then compile each file
found to ensure all examples in the entire directory tree are validated.
In `@tests/token_vault_test.rs`:
- Around line 176-177: The assertions in the test starting at line 176 only
validate the `*Txid` fields (tokenAssetIdTxid and ctrlAssetIdTxid) in the JSON
output, but the migration data is a (txid, gidx) pair, leaving the gidx portion
unchecked. Add two additional assert statements after the existing ones to
verify that json_output also contains the corresponding `*Gidx` fields:
tokenAssetIdGidx and ctrlAssetIdGidx. This ensures complete validation of the
ABI structure in the CLI JSON output.
---
Outside diff comments:
In `@examples/arkade_kitties.ark`:
- Around line 15-24: Add the missing `int exit` constructor parameter to the
ArkadeKitties contract constructor signature alongside the existing parameters
speciesControlIdTxid, speciesControlIdGidx, and oraclePk. Then update the
options block to use `exit = exit;` instead of the hardcoded `exit = 576;` so
the exit timelock value can be parameterized by the playground as required by
the coding guidelines.
In `@examples/controlled_mint.ark`:
- Around line 12-23: The ControlledMint contract constructor is missing the int
exit parameter and the options block contains a hardcoded exit value instead of
a parameterized one. Add int exit as a constructor parameter to the
ControlledMint function (after the issuerPk parameter), and update the options
block to use exit = exit; instead of the hardcoded exit = 288; to allow the
playground to parameterize the exit timelock per coding guidelines.
In `@examples/fee_adapter.ark`:
- Around line 11-23: The FeeAdapter contract is missing the int exit constructor
parameter required by coding guidelines. Add int exit as a parameter to the
FeeAdapter constructor definition (after the minFee parameter). Additionally,
update the options block to change exit = 144; to exit = exit; so that the exit
value is parameterized from the constructor argument instead of being hardcoded.
In `@examples/non_interactive_swap.ark`:
- Around line 14-36: The NonInteractiveSwap contract constructor is missing the
required `int exit` parameter as specified in the coding guidelines. Add `int
exit` as a constructor parameter (positioned after the expirationTime parameter)
to allow the playground to parameterize the exit delay, and update the options
block to use the passed `exit` parameter value instead of the hardcoded value
144.
In `@examples/threshold_oracle.ark`:
- Around line 13-16: The options block in this contract hardcodes the exit value
instead of using a parameterized variable, and the constructor is missing the
int exit parameter required by the coding guidelines. Change the exit assignment
in the options block from the hardcoded value of 288 to use the parameterized
exit variable. Add int exit as a constructor parameter to the contract.
Additionally, update the continuation constructor (referenced around line 45) to
also use the parameterized exit value instead of any hardcoded value, ensuring
consistency throughout the contract.
In `@examples/token_vault.ark`:
- Around line 11-14: The options block in the token_vault.ark contract has a
hardcoded exit value of 144 instead of using a parameterized variable. Change
the exit assignment in the options block from `exit = 144;` to `exit = exit;` to
use the parameter value, and add `int exit` as a constructor parameter to the
contract so the exit value can be dynamically passed in rather than being
hardcoded.
---
Nitpick comments:
In `@src/compiler/mod.rs`:
- Around line 1032-1105: The functions generate_expression_asm and
emit_expression_asm contain overlapping logic for emitting assembly from
expressions, creating maintenance burden and risk of divergence. Consolidate
these two functions into a single implementation by identifying the common logic
(expression matching, opcode emission, conversion handling) and refactoring to
eliminate duplication. Consider extracting shared helper functions or merging
one function into the other to establish a single source of truth for expression
assembly generation.
- Around line 172-176: The pattern match arm for Expression::AssetLookup and
Expression::AssetHas is missing recursion into the asset_txid and asset_gidx
sub-expression fields. Currently, the code only calls
collect_pubkey_usage_in_expr on the index field, but AssetLookup and AssetHas
variants also contain asset_txid and asset_gidx expressions that may themselves
contain pubkey references. Add additional collect_pubkey_usage_in_expr calls for
the asset_txid and asset_gidx fields when handling these variants to ensure all
nested pubkey usages are captured, while keeping the AssetCount variant's
behavior unchanged since it does not have these fields.
- Around line 260-265: The variants GroupFind, GroupHas, and GroupControlIs in
the match expression are currently being treated as leaf nodes with empty
blocks, but they now contain boxed sub-expressions for asset_txid and asset_gidx
operands that must be recursed into. Remove these three variants from the
current catch-all pattern (the line with `=> {}`), and add separate match arms
for each of them that extract and recursively traverse their boxed expression
sub-expressions, similar to how other non-leaf variants in this match statement
handle recursion. This ensures pubkey usage nested within these Asset ID
operands is properly collected for exit-signature analysis.
🪄 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
Run ID: d5197850-320a-49c2-ba2b-e81e32b799fd
📒 Files selected for processing (42)
docs/ArkadeKitties.mddocs/arkade-primitives-spec.mddocs/arkade-script-with-assets.mdexamples/arkade_kitties.arkexamples/arkade_kitties.hackexamples/arkade_kitties.jsonexamples/bonds/bond_mint.arkexamples/bonds/repayment_pool.arkexamples/controlled_mint.arkexamples/fee_adapter.arkexamples/nft_mint.arkexamples/nft_mint.jsonexamples/non_interactive_swap.arkexamples/non_interactive_swap.hackexamples/non_interactive_swap.jsonexamples/options/cash_secured_put.arkexamples/options/cash_secured_put.jsonexamples/options/covered_call.arkexamples/options/covered_call.jsonexamples/threshold_oracle.arkexamples/token_vault.arksrc/compiler/mod.rssrc/models/mod.rssrc/opcodes/mod.rssrc/parser/grammar.pestsrc/parser/mod.rssrc/typechecker/mod.rssrc/validator/mod.rstests/asset_id_explicit_test.rstests/bare_vtxo_test.rstests/beacon_test.rstests/bond_mint_test.rstests/cash_secured_put_test.rstests/controlled_mint_test.rstests/covered_call_test.rstests/epoch_limiter_test.rstests/fee_adapter_test.rstests/group_properties_test.rstests/no_shadowing_test.rstests/repayment_pool_test.rstests/threshold_oracle_test.rstests/token_vault_test.rs
| | `u32le` | 4 bytes | Unsigned LE | OP_INSPECTLOCKTIME | OP_LE32TOLE64 | | ||
| | `u64le` | 8 bytes | Signed LE | INSPECTASSETGROUPSUM, ADD64, INSPECTINASSETLOOKUP (non-sentinel) | ADD64, GREATERTHAN64, EQUAL | | ||
| | `sentinel` | varies | CScriptNum `-1` | INSPECTINASSETLOOKUP (not found), INSPECTOUTASSETLOOKUP (not found) | Must branch before arithmetic | | ||
| | `u64le` | 8 bytes | Signed LE | INSPECTASSETGROUPSUM, ADD64, asset lookup result | ADD64, GREATERTHAN64, EQUAL | |
There was a problem hiding this comment.
Fix u64le format description to unsigned.
Line 108 labels u64le as “Signed LE”, which conflicts with the u64le type name and surrounding conversion rules. This should be “Unsigned LE”.
🤖 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 `@docs/arkade-primitives-spec.md` at line 108, The `u64le` entry in the
specification table on line 108 incorrectly describes the format as "Signed LE"
when it should be "Unsigned LE" to match the unsigned integer type naming
convention. Change the "Signed LE" text to "Unsigned LE" in the table row for
the `u64le` type to correct this inconsistency.
| All Asset IDs are represented as **two stack items**: `(txid32, gidx_u16)`. `txid32` is the transaction ID of the genesis transaction where the asset was minted, and `gidx_u16` is the index of the asset group within that genesis transaction. | ||
| All Asset IDs are the canonical pair **`(asset_txid, asset_gidx)`** — two stack items. `asset_txid` (32 bytes) is the **issuance** transaction ID where the asset was minted. `asset_gidx` is the group index **at which the asset was issued**, a minimally encoded ScriptNum in `0..65535` (not a group's current packet position — the two coincide only for a fresh issuance). | ||
|
|
||
| The lookup/find/control opcodes return a trailing **success flag**: `… 1` on success, `0 0` (or `empty 0 0` for control) on absence. The Arkade Script sugar consumes that flag for you — `lookup`/`find`/`controlIs` assert presence; `has`/`hasControl` expose it as a boolean. |
There was a problem hiding this comment.
controlIs is documented as aborting, but it evaluates to a boolean.
This line says controlIs “asserts presence,” but current codegen drops the success flag and performs equality checks, so absence/mismatch evaluates to false rather than aborting. Please align this sentence with the actual controlIs semantics.
Suggested doc fix
-The lookup/find/control opcodes return a trailing **success flag**: `… 1` on success, `0 0` (or `empty 0 0` for control) on absence. The Arkade Script sugar consumes that flag for you — `lookup`/`find`/`controlIs` assert presence; `has`/`hasControl` expose it as a boolean.
+The lookup/find/control opcodes return a trailing **success flag**: `… 1` on success, `0 0` (or `empty 0 0` for control) on absence. The Arkade Script sugar consumes that flag for you — `lookup`/`find` assert presence; `controlIs` performs canonical equality and returns a boolean; `has`/`hasControl` expose presence as a boolean.📝 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.
| The lookup/find/control opcodes return a trailing **success flag**: `… 1` on success, `0 0` (or `empty 0 0` for control) on absence. The Arkade Script sugar consumes that flag for you — `lookup`/`find`/`controlIs` assert presence; `has`/`hasControl` expose it as a boolean. | |
| The lookup/find/control opcodes return a trailing **success flag**: `… 1` on success, `0 0` (or `empty 0 0` for control) on absence. The Arkade Script sugar consumes that flag for you — `lookup`/`find` assert presence; `controlIs` performs canonical equality and returns a boolean; `has`/`hasControl` expose presence as a boolean. |
🤖 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 `@docs/arkade-script-with-assets.md` at line 15, The documentation incorrectly
groups controlIs with lookup and find as opcodes that "assert presence," but
controlIs actually evaluates to a boolean on absence or mismatch rather than
aborting. Update the sentence at line 15 to move controlIs from the assertion
group (lookup/find) to the boolean exposure group with has and hasControl, so it
accurately reflects that controlIs returns false for absence/mismatch instead of
aborting.
| let sireGroup = tx.assetGroups.find(sireIdTxid, sireIdGidx); | ||
| let dameGroup = tx.assetGroups.find(dameIdTxid, dameIdGidx); | ||
| require(sireGroup != null && dameGroup != null, "Sire and Dame assets must be spent"); | ||
| require(sireGroup.control == speciesControlId, "Sire not Species-Controlled"); | ||
| require(dameGroup.control == speciesControlId, "Dame not Species-Controlled"); | ||
| require(sireGroup.controlIs(speciesControlIdTxid, speciesControlIdGidx), "Sire not Species-Controlled"); |
There was a problem hiding this comment.
Remove stale null-guard examples after find(txid, gidx).
Line 171, Line 246, and Line 281 use find(...) followed by != null checks. Under the new success-flag semantics, find already verifies presence; these null checks are obsolete and can misdocument runtime behavior.
Suggested doc fix
- let sireGroup = tx.assetGroups.find(sireIdTxid, sireIdGidx);
- let dameGroup = tx.assetGroups.find(dameIdTxid, dameIdGidx);
- require(sireGroup != null && dameGroup != null, "Sire and Dame assets must be spent");
+ let sireGroup = tx.assetGroups.find(sireIdTxid, sireIdGidx);
+ let dameGroup = tx.assetGroups.find(dameIdTxid, dameIdGidx);
- let speciesGroup = tx.assetGroups.find(speciesControlIdTxid, speciesControlIdGidx);
- require(speciesGroup != null && speciesGroup.delta == 0, "Species Control must be present and retained");
+ let speciesGroup = tx.assetGroups.find(speciesControlIdTxid, speciesControlIdGidx);
+ require(speciesGroup.delta == 0, "Species Control must be present and retained");
- let newKittyGroup = tx.assetGroups.find(newKittyIdTxid, newKittyIdGidx);
- require(newKittyGroup != null, "New Kitty asset group not found");
+ let newKittyGroup = tx.assetGroups.find(newKittyIdTxid, newKittyIdGidx);
- let speciesGroup = tx.assetGroups.find(speciesControlIdTxid, speciesControlIdGidx);
- require(speciesGroup != null && speciesGroup.delta == 0, "Species Control must be retained");
+ let speciesGroup = tx.assetGroups.find(speciesControlIdTxid, speciesControlIdGidx);
+ require(speciesGroup.delta == 0, "Species Control must be retained");Also applies to: 246-253, 281-282
🤖 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 `@docs/ArkadeKitties.md` around lines 171 - 174, Remove the obsolete null-guard
checks that follow the find(txid, gidx) method calls in the documentation
examples. The find method under the new success-flag semantics already verifies
asset group presence, so the != null comparisons in the require statements at
lines 171, 246, and 281 are unnecessary. Delete these null checks from the
require statements while keeping the rest of the validation logic intact (such
as the controlIs checks that follow them).
| options { | ||
| server = server; | ||
| exit = 288; | ||
| server = server; | ||
| exit = 288; | ||
| } | ||
|
|
||
| contract NFTMint( | ||
| bytes32 collectionCtrlId, | ||
| bytes32 collectionCtrlIdTxid, | ||
| int collectionCtrlIdGidx, | ||
| pubkey issuerPk | ||
| ) { |
There was a problem hiding this comment.
Missing int exit constructor parameter per coding guidelines.
As per coding guidelines: "All contracts must use options { server = server; exit = exit; } ... and include int exit ... as constructor parameters so the playground can parameterize them".
🛠️ Suggested fix
options {
- server = server;
- exit = 288;
+ server = server;
+ exit = exit;
}
contract NFTMint(
bytes32 collectionCtrlIdTxid,
int collectionCtrlIdGidx,
- pubkey issuerPk
+ pubkey issuerPk,
+ int exit
) {📝 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.
| options { | |
| server = server; | |
| exit = 288; | |
| server = server; | |
| exit = 288; | |
| } | |
| contract NFTMint( | |
| bytes32 collectionCtrlId, | |
| bytes32 collectionCtrlIdTxid, | |
| int collectionCtrlIdGidx, | |
| pubkey issuerPk | |
| ) { | |
| options { | |
| server = server; | |
| exit = exit; | |
| } | |
| contract NFTMint( | |
| bytes32 collectionCtrlIdTxid, | |
| int collectionCtrlIdGidx, | |
| pubkey issuerPk, | |
| int exit | |
| ) { |
🤖 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/nft_mint.ark` around lines 10 - 19, The NFTMint contract is missing
the int exit constructor parameter and has a hardcoded exit value in the options
block instead of using a parameterized one. Add int exit as a constructor
parameter to the NFTMint function (after the existing pubkey issuerPk
parameter), and update the options block to change exit = 288; to exit = exit;
so the exit value can be parameterized when the contract is instantiated.
Source: Coding guidelines
| "warnings": [ | ||
| "warning[type]: fn swap: comparison '>=' mixes uint64le ('uint64le') with scriptnum ('int') — implicit conversion applied; use le64ToScriptNum() for explicit control", | ||
| "warning[type]: fn swap: comparison '>=' mixes uint64le ('uint64le') with scriptnum ('int') — implicit conversion applied; use le64ToScriptNum() for explicit control" | ||
| "warning[type]: fn swap: comparison '>=' mixes uint64le ('uint64le') with scriptnum ('int') — implicit conversion applied; use le64ToScriptNum() for explicit control", | ||
| "warning[output-invariant]: fn 'swap' (serverVariant=false): placeholder <takerPk> is not in witnessSchema or constructorInputs; this transaction cannot be constructed without a binding for 'takerPk'" | ||
| ] |
There was a problem hiding this comment.
Fix unresolved <takerPk> binding in swap (serverVariant=false).
Line 272 explicitly states this branch cannot be constructed because <takerPk> is not bindable from witnessSchema or constructorInputs, while ASM still consumes it (Line 168). This leaves the fallback swap path unusable; ensure takerPk is bound in the emitted schema (or remove that placeholder usage) and regenerate artifacts.
🤖 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/non_interactive_swap.json` around lines 269 - 273, The swap function
with serverVariant=false has an unresolved placeholder binding for takerPk that
appears in the ASM code (referenced at line 168) but is not available in either
the witnessSchema or constructorInputs, making this variant unconstructable.
Resolve this by either adding takerPk to the schema or constructor inputs so it
can be properly bound, or alternatively remove the takerPk placeholder usage
from the ASM code if it is not needed. After making either change, regenerate
the artifacts to eliminate the output-invariant warning.
| // `serverSig` is appended unconditionally to the cooperative witness | ||
| // schema (no dedup), so a parameter of that name genuinely collides. | ||
| if contract.has_server_key { | ||
| record_name("serverSig".to_string(), &func.name, &mut seen, issues); | ||
| } |
There was a problem hiding this comment.
Reserve the injected <SERVER_KEY> placeholder too.
serverSig is checked, but the cooperative path also emits <SERVER_KEY>. A constructor or function parameter named SERVER_KEY would share the same placeholder as the operator key injection, making the generated bindings ambiguous.
Reserve both generated server placeholders
// `serverSig` is appended unconditionally to the cooperative witness
// schema (no dedup), so a parameter of that name genuinely collides.
if contract.has_server_key {
+ record_name("SERVER_KEY".to_string(), &func.name, &mut seen, issues);
record_name("serverSig".to_string(), &func.name, &mut seen, issues);
}📝 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.
| // `serverSig` is appended unconditionally to the cooperative witness | |
| // schema (no dedup), so a parameter of that name genuinely collides. | |
| if contract.has_server_key { | |
| record_name("serverSig".to_string(), &func.name, &mut seen, issues); | |
| } | |
| // `serverSig` is appended unconditionally to the cooperative witness | |
| // schema (no dedup), so a parameter of that name genuinely collides. | |
| if contract.has_server_key { | |
| record_name("SERVER_KEY".to_string(), &func.name, &mut seen, issues); | |
| record_name("serverSig".to_string(), &func.name, &mut seen, issues); | |
| } |
🤖 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 570 - 574, The code currently reserves the
serverSig parameter name when has_server_key is true, but fails to reserve the
SERVER_KEY placeholder that is also injected by the operator. Add a second
record_name call after the existing serverSig reservation (in the same
has_server_key conditional block) to reserve the SERVER_KEY placeholder name to
prevent parameter name collisions with the injected placeholder.
| fn server_asm(src: &str, func: &str) -> String { | ||
| let out = compile(src).unwrap_or_else(|e| panic!("compile failed: {e:?}")); | ||
| let f = out | ||
| .functions | ||
| .iter() | ||
| .find(|f| f.name == func && f.server_variant) | ||
| .unwrap_or_else(|| panic!("{func} server variant not found")); | ||
| f.asm.join(" ") | ||
| } |
There was a problem hiding this comment.
Tests only assert server variant behavior for the new syntax/opcode paths.
All assembly checks flow through server_asm, so the new (txid, gidx) and success-flag semantics are not validated on the serverVariant=false path for non-internal functions. Please add paired variant assertions (or a dual-variant helper) for these new paths.
As per coding guidelines, "tests/**/*.rs: ... verify both function variants (serverVariant=true/false) for non-internal functions".
🤖 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/asset_id_explicit_test.rs` around lines 10 - 18, The function
server_asm only validates the server variant path of non-internal functions,
leaving the non-server variant (serverVariant=false) untested for the new (txid,
gidx) and success-flag semantics. Create a complementary helper function
(similar to server_asm) that extracts and returns assembly for the non-server
variant by searching for functions where server_variant is false, then add
paired test assertions that validate both variants for all new syntax/opcode
paths, ensuring both serverVariant=true and serverVariant=false paths are
verified per coding guidelines.
Source: Coding guidelines
| bytes32 tickerTxid, int tickerGidx, | ||
| bytes32 clockTxid, int clockGidx, | ||
| pubkey oraclePk, | ||
| int exit | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify full PRICE_BEACON_CODE constructor and all PriceBeacon(...) call sites.
rg -n -C4 'const\s+PRICE_BEACON_CODE|contract\s+PriceBeacon\(|new\s+PriceBeacon\(|tickerTxid|tickerGidx|clockTxid|clockGidx|\bticker\b|\bclock\b' tests/beacon_test.rsRepository: arkade-os/compiler
Length of output: 4002
Undefined identifiers ticker and clock in constructor calls—test fixture will not compile.
The constructor at lines 51–56 defines six parameters: tickerTxid, tickerGidx, clockTxid, clockGidx, oraclePk, exit. However, lines 65, 80, and 104 all call new PriceBeacon(ticker, clock, oraclePk, exit) where ticker and clock are never declared or assigned anywhere in the fixture. Additionally, these calls only pass 4 arguments instead of the required 6. The fixture code will fail compilation.
Likely fix: replace ticker and clock with the corresponding parameter pairs:
new PriceBeacon(tickerTxid, tickerGidx, clockTxid, clockGidx, oraclePk, exit)
Affects lines: 65, 80, 104
🤖 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/beacon_test.rs` around lines 52 - 56, The test fixture contains
multiple calls to the PriceBeacon constructor using undefined identifiers ticker
and clock with only 4 arguments, but the constructor definition requires 6
parameters: tickerTxid, tickerGidx, clockTxid, clockGidx, oraclePk, and exit.
Locate all calls to new PriceBeacon throughout the fixture and replace each call
with the correct 6-argument version, substituting the undefined ticker and clock
identifiers with their corresponding parameter pairs tickerTxid and tickerGidx
(for ticker) and clockTxid and clockGidx (for clock), while keeping oraclePk and
exit unchanged. This will resolve the compilation errors across all three
affected constructor invocations.
| for entry in fs::read_dir(&dir).expect("read examples dir") { | ||
| let path = entry.expect("dir entry").path(); | ||
| if path.extension().and_then(|s| s.to_str()) != Some("ark") { | ||
| continue; | ||
| } | ||
| let src = fs::read_to_string(&path).expect("read example"); | ||
| let result = compile(&src); | ||
| assert!( | ||
| result.is_ok(), | ||
| "example {} failed to compile: {:?}", | ||
| path.display(), | ||
| result.err() | ||
| ); | ||
| checked += 1; | ||
| } | ||
| assert!(checked > 0, "no .ark examples found in {}", dir.display()); |
There was a problem hiding this comment.
all_examples_still_compile misses nested example contracts.
The loop is non-recursive, so .ark files under subdirectories (like examples/bonds/ and examples/options/) are skipped. That creates a false pass despite the test’s stated invariant.
Suggested fix (recursive walk, std-only)
fn all_examples_still_compile() {
let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("examples");
let mut checked = 0;
- for entry in fs::read_dir(&dir).expect("read examples dir") {
- let path = entry.expect("dir entry").path();
- if path.extension().and_then(|s| s.to_str()) != Some("ark") {
- continue;
- }
- let src = fs::read_to_string(&path).expect("read example");
- let result = compile(&src);
- assert!(
- result.is_ok(),
- "example {} failed to compile: {:?}",
- path.display(),
- result.err()
- );
- checked += 1;
- }
+ let mut stack = vec![dir.clone()];
+ while let Some(cur) = stack.pop() {
+ for entry in fs::read_dir(&cur).expect("read examples dir") {
+ let path = entry.expect("dir entry").path();
+ if path.is_dir() {
+ stack.push(path);
+ continue;
+ }
+ if path.extension().and_then(|s| s.to_str()) != Some("ark") {
+ continue;
+ }
+ let src = fs::read_to_string(&path).expect("read example");
+ let result = compile(&src);
+ assert!(
+ result.is_ok(),
+ "example {} failed to compile: {:?}",
+ path.display(),
+ result.err()
+ );
+ checked += 1;
+ }
+ }
assert!(checked > 0, "no .ark examples found in {}", dir.display());
}📝 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 entry in fs::read_dir(&dir).expect("read examples dir") { | |
| let path = entry.expect("dir entry").path(); | |
| if path.extension().and_then(|s| s.to_str()) != Some("ark") { | |
| continue; | |
| } | |
| let src = fs::read_to_string(&path).expect("read example"); | |
| let result = compile(&src); | |
| assert!( | |
| result.is_ok(), | |
| "example {} failed to compile: {:?}", | |
| path.display(), | |
| result.err() | |
| ); | |
| checked += 1; | |
| } | |
| assert!(checked > 0, "no .ark examples found in {}", dir.display()); | |
| fn all_examples_still_compile() { | |
| let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("examples"); | |
| let mut checked = 0; | |
| let mut stack = vec![dir.clone()]; | |
| while let Some(cur) = stack.pop() { | |
| for entry in fs::read_dir(&cur).expect("read examples dir") { | |
| let path = entry.expect("dir entry").path(); | |
| if path.is_dir() { | |
| stack.push(path); | |
| continue; | |
| } | |
| if path.extension().and_then(|s| s.to_str()) != Some("ark") { | |
| continue; | |
| } | |
| let src = fs::read_to_string(&path).expect("read example"); | |
| let result = compile(&src); | |
| assert!( | |
| result.is_ok(), | |
| "example {} failed to compile: {:?}", | |
| path.display(), | |
| result.err() | |
| ); | |
| checked += 1; | |
| } | |
| } | |
| assert!(checked > 0, "no .ark examples found in {}", dir.display()); | |
| } |
🤖 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/no_shadowing_test.rs` around lines 252 - 267, The
all_examples_still_compile function uses a non-recursive fs::read_dir() loop
that only checks files in the top-level examples directory, causing it to skip
.ark files in subdirectories like examples/bonds/ and examples/options/. Replace
the current for loop that iterates over fs::read_dir() with a recursive
directory walk that traverses all subdirectories and collects all .ark files at
any depth, then compile each file found to ensure all examples in the entire
directory tree are validated.
| assert!(json_output.contains("tokenAssetIdTxid")); | ||
| assert!(json_output.contains("ctrlAssetIdTxid")); |
There was a problem hiding this comment.
CLI JSON assertion should also validate the *Gidx fields.
This migration is a (txid, gidx) pair; checking only *Txid in CLI output leaves half the ABI unchecked.
Suggested test extension
assert!(json_output.contains("tokenAssetIdTxid"));
+ assert!(json_output.contains("tokenAssetIdGidx"));
assert!(json_output.contains("ctrlAssetIdTxid"));
+ assert!(json_output.contains("ctrlAssetIdGidx"));📝 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.
| assert!(json_output.contains("tokenAssetIdTxid")); | |
| assert!(json_output.contains("ctrlAssetIdTxid")); | |
| assert!(json_output.contains("tokenAssetIdTxid")); | |
| assert!(json_output.contains("tokenAssetIdGidx")); | |
| assert!(json_output.contains("ctrlAssetIdTxid")); | |
| assert!(json_output.contains("ctrlAssetIdGidx")); |
🤖 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/token_vault_test.rs` around lines 176 - 177, The assertions in the test
starting at line 176 only validate the `*Txid` fields (tokenAssetIdTxid and
ctrlAssetIdTxid) in the JSON output, but the migration data is a (txid, gidx)
pair, leaving the gidx portion unchecked. Add two additional assert statements
after the existing ones to verify that json_output also contains the
corresponding `*Gidx` fields: tokenAssetIdGidx and ctrlAssetIdGidx. This ensures
complete validation of the ABI structure in the CLI JSON output.
1 participant
Summary
Asset lookups now take the canonical Asset ID as two explicit operands �
asset_txid(bytes32) andasset_gidx(int,0..65535) � matching the introspector opcode ABI 1:1. This removes the compiler's implicitbytes32 � _txid/_gidxdecomposition and adopts the new(result, success_flag)opcode return ABI. Implementsdocs/superpowers/specs/2026-06-11-asset-lookup-explicit-txid-gidx-design.md.Source syntax
assets.lookup(foo)assets.lookup(fooTxid, fooGidx)assetGroups.find(foo)assetGroups.find(fooTxid, fooGidx)group.control == foogroup.controlIs(fooTxid, fooGidx)bytes32 foo(auto-split)bytes32 fooTxid, int fooGidx(two ordinary params)New struct-free predicates added:
assets.has(txid, gidx),assetGroups.has(txid, gidx),group.hasControl� allBool.Behavior changes
lookup/findassert presence by consuming the success flag with a singleOP_VERIFY(replaces the stale-1sentinel guard); they leave the typedamount/k.has/hasControlkeep the flag and drop the result (OP_NIP) to expose presence as a boolean.findrequirement emits� OP_VERIFY OP_DROP OP_1so a valid positionk == 0isn't treated as false.controlIsdoes full canonical Asset ID equality (OP_DROPflag, compare both components,OP_BOOLAND) � noOP_EQUALVERIFY, so a mismatch/absence yieldsfalserather than aborting..controlproperty removed; legacy single-arglookup(foo)/find(foo)now hard-error instead of silently misparsing as a property.Implementation
lookup/find; newAssetHas/GroupHas/GroupControlIsAST + emission; deletedcollect_lookup_asset_idsand the bytes32 branch ofdecompose_constructor_params(now array-flattening only); loop-unrolling (substitute_expression) recurses the new operands.asset_txidmust resolve tobytes32,asset_gidxtoint(literals range-checked0..65535); rejects swapped/unresolved operands at compile time. Expanded-namespace check updated for the one-arg decomposition helper; asset-decomposition collision class removed.OP_SWAP,OP_BOOLAND.Migration
new �()reconstructions,.control�controlIs)..json+.hack); unrelated artifacts left untouched.arkade-script-with-assets.md,arkade-primitives-spec.md,ArkadeKitties.md): two-arg forms,(result, flag)ABI, canonicalasset_gidxframing.Tests
tests/asset_id_explicit_test.rs(15 tests): flag consume, bare-findOP_DROP OP_1,has/controlIs/hasControlASM, minimal-encoding gidx, parser rejection (legacy/missing/extra arg), fatal validation (swapped/out-of-range), loop-index gidx.cargo fmt --checkclean.Out of scope (follow-ups, marked
TODO(asset-id-struct))First-class composite
AssetIdtype (or return value unpacking);.assetIdvalue-side typing (emission already PR#97-correct); threadingfind's resolvedkinto group-property access.Summary by CodeRabbit
Release Notes