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.

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

Event-based procedure index lookups #405

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Event-based procedure index lookups #405

merged 2 commits into from
Jan 8, 2024

Conversation

bobbinth
Copy link
Contributor

@bobbinth bobbinth commented Jan 8, 2024

This PR builds on #404 and partially addresses #273. Specifically, we no longer put the proc_root |-> proc_index map into the advice provider. Instead, we introduce PushAccountProcedureIndex event which the TransactionHost uses to populate the advice stack with the index of the relevant procedure.

Also, as a part of this PR I updated the tests to use MockHost - this resulted in quite a few changes in existing tests.

@phklive
Copy link
Contributor

phklive commented Jan 8, 2024

Looking into more and more comments I have seen that we have comments starting with a capital letter, others starting with lower case letters.

Would be good to have a convention about how to build comments and documentation around the codebase in masm and rs files.

@phklive
Copy link
Contributor

phklive commented Jan 8, 2024

Could you explain to me what the run_within methods mean?

Like run_within_host

@phklive
Copy link
Contributor

phklive commented Jan 8, 2024

I see the difference between the types and their mocked version, but can you explain to me why do we need them and why do we use them instead of the classical types?

Copy link
Contributor

@phklive phklive left a comment

Choose a reason for hiding this comment

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

LGTM! Mainly nits, typos and questions.

Code compiles and all tests are green ✅

miden-lib/asm/miden/kernels/tx/account.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernels/tx/account.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernels/tx/account.masm Show resolved Hide resolved
miden-lib/src/tests/test_note.rs Show resolved Hide resolved
miden-lib/src/tests/test_tx.rs Show resolved Hide resolved
objects/src/transaction/inputs.rs Outdated Show resolved Hide resolved
miden-tx/tests/common/mod.rs Outdated Show resolved Hide resolved
miden-tx/src/tests.rs Outdated Show resolved Hide resolved
mock/src/lib.rs Show resolved Hide resolved
miden-tx/src/host/mod.rs Show resolved Hide resolved
Base automatically changed from bobbin-tx-events to main January 8, 2024 20:15
@bobbinth bobbinth merged commit 2f43f5c into main Jan 8, 2024
6 checks passed
@bobbinth bobbinth deleted the bobbin-proc-index branch January 8, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants