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

Cheatcode function #3488

Merged
merged 9 commits into from
Jun 26, 2023
Merged

Cheatcode function #3488

merged 9 commits into from
Jun 26, 2023

Conversation

piotmag769
Copy link
Collaborator

@piotmag769 piotmag769 commented Jun 22, 2023

This PR is meant to introduce the cheatcode function. Cairo users can implement their own cheatcodes by injecting custom CairoHintProcessor. It will help Protostar to get rid of Cairo fork.


This change is Reviewable

Copy link
Collaborator Author

@piotmag769 piotmag769 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 8 files reviewed, all discussions resolved


crates/cairo-lang-sierra-to-casm/src/invocations/starknet/testing.rs line 130 at r1 (raw file):

                [input_start, input_end],
                [output_start, output_end],
            );

Tried using casm_build_extend! here but I would have to modify the macro definition for it to work - it treats selector as an input argument

            casm_build_extend! {casm_builder,
                tempvar output_start;
                tempvar output_end;
                hint StarknetHint::Cheatcode {
                    selector: selector,
                    input_start: input_start,
                    input_end: input_end
                } into {
                    output_start: output_start,
                    output_end: output_end
                };
                ap += 2;
            };

Code quote:

            casm_builder.add_hint(
                |[input_start, input_end], [output_start, output_end]| StarknetHint::Cheatcode {
                    selector: BigIntAsHex { value: c.selector.clone() },
                    input_start,
                    input_end,
                    output_start,
                    output_end,
                },
                [input_start, input_end],
                [output_start, output_end],
            );

Copy link
Collaborator

@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.

Reviewed 5 of 8 files at r1, all commit messages.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

        match self {
            StarknetHint::Cheatcode { .. } => {
                write!(f, "cheatcode")

Suggestion:

raise NotImplemented

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

                }
            }
            StarknetHint::Cheatcode {

Are you planning to add some sort of hook here?
if so add a TODO to make the plan more explicit.


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

                            let (base, offset) = extract_buffer(value);
                            get_ptr(vm, base, &offset)
                        };

maybe a non-print example?
just one of the other setter cheatcodes instead?
e.g. set_signature? (just replacing its implementation with this)


crates/cairo-lang-sierra-to-casm/src/invocations/starknet/testing.rs line 130 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Tried using casm_build_extend! here but I would have to modify the macro definition for it to work - it treats selector as an input argument

            casm_build_extend! {casm_builder,
                tempvar output_start;
                tempvar output_end;
                hint StarknetHint::Cheatcode {
                    selector: selector,
                    input_start: input_start,
                    input_end: input_end
                } into {
                    output_start: output_start,
                    output_end: output_end
                };
                ap += 2;
            };

generally - we do not use non CASM options in hints.
But it is sensible to allow it.
(allowing it through the macro should be pretty easy, but let leave it for now)


crates/cairo-lang-starknet/cairo_level_tests/cheatcode_tests.cairo line 14 at r1 (raw file):

    assert(1 == 1, '');
}

this tests nothing - but you can actually implement set_block_number with the cheatcode.

Code quote:

#[test]
#[available_gas(300000)]
fn test_set_block_number() {
    let mut data = ArrayTrait::new();
    data.append(42);

    let r = cheatcode::<'print'>(data.span());

    assert(1 == 1, '');
}

Copy link
Collaborator Author

@piotmag769 piotmag769 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: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @orizi and @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

        match self {
            StarknetHint::Cheatcode { .. } => {
                write!(f, "cheatcode")

Will do before merging - otherwise there is an error when running ./scripts/starknet_test.sh


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

Previously, orizi wrote…

Are you planning to add some sort of hook here?
if so add a TODO to make the plan more explicit.

I would leave it as it is (maybe without the example cheatcode) . There is already a way to inject a custom CairoHintProcessor afaik, it should be enough.


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

Previously, orizi wrote…

maybe a non-print example?
just one of the other setter cheatcodes instead?
e.g. set_signature? (just replacing its implementation with this)

Done


crates/cairo-lang-starknet/cairo_level_tests/cheatcode_tests.cairo line 14 at r1 (raw file):

Previously, orizi wrote…

this tests nothing - but you can actually implement set_block_number with the cheatcode.

Done.

@piotmag769 piotmag769 requested a review from orizi June 22, 2023 22:54
Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Will do before merging - otherwise there is an error when running ./scripts/starknet_test.sh

That is the python hints - this shouldn't affect this repo.


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

Previously, piotmag769 (Piotr Magiera) wrote…

Done

and replace the high level implementation of set_block_number with this.

@piotmag769 piotmag769 requested a review from orizi June 25, 2023 13:54
Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 8 files at r1, 2 of 2 files at r2.
Reviewable status: 1 of 8 files reviewed, 4 unresolved discussions (waiting on @orizi)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, orizi wrote…

That is the python hints - this shouldn't affect this repo.

Misunderstanding, done.


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

Previously, orizi wrote…

and replace the high level implementation of set_block_number with this.

Done.

Copy link
Collaborator

@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.

Reviewed 5 of 7 files at r3, all commit messages.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @piotmag769)

a discussion (no related file):
@yuvalsw added for 2nd eye.
@ilyalesokhin-starkware - for making sure deleting the hint is fine.


Copy link
Collaborator

@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.

Reviewed 2 of 7 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 196 at r3 (raw file):

}

