chore(starknet_l1_provider): add l1 scraper test

[giladchase]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-20T21:46:20Z
• Started at 2024-12-24T14:05:40Z
• Started at 2025-01-06T10:48:30Z
• Started at 2025-01-06T14:51:12Z
• Started at 2025-01-06T18:52:08Z
• Started at 2025-01-07T07:47:15Z
• Started at 2025-01-07T07:51:59Z
• Started at 2025-01-08T15:16:52Z
• Started at 2025-01-12T15:21:22Z
• Started at 2025-01-13T06:37:44Z
• Started at 2025-01-14T10:36:20Z
• Started at 2025-01-14T12:51:53Z
• Started at 2025-01-14T12:57:32Z
• Started at 2025-01-14T13:11:46Z
• Started at 2025-01-14T16:34:48Z
• Started at 2025-01-15T08:23:30Z
• Started at 2025-01-15T12:09:21Z
• Started at 2025-01-15T13:32:38Z
• Started at 2025-01-15T13:32:53Z
• Started at 2025-01-15T13:32:53Z
• Started at 2025-01-16T13:20:01Z
• Started at 2025-01-16T13:42:52Z

[elintul]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @giladchase and @matan-starkware)

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 52 at r1 (raw file):

    let base_layer = EthereumBaseLayerContract::new(config);

    // Deploy fresh starknet contract on anvil from the bytecode in the json file.

😬

Suggestion:

Deploy a fresh Starknet contract on Anvil from the bytecode in the JSON file.

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 54 at r1 (raw file):

    // Deploy fresh starknet contract on anvil from the bytecode in the json file.
    let contract = Starknet::deploy(base_layer.contract.provider().clone()).await.unwrap();
    println!("Deployed contract at: {}", contract.address());

Remove.

Code quote:

println!("Deployed contract at: {}", contract.address());

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 62 at r1 (raw file):

            base_layer,
            event_identifiers_to_track(),
        ),

Variable, please.
Also, give Default::default() a name.

