Add pop logs to testing

[ericnordelo]

This change is 

[spapinistarkware]

Reviewed 1 of 8 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ericnordelo)

---

corelib/src/starknet/testing.cairo line 3 at r2 (raw file):

use starknet::ContractAddress;

#[derive(Drop)]

Derive Clone as well.

---

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

#[derive(Serialize, Deserialize, Debug, Eq, PartialEq, Clone)]
pub enum StarknetHint {
    SystemCall {

Fix formatting

---

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

                )
            }
            StarknetHint::PopLog {

@orizi what should we put here?

[ericnordelo]

Derive Clone as well.

Array<felt252> does not implement the Clone trait. We can't derive clone on Log, right?

[spapinistarkware]

Array does implement Clone

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @ericnordelo)

[ericnordelo]

Array does implement Clone

I meant @Array sorry. For some reason I'm not sure of, when deriving clone on this struct:

#[derive(Drop, Clone)]
struct Log {
    keys: Array<felt252>,
    data: Array<felt252>,
}

I'm getting:

error: Method `clone` not found on type "@core::array::Array::<core::felt252>". Did you import the correct trait and impl?
 --> impls:5:29
            keys: self.keys.clone(),

Not sure why struct members are treated as snapshots here.

[orizi]

Reviewable status: 4 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

extern fn set_nonce(nonce: felt252) implicits() nopanic;
extern fn set_signature(signature: Span<felt252>) implicits() nopanic;
extern fn pop_log(address: ContractAddress) -> Option<Log> implicits() nopanic;

maybe just a tuple of two spans? This will remove the need for the underlying code to know the structure (and wrapping with something external should probably happen anyway at some point.
@spapinistarkware WDYT as well?

---

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

Previously, spapinistarkware (Shahar Papini) wrote…

@orizi what should we put here?

I'd rather fail here than fail when reading the cells.
Idk if the python context contains any relevant info.

---

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

    /// A mapping from contract address to logs.
    logs: HashMap<Felt252, VecDeque<Log>>,
    /// The simulated execution info.

If you return spans instead of arrays you can just return the original values, not even really process anything.

[ericnordelo]

maybe just a tuple of two spans? This will remove the need for the underlying code to know the structure (and wrapping with something external should probably happen anyway at some point.
@spapinistarkware WDYT as well?

An optional tuple you mean? We still need to return something when there are no events. I have nothing against using a tuple of spans for this. Interested in @spapinistarkware thoughts as well.

[orizi]

Reviewable status: 4 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

Previously, orizi wrote…

maybe just a tuple of two spans? This will remove the need for the underlying code to know the structure (and wrapping with something external should probably happen anyway at some point.
@spapinistarkware WDYT as well?

I indeed meant Option<(Span<felt252>, Span<felt252>)>

[spapinistarkware]

Sounds good. It's also almost zero changes in sierra.

Reviewed 1 of 5 files at r3, all commit messages.
Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @orizi)

---

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

Previously, orizi wrote…

I'd rather fail here than fail when reading the cells.
Idk if the python context contains any relevant info.

So, output something like "raise NotImplemented"?

[ericnordelo]

If you return spans instead of arrays you can just return the original values, not even really process anything.

What do you mean by the original values in this context? How is the Span represented in memory layout? The same as an Array (span_start,span_end)? what do you mean without processing anything? cc @spapinistarkware

[orizi]

please answer in https://reviewable.io/reviews/starkware-libs/cairo/3300#- so that we'd get different threads in different threads.

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

[orizi]

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

Previously, orizi wrote…

If you return spans instead of arrays you can just return the original values, not even really process anything.

I mean that if instead of reading an Array of felts from the given data, and saving it you would have just saved 4 relocatable values (start and end ptr per array) - you'd be able to return the exact same value without allocation at all.
but this also means that working with spans makes more sense, as you'd return something that is not appendable.

[ericnordelo]

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @orizi and @spapinistarkware)

---

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

Previously, orizi wrote…

I mean that if instead of reading an Array of felts from the given data, and saving it you would have just saved 4 relocatable values (start and end ptr per array) - you'd be able to return the exact same value without allocation at all.
but this also means that working with spans makes more sense, as you'd return something that is not appendable.

I'm not sure what you mean by no allocation at all. Do you mean allocating the segment in the EmitEvent hint instead of the PopLog one? and storing the segment pointers instead of the vectors? We still need to allocate for this. Remember I don't have enough context on the compiler, I probably need more explanation on the whys and hows, thank you for your patience.

[orizi]

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

Previously, ericnordelo (Eric Nordelo) wrote…

I'm not sure what you mean by no allocation at all. Do you mean allocating the segment in the EmitEvent hint instead of the PopLog one? and storing the segment pointers instead of the vectors? We still need to allocate for this. Remember I don't have enough context on the compiler, I probably need more explanation on the whys and hows, thank you for your patience.

the VecDeque is needed anyway - but the main thing is if we just keep pointers to the original memory, since it is immutable - we will get the same values if we just return these.
anyway - no need to change the implementation here (it would also be good for cases where we fill the information not from an 'emit event' context.
for now i still suggest that at the high level we'd be using spans - just to allow for different backend implementations.

[ericnordelo]

It is normal that the feedback from the clippy script is different locally and from the GitHub action?

[orizi]

Reviewed 2 of 5 files at r3, 2 of 5 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

                let logs = self.starknet_state.logs.entry(contract_address).or_default();

                let log = logs.pop_front();

if let Some((keys, values)) = logs.pop_front() {

---

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

        }
    }
    casm_build_extend! {casm_builder, ap += 0; };

This is not required in your case, make sure it isn't added

[ericnordelo]

Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @orizi and @spapinistarkware)