/// Libfunc for creating a constant felt252.

Suggestion:

/// Libfunc for a general cheatcode.

Copy link
Collaborator

@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.

Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @piotmag769)

@ilyalesokhin-starkware
Copy link
Contributor

why is it called cheatcode?

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-casm/src/hints/mod.rs line 707 at r3 (raw file):

        match self {
            StarknetHint::Cheatcode { .. } => {
                write!(f, "raise NotImplementedError")

Shouldn't the compilation fail here?

Copy link
Collaborator

@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: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 707 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

Shouldn't the compilation fail here?

do you mean panic?
this shouldn't actually be in any generated printed casm.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware and @piotmag769)


corelib/src/starknet/testing.cairo line 24 at r3 (raw file):

) -> Span<felt252> implicits() nopanic;

fn set_block_number(block_number: u64) {

please document.


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

        match self {
            StarknetHint::Cheatcode { .. } => {
                write!(f, "cheatcode")

Please use the same format as others, and also print the selector.


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

                output_start: _,
                output_end: _,
            } => {

Suggestion:

            StarknetHint::Cheatcode {
                selector,
                input_start,
                input_end,
                ..
            } => {

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

                        }
                    }
                    _ => Err(HintError::CustomHint(Box::from("Unknown hint")))?,

Please print the unknown selector.


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

                        }

                        self.starknet_state.exec_info.block_info.block_number = input[0].clone();

Suggestion:

                        match &input[..] {
                            [input] => {
                                    self.starknet_state.exec_info.block_info.block_number = input.clone();
                                }
                            _ => {
                                return Err(HintError::CustomHint(Box::from(
                                    "set_block_number cheatcode invalid args: pass span of an array \
                                    with exactly one element",
                                )));
                        }

crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

        &self,
        context: &dyn SignatureSpecializationContext,
        _args: &[GenericArg],

Please verify args is empty.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-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 3 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 707 at r3 (raw file):

Previously, orizi wrote…

do you mean panic?
this shouldn't actually be in any generated printed casm.

Is it hard to return a failure here?

I guess we have an issue with the Display using pythonic hint in general, but it is beyond this PR,

Changed to non-blocking.

Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @orizi, @piotmag769, and @yuvalsw)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, yuvalsw wrote…

Please use the same format as others, and also print the selector.

I was asked by @orizi to do it the way it is done now.


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

Previously, yuvalsw wrote…

Please print the unknown selector.

Done.


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

                        }

                        self.starknet_state.exec_info.block_info.block_number = input[0].clone();

Done


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, yuvalsw wrote…

Please verify args is empty.

It is not empty, there is always a selector generic arg there. We just don't use it here since the signature will always be the same. I can pass an empty array to this function (it is called in specialize) and assert that it is empty if that suits you better, but I don't see how it is beneficial.


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 196 at r3 (raw file):

}

/// Libfunc for creating a constant felt252.

Done.

@piotmag769 piotmag769 requested review from orizi and yuvalsw June 25, 2023 16:28
Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

It is a name used by Protostar that originally comes from Foundry. It is called this way since some cheatcodes allow you to illegally (that's where "cheat" comes from) modify the state of a blockchain for testing purposes

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @orizi, @piotmag769, and @yuvalsw)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

I was asked by @orizi to do it the way it is done now.

Oh wait, you are looking at the r1 revision, look at the latest one.
Also now I noticed: I think there is a mistake in Display implementation of PopLog, raise NotImplemented is not a valid python instruction - https://docs.python.org/3/library/constants.html#NotImplemented. Should I correct it?

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-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: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @orizi, @piotmag769, and @yuvalsw)

a discussion (no related file):

Previously, orizi wrote…

@yuvalsw added for 2nd eye.
@ilyalesokhin-starkware - for making sure deleting the hint is fine.