Code quote:

        L1Scraper::new(
            Default::default(),
            fake_client.clone(),
            base_layer,
            event_identifiers_to_track(),
        ),

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 78 at r1 (raw file):

    let l2_contract_address = "0x12";
    let l2_entry_point = "0x34";
    let send_message_to_l2 = scraper.base_layer.contract.sendMessageToL2(

And message_to_l2_1.

Suggestion:

message_to_l2_0

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 91 at r1 (raw file):

    // Send the txs.
    send_message_to_l2.send().await.unwrap().get_receipt().await.unwrap();
    send_message_to_l2_2.send().await.unwrap().get_receipt().await.unwrap();

for loop? Maybe also for creating them and their expected (you can define payloads and loop over them)?

Suggestion:

    // Send the transactions.
    send_message_to_l2.send().await.unwrap().get_receipt().await.unwrap();
    send_message_to_l2_2.send().await.unwrap().get_receipt().await.unwrap();

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 96 at r1 (raw file):

    let expected_internal_l1_tx = L1HandlerTransaction {
        version: expected_version,
        nonce: Nonce(StarkHash::ZERO),

Suggestion:

nonce!(0)

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 115 at r1 (raw file):

    let first_expected_log = Event::L1HandlerTransaction(tx.clone());
    let expected_internal_l1_tx_2 = L1HandlerTransaction {
        nonce: Nonce(StarkHash::ONE),

Suggestion:

nonce!(1)

[giladchase]

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @elintul and @matan-starkware)

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 96 at r1 (raw file):

    let expected_internal_l1_tx = L1HandlerTransaction {
        version: expected_version,
        nonce: Nonce(StarkHash::ZERO),

nonce! is leaking starknet-type-core, it forces the user to use that dep (the solution is for SNAPI to re-export starknet-types-core::path::to::Felt).
Used felt! to comply closest with the request

[matan-starkware]

Reviewed 2 of 4 files at r2, 2 of 2 files at r4, 1 of 3 files at r5, 1 of 1 files at r7, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 32 at r10 (raw file):

// see https://github.com/foundry-rs/foundry/tree/master/crates/anvil.
const DEFAULT_ANVIL_ACCOUNT_ADDRESS: &str = "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266";
const DEFAULT_ANVIL_DEPLOY_ADDRESS: &str = "0x5fbdb2315678afecb367f032d93f642f64180aa3";

Felt::from_hex_unchecked is a const fn, so you can build the felt here if you want.

Code quote:

const DEFAULT_ANVIL_ACCOUNT_ADDRESS: &str = "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266";
const DEFAULT_ANVIL_DEPLOY_ADDRESS: &str = "0x5fbdb2315678afecb367f032d93f642f64180aa3";

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 42 at r10 (raw file):

// TODO: Replace EthereumBaseLayerContract with a mock that has a provider initialized with
// `with_recommended_fillers`, in order to be able to create txs from non-default users.
async fn scraper(

Why not make this the fixture instead? Is the fixture reused across tests (I presume not)

Code quote:

async fn scraper(

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 92 at r10 (raw file):

                )
            },
        );

I find this to be a bit simpler to read.

Suggestion:

    let message_to_l2_0 = scraper.base_layer.contract.sendMessageToL2(
        l2_contract_address.parse().unwrap(),
        l2_entry_point.parse().unwrap(),
        vec![U256::from(1_u8), U256::from(2_u8)],
    );
    let message_to_l2_1 = scraper.base_layer.contract.sendMessageToL2(
        l2_contract_address.parse().unwrap(),
        l2_entry_point.parse().unwrap(),
        vec![U256::from(3_u8), U256::from(4_u8)],
    );

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 96 at r10 (raw file):

    // Send the transactions.
    for msg in &[message_to_l2_0, message_to_l2_1] {
        msg.send().await.unwrap().get_receipt().await.unwrap();

So this line implies that the message build in sendMessageToL2 holds some sort of client to alloy? Why do we need to send again after that call?

Code quote:

        msg.send().await.unwrap().get_receipt().await.unwrap();

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 128 at r10 (raw file):

        ],
        ..tx.tx
    };

Suggestion, mostly to remove many macro invocations.

I didn't see anything to remove contract_address!. Unfortunately generic TryFrom is not allowed: https://stackoverflow.com/questions/73390086/unexpected-conflicting-implementation-for-tryfrom

Suggestion:

    let account_address = StarkHash::from_hex_unchecked(DEFAULT_ANVIL_ACCOUNT_ADDRESS);  // CONST?
    let expected_version = TransactionVersion(StarkHash::ZERO);  // CONST?

    let expected_internal_l1_tx = L1HandlerTransaction {
        version: expected_version,
        nonce: Nonce(Felt::from(0)),
        contract_address: contract_address!(l2_contract_address),
        entry_point_selector: EntryPointSelector(Felt::from_hex_unchecked(l2_entry_point)),
        calldata: Calldata(vec![account_address, Felt::from(1), Felt::from(2)].into()),
    };
    let tx = ExecutableL1HandlerTransaction {
        tx_hash: expected_internal_l1_tx
            .calculate_transaction_hash(&scraper.config.chain_id, &expected_version)
            .unwrap(),
        tx: expected_internal_l1_tx,
        paid_fee_on_l1: Fee(0),
    };
    let first_expected_log = Event::L1HandlerTransaction(tx.clone());

    let expected_internal_l1_tx_2 = L1HandlerTransaction {
        nonce: Nonce(Felt::from(1)),
        calldata: Calldata(vec![account_address, Felt::from(3), Felt::from(4)].into()),
        ..tx.tx
    };

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 38 at r8 (raw file):

fn anvil() -> AnvilInstance {
    Anvil::new().spawn()
}

Suggestion:

// Spin up Anvil instance, a local Ethereum node, dies when dropped.
#[fixture]
fn anvil() -> AnvilInstance {
    Anvil::new().spawn()
}

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 27 at r9 (raw file):

fn in_ci() -> bool {
    std::env::var("CI").is_ok()
}

This env variable already exists? Why was it added before?

Code quote:

// TODO: move to global test_utils crate and use everywhere instead of relying on the
// confusing `#[ignore]` api to mark slow tests.
fn in_ci() -> bool {
    std::env::var("CI").is_ok()
}

---

.github/workflows/main.yml line 45 at r9 (raw file):

            l1:
              - 'crates/starknet_l1_provider/**'
              - 'crates/papyrus_base_layer/**'

How does this work? It checks if we have the l1 scraper as a dependency of the thing we are building? Does it check if we already downloaded/built it?

Code quote:

  changes:
    runs-on: starkware-ubuntu-20-04-medium
    permissions:
      pull-requests: read
    outputs:
      l1: ${{ steps.filter.outputs.l1 }}
    steps:
      - uses: actions/checkout@v4
      - uses: dorny/paths-filter@v3
        id: filter
        with:
          filters: |
            l1:
              - 'crates/starknet_l1_provider/**'
              - 'crates/papyrus_base_layer/**'

[giladchase]

Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @elintul and @matan-starkware)

