Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Set to 1 all faucet account storage offsets #875

Merged
merged 16 commits into from
Sep 17, 2024
Merged

Set to 1 all faucet account storage offsets #875

merged 16 commits into from
Sep 17, 2024

Conversation

phklive
Copy link
Contributor

@phklive phklive commented Sep 13, 2024 •
edited
Loading

In this PR I propose to set to 1 all procedures storage offsets of faucet accounts.

Closes: #864 and #863

@phklive phklive changed the base branch from main to next September 13, 2024 13:37
@phklive phklive mentioned this pull request Sep 16, 2024
Closed
@phklive phklive marked this pull request as ready for review September 16, 2024 14:55
@phklive phklive requested review from Fumuran, igamigo and bobbinth and removed request for Fumuran September 16, 2024 15:13
bobbinth
bobbinth reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Not a full review yet (I haven't yet looked at MASM code), but I left a few comments inline. The main one is the question about needing to add location parameter for slot additions.

objects/src/testing/storage.rs Outdated Show resolved Hide resolved
objects/src/testing/account_code.rs Show resolved Hide resolved
objects/src/accounts/code/mod.rs Show resolved Hide resolved
objects/src/accounts/code/mod.rs Outdated Show resolved Hide resolved
objects/src/testing/account.rs Outdated Show resolved Hide resolved
bobbinth
bobbinth reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I reviewed all non-test code (and some test code as well), and left some more comments inline.

miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
miden-lib/src/accounts/faucets/mod.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/account.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/account.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/account.masm Outdated Show resolved Hide resolved
@phklive phklive mentioned this pull request Sep 17, 2024
Open
@igamigo igamigo mentioned this pull request Sep 17, 2024
Merged
igamigo
igamigo approved these changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@igamigo igamigo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Missing some context for the MASM changes, but the code looks good. Left some minor comments.
EDIT: For some reason the comments did not show up (because I started reviewing before the newest commit?), adding them right now.

igamigo
igamigo reviewed Sep 17, 2024
View reviewed changes
miden-tx/src/tests/kernel_tests/test_account.rs Outdated Show resolved Hide resolved
objects/src/accounts/code/mod.rs Outdated Show resolved Hide resolved
objects/src/testing/storage.rs Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/account.masm Outdated Show resolved Hide resolved
bobbinth
bobbinth approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@bobbinth bobbinth merged commit ac84daa into next Sep 17, 2024
8 checks passed
@bobbinth bobbinth deleted the phklive-faucet-offset branch September 17, 2024 19:31
This was referenced Sep 17, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igamigo igamigo igamigo approved these changes

@bobbinth bobbinth bobbinth approved these changes

@Fumuran Fumuran Awaiting requested review from Fumuran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Offset faucet procedures by 1 to enable correct authentication and reserved slot access
3 participants
@phklive @igamigo @bobbinth