Conversation
chore: simplify test chore: be explicit about padding
* Initial plan * chore: add changelog entry for PR 2203 Co-authored-by: mmagician <8402446+mmagician@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mmagician <8402446+mmagician@users.noreply.github.com>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
chore: remove component code
There was a problem hiding this comment.
Pull request overview
This PR introduces a double-word array abstraction for Miden, enabling storage and retrieval of two words per index through a convenient interface. The implementation extends the existing single-word array pattern by storing words under keys [index, 0, 0, 0] and [index, 1, 0, 0] in the storage map.
Changes:
- Added
double_word_array.masmimplementing get/set operations for double-word array storage - Added comprehensive test coverage in
test_array.rsto verify double-word array functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/miden-standards/asm/standards/data_structures/double_word_array.masm |
New MASM module implementing double-word array abstraction with get and set procedures |
crates/miden-testing/src/kernel_tests/tx/test_array.rs |
Added test for double-word array get/set operations with initial and updated values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/miden-standards/asm/standards/data_structures/double_word_array.masm
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
Left some small comments. I think the important part is the padding/call documentation. Also, I did not re-review array.masm even though it appears as new here, not sure why.
crates/miden-standards/asm/standards/data_structures/double_word_array.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/data_structures/double_word_array.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/data_structures/double_word_array.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/data_structures/double_word_array.masm
Outdated
Show resolved
Hide resolved
| exec.active_account::get_map_item | ||
| swapw | ||
| # => [VALUE_0, VALUE_1, pad(16)] |
There was a problem hiding this comment.
Nit: Not really worth changing, but that swapw could probably be avoided if we loaded VALUE_1 first, and then VALUE_0.
| #! Inputs: [slot_id_prefix, slot_id_suffix, index, VALUE_0, VALUE_1, pad(5)] | ||
| #! Outputs: [OLD_VALUE_0, OLD_VALUE_1, pad(8)] |
There was a problem hiding this comment.
These procedures deal with padding and have Invocation: Call, but in the tests in test_array, they are exec-ed. And the array and double-array should be used as libraries rather than account components, right? So, iiuc, I think we should remove the pad comments here and change the doc comment to Invocation: exec. Applies here and in array.masm.
There was a problem hiding this comment.
good catch. I refactored both array.masm and double_array.masm to assume exec invocation. I think this was a leftover from how these data structures were meant to be used as components.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
|
@mmagician I've opened a new pull request, #2385, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: move double-word array changelog entry to 0.14 Co-authored-by: mmagician <8402446+mmagician@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mmagician <8402446+mmagician@users.noreply.github.com>
Similar to #2203, but lets the caller get/set double-words in one go with a convenient interface.
Internally, the words are stored under keys
[index, 0, 0, 0]and[index, 1, 0, 0]in the storage map.In the light of having a double-word array, should we also rename the
arraytosingle_word_array? (I would probably leave it as-is, I think simplearrayis descriptive enough)