Skip to content

modularize example state machine code for generic usage#89

Open
chung-thai-nguyen wants to merge 2 commits into
mainfrom
feature/refactor_default_state_machine
Open

modularize example state machine code for generic usage#89
chung-thai-nguyen wants to merge 2 commits into
mainfrom
feature/refactor_default_state_machine

Conversation

@chung-thai-nguyen
Copy link
Copy Markdown
Contributor

@chung-thai-nguyen chung-thai-nguyen commented Dec 24, 2024

[x] Move existing VM-related structs into a separate file for quick usage at zkmemory/src/default_state_machine.rs

@chung-thai-nguyen chung-thai-nguyen force-pushed the feature/refactor_default_state_machine branch from d8541fa to b91deb5 Compare December 24, 2024 10:11
@chung-thai-nguyen chung-thai-nguyen marked this pull request as ready for review December 24, 2024 18:22
Copy link
Copy Markdown
Contributor

@chiro-hiro chiro-hiro left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor — extracting StandardStateMachine/StandardInstruction into default_state_machine.rs is a good structural improvement and the $crate macro fix is a real correctness win.

A few things to address before merging:

  1. Dead RNG call (randomize.gen_range(u64::MAX / 2..u64::MAX) with the result discarded) appears in both the test in default_state_machine.rs and in 256bits-machine.rs. Remove the line or assign it if you intend to use it.

  2. Typo in example output: "Intruction: {:?}""Instruction: {:?}" in 256bits-machine.rs.

  3. memory-consistency example removed with no replacement: renaming examples/memory-consistency.rs to a library source file removes a runnable example. If this was intentional, please add a note to the README and check any CI jobs that ran cargo run --example memory-consistency.

  4. assert_eq!(sections.len(), 3) is always true at compile time. Not a bug, but it adds no test value.

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