---

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

Previously, orizi wrote…

if let Some((keys, values)) = logs.pop_front() {

This should be keys and data (instead of values), right? When you say keys and values it looks like a mapping of some sort, but keys and data are all just event data, where keys are supposed to be the indexed one. Correct me if my understanding of this is mistaken.

[ericnordelo]

Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @orizi and @spapinistarkware)

---

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

Previously, orizi wrote…

This is not required in your case, make sure it isn't added

Is not added in the PopLog case. There is a return on line 96.

[orizi]

Reviewed 1 of 5 files at r5.
Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

---

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

Previously, ericnordelo (Eric Nordelo) wrote…

This should be keys and data (instead of values), right? When you say keys and values it looks like a mapping of some sort, but keys and data are all just event data, where keys are supposed to be the indexed one. Correct me if my understanding of this is mistaken.

You are correct afaik, this was mostly about the rust styling

---

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

Previously, ericnordelo (Eric Nordelo) wrote…

Is not added in the PopLog case. There is a return on line 96.

Oh, I missed the return, nm

[orizi]

Reviewed 1 of 5 files at r5, 1 of 1 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @ericnordelo and @spapinistarkware)

[ericnordelo]

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @spapinistarkware)

---

corelib/src/starknet/testing.cairo line 3 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Derive Clone as well.

To set the comment as part of the thread.

#3300 (comment)

---

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

Previously, spapinistarkware (Shahar Papini) wrote…

So, output something like "raise NotImplemented"?

Let me know if I should update it.

[spapinistarkware]

Reviewed 2 of 5 files at r3, 4 of 5 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ericnordelo)

[orizi]

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

---

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

Previously, ericnordelo (Eric Nordelo) wrote…

Let me know if I should update it.

you should - i don't want to get an unclear error when running this in playground.

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

[ericnordelo]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)

---

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

Previously, orizi wrote…

you should - i don't want to get an unclear error when running this in playground.

What should I put here then?

[orizi]

Reviewed 5 of 6 files at r8, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

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

Previously, ericnordelo (Eric Nordelo) wrote…

What should I put here then?

something that would fail here.
@spapinistarkware 's suggestion sounds good.

[ericnordelo]

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)

---

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

Previously, orizi wrote…

something that would fail here.
@spapinistarkware 's suggestion sounds good.

Done.

[orizi]

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

---

crates/cairo-lang-casm/src/builder.rs line 847 at r9 (raw file):

    };
    ($builder:ident, hint $hint_lead:ident::$hint_name:ident {
            $($input_name:ident : $input_value:ident),*

Can you please try to delete this and see if it still works?
I believe I made changes that made this part not needed.

[ericnordelo]

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)

---

crates/cairo-lang-casm/src/builder.rs line 847 at r9 (raw file):

Previously, orizi wrote…

Can you please try to delete this and see if it still works?
I believe I made changes that made this part not needed.

Done.

[orizi]

:lgtm

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

[spapinistarkware]

Reviewed 4 of 6 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)