Align compiler with unified VM opcodes - #63
Conversation
Playground PreviewA live preview of this PR's playground is available at:
|
WalkthroughThe compiler adds cryptographic and byte-reversal expressions, removes fixed-width numeric conversions, updates type inference and opcode emission, revises introspection handling, removes legacy encodings from bindings, and changes oracle message hashing to explicit ChangesExpression and compiler update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant Parser
participant Expression
participant Compiler
participant ASM
Source->>Parser: parse new function expression
Parser->>Expression: construct expression variant
Expression->>Compiler: compile operands
Compiler->>ASM: emit opcode sequence
Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/compiler/concat.rs (1)
217-238: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Digestis missing from the concat-rewrite pass — nested+insidedigest(...)won't be rewritten to byte concatenation.
Digest.datais parsed viaparse_additive_expr(src/parser/crypto.rs::parse_digest) — the exact same path asSha256.data, which is explicitly handled here (recursing to rewrite nested+intoConcat).Digestinstead falls through to theothercatch-all, which does not recurse into its operands at all. As a result,digest(bytesA + bytesB, hashType)keepsbytesA + bytesBas a rawBinaryOp, andemit_expression_asmwill later compile it as arithmeticOP_ADDinstead ofOP_CAT, producing incorrect bytecode for a bytes-concatenation-then-hash expression.As per path instructions,
Implement language features across the grammar, AST/models, parser, and compiler together; do not add expression or requirement variants without compiler emission and tests— this specific compiler-pass gap (and the missing edge-case test fordigestwith a+operand) should be closed before merge.🐛 Proposed fix: add explicit `Digest` arm mirroring `Sha256`
+ Expression::Digest { data, hash_type } => { + let (new_data, _) = rewrite_expression_concat(*data, scope); + ( + Expression::Digest { + data: Box::new(new_data), + hash_type, + }, + ArkType::Bytes, + ) + } // Leaves and other compound expressions: no `+` to rewrite below the surface. other => { let t = crate::typechecker::infer_type(&other, scope); (other, t) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/concat.rs` around lines 217 - 238, Update rewrite_expression_concat to add an explicit Digest arm, recursively applying rewrite_expression_concat to Digest.data just as the existing Sha256 handling does, while preserving the other Digest fields and returning the correct expression type. Add or update the edge-case test covering digest with a bytes-concatenation operand so nested + is emitted as byte concatenation rather than arithmetic.Source: Path instructions
src/parser/grammar.pest (1)
190-232: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
reverseBytesbreaks when used as a function-call/constructor argument.
complex_expression's ordered choice was extended withdigest_func,sighash_func,negate_func,mod_exp_func,ec_add,ec_mul,ec_pairing— but notreverse_bytes_func(unlikeprimary_expr, which does include it at Line 152). Since"reverseBytes"is registered inreserved_function_signature(src/parser/expr.rs), anyreverseBytes(...)call that must be parsed as acomplex_expression(function-call arguments, constructor args, array literals) will instead match the genericfunction_callalternative and get unconditionally rejected byreject_reserved_function_callwith "malformed reserved function callreverseBytes(...)" — even with a syntactically correct single argument. This only works today viarequire(reverseBytes(...)), which reachesprimary_exprthroughgeneral_expression; no test currently exercises thecomplex_expressionpath, so this defect is untested.🐛 Proposed fix
digest_func | sighash_func | negate_func | mod_exp_func | ec_add | ec_mul | ec_pairing | ec_mul_scalar_verify | tweak_verify | substr_func | cat_func | bin2num_func | num2bin_func | + reverse_bytes_func | size_func |As per coding guidelines, "Implement language features across the grammar, AST/models, parser, and compiler together; do not add expression or requirement variants without compiler emission and 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/parser/grammar.pest` around lines 190 - 232, Update the complex_expression ordered alternatives to include reverse_bytes_func alongside the other reserved expression functions, ensuring reverseBytes(...) parses correctly in function-call, constructor, and array-literal arguments. Reuse the existing reverse_bytes_func and primary_expr handling; do not alter generic function-call rejection or add unrelated expression variants.Source: Coding guidelines
🧹 Nitpick comments (1)
tests/features/opcode_functions.rs (1)
359-361: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing negative-test coverage for removed conversion functions.
test_conversion_chainwas deleted, but no test now verifiesneg64/le64ToScriptNum/le32ToLe64produce the new rejection error added insrc/parser/expr.rs(reject_reserved_function_call). This is a real behavior change (previously compiled fixed-width ops now hard-error) with zero coverage.✅ Suggested addition
#[test] fn test_neg64_is_rejected() { let code = r#" contract Removed(int a) { function check() { let x = neg64(a); require(true); } } "#; let err = compile(code).expect_err("neg64 should be rejected"); assert!(err.to_string().contains("BigNum arithmetic"), "unexpected error: {err}"); }As per coding guidelines, "Add integration tests for every new syntax or opcode path, including happy paths and edge conditions."
🤖 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 359 - 361, Add negative integration tests in the opcode function test module covering each removed conversion function—neg64, le64ToScriptNum, and le32ToLe64—through compilation attempts that must fail with the new “BigNum arithmetic” rejection error from reject_reserved_function_call. Replace the deleted test_conversion_chain coverage with these rejection cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compiler/loops.rs`:
- Around line 276-381: Add an explicit Expression::Negate arm in
substitute_expression that recursively substitutes its value using the same
index_var, value_var, k, and array_name arguments, preserving the Negate
wrapper. Add or update loop-unrolling coverage to verify negate(i) and
negate(arr[i]) resolve to the substituted literal or array element.
---
Outside diff comments:
In `@src/compiler/concat.rs`:
- Around line 217-238: Update rewrite_expression_concat to add an explicit
Digest arm, recursively applying rewrite_expression_concat to Digest.data just
as the existing Sha256 handling does, while preserving the other Digest fields
and returning the correct expression type. Add or update the edge-case test
covering digest with a bytes-concatenation operand so nested + is emitted as
byte concatenation rather than arithmetic.
In `@src/parser/grammar.pest`:
- Around line 190-232: Update the complex_expression ordered alternatives to
include reverse_bytes_func alongside the other reserved expression functions,
ensuring reverseBytes(...) parses correctly in function-call, constructor, and
array-literal arguments. Reuse the existing reverse_bytes_func and primary_expr
handling; do not alter generic function-call rejection or add unrelated
expression variants.
---
Nitpick comments:
In `@tests/features/opcode_functions.rs`:
- Around line 359-361: Add negative integration tests in the opcode function
test module covering each removed conversion function—neg64, le64ToScriptNum,
and le32ToLe64—through compilation attempts that must fail with the new “BigNum
arithmetic” rejection error from reject_reserved_function_call. Replace the
deleted test_conversion_chain coverage with these rejection cases.
🪄 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: d0116508-0d30-4b9b-bb01-f378ce6764a4
📒 Files selected for processing (23)
examples/stability/stability.mdsrc/compiler/asset.rssrc/compiler/concat.rssrc/compiler/expr.rssrc/compiler/introspection.rssrc/compiler/loops.rssrc/compiler/mod.rssrc/models/mod.rssrc/opcodes/mod.rssrc/parser/crypto.rssrc/parser/expr.rssrc/parser/grammar.pestsrc/parser/introspection.rssrc/typechecker/mod.rssrc/validator/mod.rstests/examples/arkade_kitties.rstests/features.rstests/features/concat_op.rstests/features/group_properties.rstests/features/io_introspection.rstests/features/opcode_functions.rstests/features/packet_primitives.rstests/features/type_system.rs
💤 Files with no reviewable changes (1)
- src/compiler/introspection.rs
Fix the scriptPubKey inspections dropping the witness version the opcode pushes above the program, so `==` compares the program rather than the version. Applied once in a shared helper and used by the indexed input, indexed output, current-input, and this.activeBytecode paths. Reject implicit int coercion in byte concatenation. `bytes + int` now errors and asks for num2bin(value, width); the compiler no longer picks a width, since the width and byte order decide what an off-chain signer must hash. Oracle-message contracts updated to state width 8. Replace negate() with a unary minus operator and delete Uint32Le, so version/locktime/sequence/weight are plain ints now that the VM pushes them as BigNums. Drop the dead le64/le32 encodings from the bindgen and playground codegen, and substitute the loop index through Negate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/compiler/concat.rs (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo differently-scoped
is_numerichelpers with diverging membership.This module's
is_numerictreatsInt | Boolas numeric (for deciding when a+operand needs explicitnum2bin), whilesrc/typechecker/mod.rs'sis_numerictreats onlyIntas numeric (for comparison-operator compatibility). Same name, same crate, different semantics — a future reader could reasonably assume they're interchangeable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/concat.rs` around lines 17 - 23, Rename the concat-specific helper is_numeric to a name that clearly reflects its byte-concatenation or num2bin purpose, and update its call sites in the concatenation logic. Leave the typechecker::is_numeric helper unchanged, preserving the distinct Int-versus-Int|Bool semantics.
🤖 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 `@src/compiler/concat.rs`:
- Around line 88-103: Capture the iterable type returned by
rewrite_expression_concat instead of discarding it, then bind value_var in
loop_scope to the array element type when the iterable is ArkType::Array(inner).
Preserve ArkType::Unknown for non-array or unavailable iterable types, while
keeping index_var typed as ArkType::Int and rewriting the loop body through
rewrite_statements_concat.
---
Nitpick comments:
In `@src/compiler/concat.rs`:
- Around line 17-23: Rename the concat-specific helper is_numeric to a name that
clearly reflects its byte-concatenation or num2bin purpose, and update its call
sites in the concatenation logic. Leave the typechecker::is_numeric helper
unchanged, preserving the distinct Int-versus-Int|Bool semantics.
🪄 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: 37733ca9-edb0-4a52-b8f7-4cff1fc136fd
📒 Files selected for processing (25)
arkade-bindgen/src/ir.rsarkade-bindgen/src/targets/go.rsarkade-bindgen/src/targets/typescript.rsarkade-bindgen/tests/ir_test.rsexamples/bonds/repayment_pool.arkexamples/stability/stability.mdexamples/stability/stability_offer.arkexamples/stability/stability_vault.arkplayground/arkade-language.jsplayground/codegen.jssrc/compiler/concat.rssrc/compiler/expr.rssrc/compiler/introspection.rssrc/compiler/loops.rssrc/compiler/mod.rssrc/models/mod.rssrc/parser/crypto.rssrc/parser/expr.rssrc/parser/grammar.pestsrc/typechecker/mod.rstests/examples/layerzero.rstests/features/concat_op.rstests/features/contract_import_instantiation.rstests/features/opcode_functions.rstests/features/packet_primitives.rs
💤 Files with no reviewable changes (4)
- src/parser/crypto.rs
- arkade-bindgen/src/targets/go.rs
- arkade-bindgen/src/ir.rs
- arkade-bindgen/tests/ir_test.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/compiler/loops.rs
- examples/stability/stability.md
- src/compiler/introspection.rs
- tests/features/concat_op.rs
- src/parser/grammar.pest
- src/models/mod.rs
digest's data operand is parsed as an additive expression like sha256's, but the concat rewrite skipped it, so `digest(a + b, ht)` compiled the concatenation as OP_ADD. Type the loop value variable from the iterable's element type instead of Unknown, so concatenating it into bytes asks for num2bin like anywhere else rather than passing through implicitly. Substitute the loop index through the remaining expression variants that carry operands. Byte and hashing operations were falling to the catch-all, so a loop value used inside sha256/num2bin/substr kept the unresolved variable after unrolling.
1 participant
Summary
reverseBytes,modExp,ecAdd,ecMul, one-pairecPairing,sighash, anddigestnew_opcodes.rsto the stableopcode_functions.rsand keep positive opcode emission coverageRequires the matching VM change
This depends on the introspector pushing
OP_INSPECTVERSION,OP_INSPECTLOCKTIME,OP_TXWEIGHT, andOP_INSPECTINPUTSEQUENCEas minimally-encoded BigNums rather than raw 4-byte little-endian arrays. Until that lands,tx.version == 2compares byte strings and is silently false, andtx.locktime/tx.weightcomparisons abort withErrMinimalDatabecause a raw 4-byte push is non-minimal. Merge after the VM side.Review fixes
scriptPubKey inspections dropped one stack item too few.
OP_INSPECT{IN,OUT}PUTSCRIPTPUBKEYpushes the witness program and the witness version on top of it, sotx.outputs[0].scriptPubKey == xcomparedxagainst the version and stranded the program. A shared helper now emits the trailingOP_DROP, used by the indexed input, indexed output, current-input, andthis.activeBytecodepaths. The<VTXO:...>placeholder resolves to the 32-byte witness program, not a 34-byte serialized P2TR script.Byte concatenation no longer coerces ints implicitly.
bytes + intis now a compile error namingnum2bin(value, width). The width and byte order are consensus-visible — they decide what an off-chain signer must hash — so the compiler does not pick one.num2binwrites little-endian sign-magnitude, which matches an unsigned LE64 only for non-negative values, and a value that does not fit the requested width fails the script rather than truncating. The oracle-message contracts inrepayment_pool.ark,stability_vault.ark, andstability_offer.arknow state width 8, preserving the bytes they emitted before.negate()replaced by a unary minus operator.unary_exprtakes an optional leading-;Expression::NegateandOP_NEGATEare unchanged. Binarya - bis unaffected. A guard rejectsnegate(x)with a pointer to-value, since an unknown call otherwise compiles to a silent placeholder.intis justint.ArkType::Uint32Leis deleted, sotx.version,tx.locktime,tx.weight, andtx.inputs[i].sequenceare ordinary ints with no width leaking into contract source. Thele64andle32witness encodings were unreachable —ArkType::parsemaps declared types only and can produce neither — so they are removed from the artifact doc table,arkade-bindgen, and the playground codegen.digestdid not rewrite+to concatenation. Itsdataoperand is parsed as an additive expression, same assha256, but the concat pass had no arm for it, sodigest(a + b, hashType)compiled the concatenation asOP_ADD.Loop value variables were typed
Unknown. The element type of the iterable is now carried into the loop scope, so concatenating a loop value into bytes asks fornum2binlike anywhere else instead of passing through implicitly — the same implicit coercion this PR rejects, previously still reachable inside loop bodies.Loop unrolling dropped the index in most expressions. Only 23 of 46 expression variants were substituted; the rest fell to a catch-all, so a loop value used inside
sha256,num2bin,substr,cat, or a concatenation kept the unresolved variable after unrolling. All operand-carrying variants now substitute.Deferred
ecAddandecMulpush x and y as two stack items, which the AST cannot represent —letbinds one name, and==would only see y. Their operand order is verified against the VM and their emission is correct, but the results cannot be consumed yet. Both carry aTODO(asset-id-struct)tying them to the composite return type thatgroup.assetIdalready waits on.Validation
cargo test --workspacecargo clippy --workspace --all-targets -- -D warningscargo fmt --check./playground/build.shOP_ECADD,OP_ECMUL,OP_ECPAIRING(includingpair_countbeforecurve_id),OP_MODEXP,OP_DIGEST, andOP_SIGHASHwas checked against their handlers