refactor: Compile comparison logic in Bitcoin Script order#47
Conversation
Walkthrough
ChangesComparison ASM operand order fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Code Review — #47
Bug: Missed match arm — (Property, ">=", Property) still emits reversed order
src/compiler/mod.rs ~line 1478 (PR branch)
The PR fixes 15 of the 16 affected arms listed in #39 but misses the last one. In the PR branch:
(Expression::Property(prop), ">=", Expression::Property(prop2)) => {
asm.push(format!("<{}>", prop));
asm.push(OP_GREATERTHANOREQUAL.to_string()); // ❌ opcode BEFORE second operand
asm.push(format!("<{}>", prop2));
}Should be:
(Expression::Property(prop), ">=", Expression::Property(prop2)) => {
asm.push(format!("<{}>", prop));
asm.push(format!("<{}>", prop2)); // ✅ second operand first
asm.push(OP_GREATERTHANOREQUAL.to_string()); // ✅ opcode last
}Bitcoin Script is postfix. OP_GREATERTHANOREQUAL pops the top two stack items. If the opcode fires before the RHS is pushed, the script is malformed. This would affect any contract comparing two introspection properties with >= (e.g., tx.weight >= tx.locktime).
Missing test coverage
The PR adds 11 tests but there are 16+ comparison arms. Missing tests for:
Variable == Literal(code fixed, no test)Variable == Property(code fixed, no test)Variable >= Property(code fixed, no test)Literal == Property(code fixed, no test)Literal >= Property(code fixed, no test)Property == Property(code fixed, no test)Property >= Property(code NOT fixed, no test — this is the bug above)
The missing Property >= Property test is particularly concerning — if it existed, it would have caught the bug.
Architectural note (non-blocking)
Issue #39 itself recommended deleting all fast-path arms and letting everything fall through to emit_comparison_asm, which already emits the correct left, right, op order and handles 64-bit operands. The manual arm-flipping approach taken here is error-prone (as the missed arm proves). Consider the delete-and-fallthrough approach for a more robust fix — fewer lines to maintain, one source of truth for operand ordering.
Summary
| Finding | Severity | Status |
|---|---|---|
(Property, ">=", Property) still reversed |
🔴 Bug — would produce broken Script | Must fix |
| 7 of 16 arms have no regression test | 🟡 Medium | Should fix |
| Manual flipping vs. delete-and-fallthrough | 🟢 Suggestion | Nice to have |
Requesting changes for the missed arm. The rest of the fix is correct and the new tests are solid. One-line fix + one more test and this is good to go.
![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 (1)
src/compiler/mod.rs (1)
1476-1480:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
(Property, ">=", Property)arm still has reversed operand order.This match arm was not updated and still emits the opcode between operands instead of after both. The sequence
<prop> OP_GREATERTHANOREQUAL <prop2>violates Bitcoin Script postfix notation—the opcode must follow both operands.🐛 Proposed fix
(Expression::Property(prop), ">=", Expression::Property(prop2)) => { asm.push(format!("<{}>", prop)); - asm.push(OP_GREATERTHANOREQUAL.to_string()); asm.push(format!("<{}>", prop2)); + asm.push(OP_GREATERTHANOREQUAL.to_string()); }As per coding guidelines,
src/compiler/mod.rscontrols script-generation semantics and is high-impact, requiring careful validation.🤖 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 1476 - 1480, The match arm for (Expression::Property(prop), ">=", Expression::Property(prop2)) currently emits operands and the opcode in the wrong order. Reorder the three asm.push() statements so that both property operands are pushed first (the format!("<{}>", prop) and format!("<{}>", prop2) calls), followed by the OP_GREATERTHANOREQUAL opcode push. This corrects the postfix notation where the opcode must follow its operands, not appear between them.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 `@tests/comparison_asm_order.rs`:
- Around line 314-343: Add a new test function
test_property_greater_or_equal_to_property in the test file that verifies the
assembly generation when comparing two properties instead of a property to a
literal. The test should compile code with tx.weight >= tx.weight, extract the
propertyGreaterOrEqualToProperty function, and assert that the function assembly
has length 3 with elements OP_TXWEIGHT, OP_TXWEIGHT, and OP_GREATERTHANOREQUAL
in that order. Additionally, consider adding a similar test for Property ==
Property comparisons to strengthen regression test coverage.
---
Outside diff comments:
In `@src/compiler/mod.rs`:
- Around line 1476-1480: The match arm for (Expression::Property(prop), ">=",
Expression::Property(prop2)) currently emits operands and the opcode in the
wrong order. Reorder the three asm.push() statements so that both property
operands are pushed first (the format!("<{}>", prop) and format!("<{}>", prop2)
calls), followed by the OP_GREATERTHANOREQUAL opcode push. This corrects the
postfix notation where the opcode must follow its operands, not appear between
them.
🪄 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: 0d1bd4bf-7ad7-48ce-8b3f-251b057e905c
📒 Files selected for processing (2)
src/compiler/mod.rstests/comparison_asm_order.rs
| #[test] | ||
| fn test_property_greater_or_equal_to_literal() { | ||
| let code = r#" | ||
| contract PropertyLiteralComparison() { | ||
| function propertyGreaterOrEqualToLiteral() { | ||
| require(tx.weight >= 12); | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| let result = compile(code); | ||
| assert!( | ||
| result.is_ok(), | ||
| "Failed to parse compare: {:?}", | ||
| result.err() | ||
| ); | ||
|
|
||
| let output = result.unwrap(); | ||
|
|
||
| let function = output | ||
| .functions | ||
| .iter() | ||
| .find(|f| f.name == "propertyGreaterOrEqualToLiteral") | ||
| .expect("Missing function"); | ||
|
|
||
| assert_eq!(function.asm.len(), 3); | ||
| assert_eq!(function.asm[0], OP_TXWEIGHT); | ||
| assert_eq!(function.asm[1], "12"); | ||
| assert_eq!(function.asm[2], OP_GREATERTHANOREQUAL); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing test coverage for Property >= Property comparison.
The test suite lacks coverage for Property >= Property comparisons, which is precisely where the unfixed bug exists in generate_comparison_asm. Consider adding a test like:
#[test]
fn test_property_greater_or_equal_to_property() {
let code = r#"
contract PropertyPropertyComparison() {
function propertyGreaterOrEqualToProperty() {
require(tx.weight >= tx.weight);
}
}
"#;
let result = compile(code);
assert!(result.is_ok(), "Failed to parse compare: {:?}", result.err());
let output = result.unwrap();
let function = output
.functions
.iter()
.find(|f| f.name == "propertyGreaterOrEqualToProperty")
.expect("Missing function");
assert_eq!(function.asm.len(), 3);
assert_eq!(function.asm[0], OP_TXWEIGHT);
assert_eq!(function.asm[1], OP_TXWEIGHT);
assert_eq!(function.asm[2], OP_GREATERTHANOREQUAL);
}A similar test for Property == Property would also strengthen the regression suite.
🤖 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/comparison_asm_order.rs` around lines 314 - 343, Add a new test
function test_property_greater_or_equal_to_property in the test file that
verifies the assembly generation when comparing two properties instead of a
property to a literal. The test should compile code with tx.weight >= tx.weight,
extract the propertyGreaterOrEqualToProperty function, and assert that the
function assembly has length 3 with elements OP_TXWEIGHT, OP_TXWEIGHT, and
OP_GREATERTHANOREQUAL in that order. Additionally, consider adding a similar
test for Property == Property comparisons to strengthen regression test
coverage.
Source: Coding guidelines
There was a problem hiding this comment.
✅ Re-review — #47 (commit 6184c19c)
Previous finding: FIXED ✅
The (Property, ">=", Property) arm at src/compiler/mod.rs:1476 now correctly emits lhs rhs OP_GREATERTHANOREQUAL in postfix order. All 16 fast-path comparison arms are verified correct.
Remaining note: test coverage gaps (non-blocking)
7 of 16 modified arms still lack dedicated tests:
| Arm | Operator |
|---|---|
Variable == Literal |
== |
Variable == Property |
== |
Variable >= Property |
>= |
Literal == Property |
== |
Literal >= Property |
>= |
Property == Property |
== |
Property >= Property |
>= |
The 11 existing tests provide good coverage of the pattern — every expression type appears as LHS, and both operators are exercised. The untested arms follow the exact same mechanical structure, so regression risk is low. I'm not blocking on this, but recommend adding them as a follow-up to make the regression suite complete.
LGTM. Approving.
shubertm
changed the title
1 participant
Resolves #39
@tiero
Summary by CodeRabbit
Bug Fixes
Tests