I don't see a problem with deleting the hint.


Copy link
Collaborator

@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: 7 of 8 files reviewed, 5 unresolved discussions (waiting on @piotmag769 and @yuvalsw)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Oh wait, you are looking at the r1 revision, look at the latest one.
Also now I noticed: I think there is a mistake in Display implementation of PopLog, raise NotImplemented is not a valid python instruction - https://docs.python.org/3/library/constants.html#NotImplemented. Should I correct it?

Would be appreciated - I mostly cared that if someone writes code that is not runnable in python - an error would occur soon (and not just after we try to access the value not provided by the hint)


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

                            input.push(value.into_owned());
                            curr += 1;
                        }

extract_relocatable assuming https://reviewable.io/reviews/starkware-libs/cairo/3503 was merged.
otherwise - just the moving out of the match, and the vm_get_range usage.

Suggestion:

                let input_start = extract_relocatable(vm, input_start)?;
                let input_end = extract_relocatable(vm, input_end)?;
                let inputs = vm_get_range(vm, input_start, input_end)?;
                match selector {
                    "set_block_number" => {

@mkaput mkaput self-requested a review June 26, 2023 07:29
Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mkaput, @piotmag769, and @yuvalsw)

Copy link
Collaborator Author

@piotmag769 piotmag769 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: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware, @mkaput, @orizi, and @yuvalsw)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, orizi wrote…

Would be appreciated - I mostly cared that if someone writes code that is not runnable in python - an error would occur soon (and not just after we try to access the value not provided by the hint)

Unfortunately correcting it makes the starknet tests fail - there must be a bug in the logic behind python hints, but that's out of the scope of this PR


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

Previously, orizi wrote…

extract_relocatable assuming https://reviewable.io/reviews/starkware-libs/cairo/3503 was merged.
otherwise - just the moving out of the match, and the vm_get_range usage.

Done.

Copy link
Member

@mkaput mkaput 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 8 files at r1, 4 of 7 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @orizi, @piotmag769, and @yuvalsw)


corelib/src/starknet/testing.cairo line 23 at r5 (raw file):

// general cheatcode function used to simplify implementation of starknet testing functions
// external users of the cairo crates can also implement their own cheatcodes
// by injecting custom CairoHintProcessor

Suggestion:

// A general cheatcode function used to simplify implementation of Starknet testing functions.
// External users of the cairo crates can also implement their own cheatcodes
// by injecting custom `CairoHintProcessor`.

corelib/src/starknet/testing.cairo line 24 at r5 (raw file):

// external users of the cairo crates can also implement their own cheatcodes
// by injecting custom CairoHintProcessor
extern fn cheatcode<const selector: felt252>(

You have placed this function in starknet::testing module, while technically this is Starknet independent? Shouldn't be it place in core::testing module instead?


corelib/src/starknet/testing.cairo line 28 at r5 (raw file):

) -> Span<felt252> implicits() nopanic;

// set_block_number high level implementation using general cheatcode function

This doc comment does not say what does this function do


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

    #[codec(index = 1)]
    SetBlockTimestamp { value: ResOperand },
    #[codec(index = 2)]

isn't changing indices a breaking change in runtime? shouldn't you just skip SetBlockNumber / 2?


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

                })?;
                match selector {
                    "set_block_number" => {

nit, consider extracting this match to a separate function for readability


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It is not empty, there is always a selector generic arg there. We just don't use it here since the signature will always be the same. I can pass an empty array to this function (it is called in specialize) and assert that it is empty if that suits you better, but I don't see how it is beneficial.

I think you should definitely assert that it is non-empty then. Assertions are always beneficial. In this case, they guard you early from bugs that can happen when you forget to synchronize two pieces of code.

Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @piotmag769 and @yuvalsw)


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think you should definitely assert that it is non-empty then. Assertions are always beneficial. In this case, they guard you early from bugs that can happen when you forget to synchronize two pieces of code.

do note that it is checked in the wrapping function (20 lines down) - but checking here won't hurt.

Copy link
Collaborator

@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: all files reviewed, 10 unresolved discussions (waiting on @piotmag769 and @yuvalsw)


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, orizi wrote…

do note that it is checked in the wrapping function (20 lines down) - but checking here won't hurt.

also - specialize isn't the only actual place that calls it - otherwise it wouldn't have existed.

Copy link
Collaborator Author

@piotmag769 piotmag769 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 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @mkaput, @orizi, and @yuvalsw)


corelib/src/starknet/testing.cairo line 23 at r5 (raw file):