---

.github/workflows/main.yml line 45 at r9 (raw file):

How does this work? It checks if we have the l1 scraper as a dependency of the thing we are building

Yes but not quite dependency check, this is coarser.
In the PR which runs this workflow, t checks if the diff includes changes in base_layer or provider, and if it does, it installs anvil.
I could have made it finer and only check if the provider crate changed, which is what's necessary ATM (because scraper tests are triggered iff there are changes in the provider crate), but I'm planning on reusing this for base_layer tests as well, which need to migrate from Ganache to Anvil soon.

Does it check if we already downloaded/built it?

Unfortunately, that is impossible in our current setting (I checked). It's a limitation of moonrepo, specifically caching crates that aren't on crates.io.
I'll bring it up in the mono-infra sync next week.
For now though, this adds 3 minutes to this job for provider/baselayer CIs (not sure if this is critical path, depends on the other ~20 jobs we have running in parallel).

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 27 at r9 (raw file):
Ya we flip it up in the CI.
This is the best way I could find to conditionally run tests on the CI only.
The other alternative to add#[ignore] and run on the CI with --include-ignored, but this constantly confuses ppl, for good reason...

Why was it added before?

Not sure I understand the question 🤔 I'm only checking if its up here, I'm not setting it

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 32 at r10 (raw file):

Previously, matan-starkware wrote…

Felt::from_hex_unchecked is a const fn, so you can build the felt here if you want.

YAAAS.
I kept the other second one though, it needs to parse into an external type which doesn't have const fn (alloy's Address type)

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 42 at r10 (raw file):

Previously, matan-starkware wrote…

Why not make this the fixture instead? Is the fixture reused across tests (I presume not)

Went back and forth on this one.

It isn't reused, but the fixutre will need to also return the Anvil instance to the test, which is a guard, since if it drops then the server is dropped.

It can be done, but this might not be obvious to users why we are returning it.
By making Anvil a fixture, the user naturally has Anvil in scope and doesn't have to worry about it.

There might be a way of making them both fixtures and using some pytest-like indirect mechanism, but I haven't found a way of doing this elegantly without pissing off the borrow checker (the Anvil instance must be a singleton and is unclonable).

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 96 at r10 (raw file):

So this line implies that the message build in sendMessageToL2 holds some sort of client to alloy

Yes but its even more general: scraper.base_layer.contract has the client and is mapped to the Starknet L1 contract it was initialized with (which is identical to the one on L1). Messages to the contract have this client as well.

Why do we need to send again after that call?

• sendMessageToL2 - creates a CallBuilder, whose "build" command is send/call/call_raw etc.
• send() - sends the call-builder's msg to "L1", which creates a TransactionBuilder for decoding the response.
• get_receipt - wait until message is received, confirmed, and decodes the response through the builder in the last step. We don't really need the receipt, just the "wait until confirmed" behavior. If, for example, we'd use call()/call_raw() instead of send, we won't have this get_receipt thing, but we'd be forced to busy-wait on fetch_events later until it appears on L1.

---

crates/starknet_l1_provider/src/l1_scraper_tests.rs line 128 at r10 (raw file):

Previously, matan-starkware wrote…

Suggestion, mostly to remove many macro invocations.

I didn't see anything to remove contract_address!. Unfortunately generic TryFrom is not allowed: https://stackoverflow.com/questions/73390086/unexpected-conflicting-implementation-for-tryfrom

Done.
Added some consts for Felt::ONE and Felt::ZERO instead of from, and used the const address from before instead of here.
I also used StarkHash instead of Felt, to spare having an extra dep (types-rs) just for this (we already need SNAPI), it's the same underthehood anyway

[matan-starkware]

Reviewed 1 of 2 files at r6, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

[giladchase]

Reviewed 2 of 4 files at r2, 1 of 2 files at r4, 1 of 2 files at r6, 1 of 1 files at r9, 1 of 1 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)