Add OP_PUT stack opcode - #136
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe change adds ChangesOP_PUT stack replacement
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This PR adds the OP_PUT opcode with focused stack behavior and validation coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — OP_PUT (0xbb)
Verdict: no blocking issues. One minor observation on fuzz coverage depth.
stack.go —
(new function, after line 364)
The bounds check delegates to , which enforces and returns for negatives and out-of-range depths. The subsequent write indexes the same slot, so after a successful peek the write is guaranteed in-bounds. No overflow risk: is , is at least 32 bits, and the peek gate ensures the subtraction cannot underderflow. Implementation is consistent with / (no unnecessary deep copy — assignment replaces the slot pointer, consistent with the rest of the codebase).
opcode.go —
(new function, after PickN ~line 1175)
Pop order is correct: (depth ) → (value) → . When is called the two sentinel items have already been removed, so depth is measured against the remaining stack exactly as specified. The doc comment examples (, ) verify against the test vectors.
Tests
Unit vectors cover:
- depth 0 (, bottom of remaining stack is a 2-item stack)
- depth 1 ()
- depth 2 ()
- empty value (, replacement)
- negative index →
- out-of-range (stack too shallow for requested depth) →
- missing target (both n and value consumed, nothing left) →
- complete underflow →
The invariant ( on success, on error) is correct: at most two items are consumed before any failure path is reached.
Fuzz coverage — minor observation
— hardcodes as the depth item (), so the corpus always exercises . The explicit unit vectors cover deeper and invalid depths adequately, but the fuzzer won't find crashes/panics at depth >0 from unexpected serialised data. Worth extending — e.g. derive the depth byte from the same way uses — but not a blocker given the explicit coverage.
Cross-repo impact
Grepped all SDK clones (ts-sdk, go-sdk, rust-sdk, dotnet-sdk, compiler) for / — no consumers found. The slot was previously , so assigning a live handler is a non-breaking change for any code that treated it as invalid. Compiler integration is correctly deferred.
Summary
Semantics, bounds checking, error propagation, and test coverage are all correct. The single observation above (fuzz depth) is low-priority. LGTM to merge once CI is green.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — OP_PUT (0xbb)
Verdict: no blocking issues. One minor observation on fuzz coverage depth.
stack.go — PutN
pkg/arkade/stack.go (new function, after line 364)
The bounds check delegates to PeekByteArray(n), which enforces 0 <= n < sz and returns ErrInvalidStackOperation for negatives and out-of-range depths. The subsequent slot write (s.stk[len(s.stk)-int(n)-1] = so) addresses the same index, so after a successful peek the write is guaranteed in-bounds. No overflow risk: n is int32, int is at least 32 bits, and the peek gate ensures the subtraction cannot underflow. The function is consistent with PickN/RollN — no unnecessary deep copy, it replaces the slot pointer, which is the established pattern throughout the stack implementation.
opcode.go — opcodePut
pkg/arkade/opcode.go (new function, after opcodePickN ~line 1175)
Pop order is correct: PopInt (depth n) → PopByteArray (value) → PutN(n, value). By the time PutN is called the two sentinel items have already been removed, so depth n is measured against the remaining stack exactly as the specified semantics require. The doc comment examples (n=0, n=2) verify against the unit test vectors — all consistent.
Tests
Unit vectors in pkg/arkade/opcode_test.go cover:
- depth 0 (
put_0) - depth 1 (
put_1) - depth 2 (
put_2) - empty (nil) value replacement (
put_empty) - negative index →
ErrInvalidStackOperation - out-of-range (stack too shallow for requested depth) →
ErrInvalidStackOperation - missing target (both n and value consumed, nothing left) →
ErrInvalidStackOperation - complete underflow →
ErrInvalidStackOperation
The checkProperties invariant (len(afterStack) == len(beforeStack)-2 on success; beforeStack-2 <= len(afterStack) <= beforeStack on error) is correct: at most two items are consumed before any failure path is reached.
Stack-level tests in pkg/arkade/stack_test.go cover Put0, Put2, and the too-little-stack case. Good.
Fuzz coverage — minor observation
pkg/arkade/opcode_fuzz_test.go — putCaseBuilder hardcodes nil as the depth item (c.stackPushes = [][]byte{..., nil}), so the fuzz corpus always exercises n=0. Explicit unit vectors cover deeper and invalid depths adequately, but the fuzzer will not explore crashes or panics at depth > 0 from unexpected serialised data. Worth extending — for example, derive the depth byte from saltedBytes(data, 0x72) the way indexCaseBuilder uses fuzzIndexSource.IndexSeed — but not a blocker given the explicit unit coverage.
Cross-repo impact
Grepped all SDK clones (ts-sdk, go-sdk, rust-sdk, dotnet-sdk, compiler) for 0xbb and OP_UNKNOWN187 — no consumers found. The slot was previously opcodeInvalid, so promoting it to a live handler is non-breaking for any code that treated it as invalid. Compiler integration is correctly deferred as a follow-up.
Summary
Semantics, bounds checking, error propagation, and test coverage are all correct. The single observation above (fuzz depth) is low-priority. LGTM to merge once CI is green.
Summary
OP_PUTat0xbb(187), replacing the previously undefined opcode slotSemantics
OP_PUTpopsdepth, popsvalue, then replaces the item atdepthmeasured in the remaining stack:The successful stack effect is always
-2.depthuses the same minimally encoded four-byte script-number rules asOP_PICKandOP_ROLL; negative and out-of-range depths fail withErrInvalidStackOperation.Arkade language impact
The compiler lives in a separate repository, so its lowering is a follow-up after this VM opcode lands. These examples were compiled with the current compiler to measure the opportunity.
Simple mutable variable
Today the replacement tail is:
With
OP_PUTit becomes:The complete covenant drops from 18 to 17 assembly tokens. The small saving is the important base case: every scalar assignment becomes constant-size and no longer needs special stack restoration.
Deep assignment
The current assignment emits 48 tokens: the value expression,
OP_16 OP_ROLL OP_DROP, 15OP_SWAPs, 14OP_TOALTSTACKs, and 14OP_FROMALTSTACKs. WithOP_PUT, the same assignment is four tokens:That reduces the full covenant from 88 to 44 assembly tokens. The saving grows linearly with assignment depth while
OP_PUTremains constant-size.Runtime-indexed array assignment will additionally retain the compiler's
0 <= index < array.lengthchecks so an index cannot overwrite an adjacent binding.Validation
make testgolangci-lint run --new-from-rev=HEADfrompkg/arkadeSummary by CodeRabbit
New Features
OP_PUTscript operation.Bug Fixes