// general cheatcode function used to simplify implementation of starknet testing functions
// external users of the cairo crates can also implement their own cheatcodes
// by injecting custom CairoHintProcessor

Done.


corelib/src/starknet/testing.cairo line 24 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

You have placed this function in starknet::testing module, while technically this is Starknet independent? Shouldn't be it place in core::testing module instead?

Not sure, vast majority of the cheatcodes will use starknet state, it is even declared as StarknetHint


corelib/src/starknet/testing.cairo line 28 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

This doc comment does not say what does this function do

Done.


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

isn't changing indices a breaking change in runtime? shouldn't you just skip SetBlockNumber / 2?

No idea. Is it true @orizi ?


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

Previously, mkaput (Marek Kaput) wrote…

nit, consider extracting this match to a separate function for readability

I would leave it for now, can be done when implementing new cheatcodes which is out of scope of this PR.


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think you should definitely assert that it is non-empty then. Assertions are always beneficial. In this case, they guard you early from bugs that can happen when you forget to synchronize two pieces of code.

Done.

Copy link
Collaborator Author

@piotmag769 piotmag769 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: 6 of 8 files reviewed, 10 unresolved discussions (waiting on @mkaput, @orizi, and @yuvalsw)


crates/cairo-lang-sierra/src/extensions/modules/starknet/testing.rs line 213 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Done.

Done.

@piotmag769 piotmag769 requested review from orizi and mkaput June 26, 2023 09:23
Copy link
Collaborator

@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.

:lgtm:

Reviewed 2 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mkaput and @yuvalsw)

Copy link
Contributor

@yuvalsw yuvalsw 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 1 of 3 files at r4, 2 of 2 files at r7, 2 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mkaput and @orizi)

Copy link
Member

@mkaput mkaput 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 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi and @piotmag769)


corelib/src/starknet/testing.cairo line 24 at r5 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Not sure, vast majority of the cheatcodes will use starknet state, it is even declared as StarknetHint

Do we expect having pure-Cairo cheatcodes?

Copy link
Collaborator

@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: all files reviewed, 2 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-casm/src/hints/mod.rs line 709 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Unfortunately correcting it makes the starknet tests fail - there must be a bug in the logic behind python hints, but that's out of the scope of this PR

yeah - there's a PR merged to dev-v2.0.0 taking care of it.
ignore PopLog for the time being.

Copy link
Member

@mkaput mkaput 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: all files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/starknet/testing.cairo line 24 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Do we expect having pure-Cairo cheatcodes?

Discussed this privately: we concluded with @piotmag769 that when we will find a need for starknet-agnostic cheatcode (which is non-trivial due to having to maintain the state under the hood), we can deprecate this one

Copy link
Collaborator

@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: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

No idea. Is it true @orizi ?

this is used by full node - and they should be able to handle changes - so we should be fine.
Still making sure @dan-starkware ?

Copy link
Collaborator

@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.

Reviewed 1 of 2 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dan-starkware)

@yair-starkware
Copy link
Contributor

crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, orizi wrote…

this is used by full node - and they should be able to handle changes - so we should be fine.
Still making sure @dan-starkware ?

We store the casms in storage, so removing variants / modifying the indices requires us to implement storage migration.
If possible, please avoid doing so.
@dan-starkware - can we still get away with resyncing?

Copy link
Collaborator Author

@piotmag769 piotmag769 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 2 files at r8, 1 of 1 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dan-starkware)


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, yair-starkware (Yair) wrote…

We store the casms in storage, so removing variants / modifying the indices requires us to implement storage migration.
If possible, please avoid doing so.
@dan-starkware - can we still get away with resyncing?

I can revert @yair-starkware

Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

I can revert @yair-starkware

does the fact these are indices that should never appear in the db affect anything?

@yair-starkware
Copy link
Contributor

crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, orizi wrote…

does the fact these are indices that should never appear in the db affect anything?

WDYM never appear in the db? They are part of the encoding

Copy link
Collaborator

@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: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)


crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, yair-starkware (Yair) wrote…

WDYM never appear in the db? They are part of the encoding

the actual values - the changed values would never be part of valid CASM code.

@yair-starkware
Copy link
Contributor

crates/cairo-lang-casm/src/hints/mod.rs line 53 at r5 (raw file):

Previously, orizi wrote…

the actual values - the changed values would never be part of valid CASM code.

Discussed with Ori - these variants are for tests.
Ok to modify them.
Thanks for checking with us.

Copy link
Collaborator Author

@piotmag769 piotmag769 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: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

Copy link
Collaborator

@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: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

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.

6 participants