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

add adapter tests #245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add adapter tests #245

wants to merge 1 commit into from

Conversation

Stavbe
Copy link
Contributor

@Stavbe Stavbe commented Dec 10, 2024

Add a unit test to ensure instructions are correctly mapped with the adapter.
The only components that can’t be tested using casm! are:

jmp rel [ap/fp + offset]
jump abs [[ap/fp + offset1] + offset2]
[ap/fp + offset0] = [[ap/fp + offset1] + offset


This change is Reviewable

@Stavbe Stavbe self-assigned this Dec 10, 2024
Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

so double derefs? doc that somewhere

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Stavbe)

@Stavbe Stavbe force-pushed the stav/add_tests branch 2 times, most recently from 273805f to 7ac2ebb Compare December 16, 2024 12:42
Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 641 at r3 (raw file):

// Ensures that instructions are correctly mapped with the adapter.
// All components were checked except:

this needs to be a TODO


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 1030 at r3 (raw file):

        let input = input_from_plain_casm(instructions, false);
        let casm_states_by_opcode = input.state_transitions.casm_states_by_opcode;
        println!("{:?}", casm_states_by_opcode.counts());

remove

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Added

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 641 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

this needs to be a TODO

We won't be able to test those using the same terminology because the casm macro does not support it, and according to Ori Ziv, it will not be expanded to support it.


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 1030 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

remove

Done.

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 641 at r3 (raw file):

Previously, Stavbe wrote…

We won't be able to test those using the same terminology because the casm macro does not support it, and according to Ori Ziv, it will not be expanded to support it.

still a todo

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 641 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

still a todo

todo what ?

@Stavbe Stavbe force-pushed the stav/add_tests branch 3 times, most recently from 1b5e0bc to 3892105 Compare December 19, 2024 13:44
Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 641 at r3 (raw file):

Previously, Stavbe wrote…

todo what ?

Done.

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 644 at r5 (raw file):

}

// Ensures that instructions are correctly mapped with the adapter.

Suggestion:

/// Tests instructions mapping.

stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 645 at r5 (raw file):

// Ensures that instructions are correctly mapped with the adapter.
// All components were checked except:

Suggestion:

/// Every opcode is tested except:

stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 650 at r5 (raw file):

// TODO(Stav): Find a way to check those without the casm macro.
#[cfg(test)]
mod tests {

Suggestion:

mod mappings_tests

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 644 at r5 (raw file):

}

// Ensures that instructions are correctly mapped with the adapter.

Done.


stwo_cairo_prover/crates/prover/src/input/state_transitions.rs line 645 at r5 (raw file):

// Ensures that instructions are correctly mapped with the adapter.
// All components were checked except:

Done.

Copy link
Contributor

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r5.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @Stavbe)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 215 at r6 (raw file):

        // TODO(Ohad): update counts after adding mull small.
        assert_eq!(components.mul_opcode_is_small_t_is_imm_t.len(), 0);
        assert_eq!(components.mul_opcode_is_small_t_is_imm_f.len(), 0);

can dev mode be turned off here and revert that?

Copy link
Contributor Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ohad-starkware and @Stavbe)


stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 215 at r6 (raw file):

Previously, ohad-starkware (Ohad) wrote…

can dev mode be turned off here and revert that?

Done.

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