-
Notifications
You must be signed in to change notification settings - Fork 104
feat: Single-word Array component
#2203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
505aa25 to
5479b77
Compare
|
@copilot add change log entry for this PR 2203 |
|
@mmagician I've opened a new pull request, #2204, to work on those changes. Once the pull request is ready, I'll request review from you. |
8263624 to
b5f4cdb
Compare
chore: simplify test chore: be explicit about padding
* Initial plan * chore: add changelog entry for PR 2203 Co-authored-by: mmagician <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mmagician <[email protected]>
b5f4cdb to
e708a51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a single-word Array account component that provides a simple, reusable array data structure for Miden accounts. The implementation wraps a StorageMap where array indices are mapped to keys of the form [index, 0, 0, 0].
Key changes:
- Added
Arraystruct with methods for creating arrays with initial elements or from slices - Implemented MASM procedures (
getandset) for array access with configurable storage slots - Added comprehensive unit tests and integration tests demonstrating the component's usage
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/miden-standards/src/account/array.rs |
Core implementation of the Array component with constructors, MASM generation, and conversion to AccountComponent |
crates/miden-standards/asm/account_components/array.masm |
MASM template defining get and set procedures for array operations |
crates/miden-standards/src/account/mod.rs |
Adds array module to the account components public API |
crates/miden-testing/src/kernel_tests/tx/test_array.rs |
Integration test verifying get/set operations work correctly in transaction context |
crates/miden-testing/src/kernel_tests/tx/mod.rs |
Includes the new test module |
CHANGELOG.md |
Documents the addition of the Array component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Looks great! This is a great feature! When reviewing the implementation, it reminded me of this issue for |
| # The MASM code template for the Array Account Component. | ||
| # | ||
| # NOTE: This file is used as a template. The placeholder {{DATA_SLOT}} is substituted | ||
| # at construction time with the actual slot name provided by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why make this a component rather than just a utility? That is, we could provide the storage slot name as a parameter to get/set procedures and it would work similar to how storage slots/maps work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this would be preferable. One consequence of "templating" is that the Array account component as it is now would not be detectable in an AccountInterface because it doesn't have a stable MAST root. Not sure if it's important for this component, but whenever possible, I think we should avoid "templating".
We eventually want to have double-word arrays & double-word queues. Once we agreed on the structure and where the files live, it's going to be trivial to extend to double-word array. A queue will require an extra storage slot to hold the metadata, though.
Features:
[index, 0, 0, 0]set(index: Felt, VALUE: Word) -> OLD_VALUE: Word&get(index: Felt) -> VALUE: WordUsage: