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

Added handling testing option for get-block-hash. #5368

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Apr 7, 2024

This change is Reviewable

@orizi orizi requested a review from ArniStarkware April 7, 2024 15:40
@orizi orizi linked an issue Apr 7, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/testing.cairo line 77 at r1 (raw file):

pub fn set_block_hash(block_id: u64, value: felt252) {
    cheatcode::<'set_block_hash'>(array![block_id.into(), value].span());
}

Note block id and block number are different.
Not that I understand what block id means in the context of this repo.

Block id - is the address of the block saved to airospike.
Block number - the sequential number of the block in the chain.

Maybe the names were mixed somewhere in the process?

Suggestion:

/// Set the hash for a block.
/// Unset blocks values call would fail.
pub fn set_block_hash(block_number: u64, value: felt252) {
    cheatcode::<'set_block_hash'>(array![block_number.into(), value].span());
}

crates/cairo-lang-runner/src/casm_run/mod.rs line 143 at r1 (raw file):

    /// The simulated execution info.
    exec_info: ExecutionInfo,
    next_id: Felt252,

This value is unrelated to get_block_hash, is it?

Code quote:

    next_id: Felt252,

crates/cairo-lang-runner/src/casm_run/mod.rs line 144 at r1 (raw file):

    exec_info: ExecutionInfo,
    /// The test provided block hashes.
    block_hash: HashMap<u64, Felt252>,

Is this runner used for something other than tests? The docstring indicates it is not.

Suggestion:

    /// A mock history, mapping block number to the class hash.
    block_hash: HashMap<u64, Felt252>,

@ArniStarkware
Copy link
Contributor

corelib/src/starknet/testing.cairo line 77 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Note block id and block number are different.
Not that I understand what block id means in the context of this repo.

Block id - is the address of the block saved to airospike.
Block number - the sequential number of the block in the chain.

Maybe the names were mixed somewhere in the process?

I see they were only mixed here.

TL;DR: rename block_id -> block_number PLZ.

@orizi orizi force-pushed the orizi/get-block-hash-testing branch from 1ed0499 to be1d4f0 Compare April 8, 2024 07:53
Copy link
Collaborator Author

@orizi orizi 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: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @ArielElp, @ArniStarkware, and @gilbens-starkware)


corelib/src/starknet/testing.cairo line 77 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I see they were only mixed here.

TL;DR: rename block_id -> block_number PLZ.

Done.


crates/cairo-lang-runner/src/casm_run/mod.rs line 143 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This value is unrelated to get_block_hash, is it?

removing in pre-PR.

Copy link
Contributor

@ArniStarkware ArniStarkware 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: 2 of 4 files reviewed, all discussions resolved (waiting on @ArielElp and @gilbens-starkware)

@orizi orizi force-pushed the orizi/get-block-hash-testing branch from be1d4f0 to c4318bf Compare April 8, 2024 09:35
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ArielElp)

Copy link
Contributor

@gilbens-starkware gilbens-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

@orizi orizi force-pushed the orizi/get-block-hash-testing branch from c4318bf to 8bdd256 Compare April 8, 2024 09:53
@orizi orizi enabled auto-merge April 8, 2024 09:53
Copy link
Contributor

@ArniStarkware ArniStarkware 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

@orizi orizi added this pull request to the merge queue Apr 8, 2024
Merged via the queue into main with commit b6fdb0b Apr 8, 2024
43 checks passed
@orizi orizi deleted the orizi/get-block-hash-testing branch April 17, 2024 16:24
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.

feat: implement get_block_hash in the CASM runner
3 participants