chore(starknet_l1_provider): implement event fetch

[giladchase]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-20T21:04:08Z
• Started at 2024-12-24T13:52:10Z
• Started at 2025-01-06T10:56:04Z
• Started at 2025-01-06T13:53:52Z

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.511 ms 34.544 ms 34.584 ms]
change: [+1.0304% +1.4044% +1.7001%] (p = 0.00 < 0.05)
Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe

[elintul]

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

---

crates/starknet_l1_provider/src/l1_scraper.rs line 57 at r1 (raw file):

            error!("Failed to get latest L1 block number, finality too high.");
            return Ok(());
        };

In case this code is repeated, consider a function.

Code quote:

        let latest_l1_block_number = self
            .base_layer
            .latest_l1_block_number(self.config.finality)
            .await
            .map_err(L1ScraperError::BaseLayer)?;

        let Some(latest_l1_block_number) = latest_l1_block_number else {
            error!("Failed to get latest L1 block number, finality too high.");
            return Ok(());
        };

---

crates/starknet_l1_provider/src/l1_scraper.rs line 72 at r1 (raw file):

            .into_iter()
            .map(|event| self.event_from_raw_l1_event(event))
            .collect::<L1ScraperResult<Vec<_>, _>>()?;

Do you prefer turbofish over type annotations?
Also, WDYM by collecting to a Result?

Code quote:

::<L1ScraperResult<Vec<_>, _>>

---

crates/starknet_l1_provider/src/l1_scraper.rs line 86 at r1 (raw file):

    }

    fn event_from_raw_l1_event(&self, l1_event: L1Event) -> L1ScraperResult<Event, B> {

Can this be Event::TryFrom<L1Event> method?

Code quote:

event_from_raw_l1_event

---

crates/starknet_l1_provider/src/l1_scraper.rs line 92 at r1 (raw file):

                ExecutableL1HandlerTransaction::create(tx, chain_id, fee)
                    .map(Event::L1HandlerTransaction)
                    .map_err(L1ScraperError::HashCalculationError)

Not very readable, IMO. Any alternative?

Code quote:

                ExecutableL1HandlerTransaction::create(tx, chain_id, fee)
                    .map(Event::L1HandlerTransaction)
                    .map_err(L1ScraperError::HashCalculationError)

---

crates/starknet_l1_provider/src/l1_scraper.rs line 124 at r1 (raw file):

#[derive(Error, Debug)]
pub enum L1ScraperError<T: BaseLayerContract + Send + Sync> {

Is this necessary? What exactly is generic here - the L1, or the contract we're interacting with?

Code quote:

T: BaseLayerContract

[matan-starkware]

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

---

crates/starknet_l1_provider/src/l1_scraper.rs line 72 at r1 (raw file):

Previously, elintul (Elin) wrote…

Do you prefer turbofish over type annotations?
Also, WDYM by collecting to a Result?

"Also, WDYM by collecting to a Result?" This tells Rust to iterate over Result<T> elements and upon encountering a single error exit, returning that error. If no errors are encountered, it produces Vec<T>.

---

crates/starknet_l1_provider/src/l1_scraper.rs line 92 at r1 (raw file):

Previously, elintul (Elin) wrote…

Not very readable, IMO. Any alternative?

Agreed. If you update the error type to HashCalculationError(#[from] StarknetApiError), Then you could do

                let tx = ExecutableL1HandlerTransaction::create(tx, &self.config.chain_id, fee)?;
                Ok(Event::L1HandlerTransaction(tx))

---

crates/starknet_l1_provider/src/l1_scraper.rs line 55 at r1 (raw file):

        let Some(latest_l1_block_number) = latest_l1_block_number else {
            error!("Failed to get latest L1 block number, finality too high.");

This means that there is no finalized block on ethereum? (dependent upon our config)

Code quote:

            error!("Failed to get latest L1 block number, finality too high.");

---

crates/starknet_l1_provider/src/l1_scraper.rs line 74 at r1 (raw file):

            .collect::<L1ScraperResult<Vec<_>, _>>()?;

        self.last_block_number_processed = latest_l1_block_number + 1;

Is BaseLayerContract::until_block inclusive on until_block? I think that should be explicit in the trait doc.

Code quote:

        self.last_block_number_processed = latest_l1_block_number + 1;

---

crates/starknet_l1_provider/src/l1_scraper.rs line 128 at r1 (raw file):

    BaseLayer(T::Error),
    #[error("Failed to calculate hash: {0}")]
    HashCalculationError(StarknetApiError),

Allow for easy conversions

Suggestion:

    HashCalculationError(#[from] StarknetApiError),

[elintul]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @giladchase and @matan-starkware)

---

crates/starknet_l1_provider/src/l1_scraper.rs line 72 at r1 (raw file):

Previously, matan-starkware wrote…

"Also, WDYM by collecting to a Result?" This tells Rust to iterate over Result<T> elements and upon encountering a single error exit, returning that error. If no errors are encountered, it produces Vec<T>.

Thanks!

[giladchase]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @elintul and @matan-starkware)

---

crates/starknet_l1_provider/src/l1_scraper.rs line 55 at r1 (raw file):

Previously, matan-starkware wrote…

This means that there is no finalized block on ethereum? (dependent upon our config)

Yep. I didn't want to throw an error, because there are cases where this can happen (like in genesis).

---

crates/starknet_l1_provider/src/l1_scraper.rs line 57 at r1 (raw file):

Previously, elintul (Elin) wrote…

In case this code is repeated, consider a function.

I don't think i'll need this again in the scraper, unless I'm misunderstanding what you mean here

---

crates/starknet_l1_provider/src/l1_scraper.rs line 74 at r1 (raw file):

Previously, matan-starkware wrote…

Is BaseLayerContract::until_block inclusive on until_block? I think that should be explicit in the trait doc.

Good point.
Documenting it with types if that's OK 💪 will now take RangeInclusive as an arg

---

crates/starknet_l1_provider/src/l1_scraper.rs line 86 at r1 (raw file):

Previously, elintul (Elin) wrote…

Can this be Event::TryFrom<L1Event> method?

Nope it needs chain_id for l1Handler variant :(
I though about maybe doing a try_from then add the chain afterwards, but this won't work cause I need the chain_id to calculate the tx_hash 🥲

---

crates/starknet_l1_provider/src/l1_scraper.rs line 92 at r1 (raw file):

Any alternative?

How about this?

            L1Event::LogMessageToL2 { tx, fee } => {
                let chain_id = &self.config.chain_id;
                match ExecutableL1HandlerTransaction::create(tx, chain_id, fee) {
                    Ok(tx) => Ok(Event::L1HandlerTransaction(tx)),
                    Err(hash_calc_err) => Err(L1ScraperError::HashCalculationError(hash_calc_err)),
                }
            }

If you update the error type to HashCalculationError(#[from] StarknetApiError)

I wanted to avoid doing that since this hash calc is the only SNAPI error possible here, whereas #[from] catches all variants.

We've already hit this issue in the past, where future devs of such code used ? on StarknetApi because "it just worked", and later on people suddenly got errors like: HashCalcError("<some error that isn't related to hash calc>").

In other words, if we add #[from], we should change the error to StarknetApiError and lose the specificity of HashCalcError variant, which in turn makes future readers of this code unable to distinguish which error variants are actually possible here (which currently is just the one).

Personally, I tend now only to use #[from] if at least min(3,n_variants_of_error) variants are possible, but of course, if you still prefer using #[from], I'll change it.

---

crates/starknet_l1_provider/src/l1_scraper.rs line 124 at r1 (raw file):

Previously, elintul (Elin) wrote…

Is this necessary? What exactly is generic here - the L1, or the contract we're interacting with?

Kind of both. It is assumed to be some wrapper around a crate that interacts with L1, that is initialized with the hardcoded Starknet contract on L1.

This was useful for testing, I could mock it easily.

---

crates/starknet_l1_provider/src/l1_scraper.rs line 128 at r1 (raw file):

Previously, matan-starkware wrote…

Allow for easy conversions

answered in this thread

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.218 ms 34.269 ms 34.323 ms]
change: [-4.7764% -3.1535% -1.7231%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
6 (6.00%) high mild

[matan-starkware]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/starknet_l1_provider/src/l1_scraper.rs line 74 at r1 (raw file):

Previously, giladchase wrote…

Good point.
Documenting it with types if that's OK 💪 will now take RangeInclusive as an arg

Yeah the new interface is nice, and makes the code inside shorter also

---

crates/starknet_l1_provider/src/l1_scraper.rs line 92 at r1 (raw file):

Previously, giladchase wrote…

Any alternative?

How about this?

            L1Event::LogMessageToL2 { tx, fee } => {
                let chain_id = &self.config.chain_id;
                match ExecutableL1HandlerTransaction::create(tx, chain_id, fee) {
                    Ok(tx) => Ok(Event::L1HandlerTransaction(tx)),
                    Err(hash_calc_err) => Err(L1ScraperError::HashCalculationError(hash_calc_err)),
                }
            }

If you update the error type to HashCalculationError(#[from] StarknetApiError)

I wanted to avoid doing that since this hash calc is the only SNAPI error possible here, whereas #[from] catches all variants.

We've already hit this issue in the past, where future devs of such code used ? on StarknetApi because "it just worked", and later on people suddenly got errors like: HashCalcError("<some error that isn't related to hash calc>").

In other words, if we add #[from], we should change the error to StarknetApiError and lose the specificity of HashCalcError variant, which in turn makes future readers of this code unable to distinguish which error variants are actually possible here (which currently is just the one).

Personally, I tend now only to use #[from] if at least min(3,n_variants_of_error) variants are possible, but of course, if you still prefer using #[from], I'll change it.

SGTM

[giladchase]

Dismissed @elintul from a discussion.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @elintul and @matan-starkware)

---

crates/starknet_l1_provider/src/l1_scraper.rs line 92 at r1 (raw file):

Previously, matan-starkware wrote…

SGTM

Done.
Elin is OOO so i'm gonna dismiss the block and merge ahead unless there are objections 😬

OOO

[giladchase]

Reviewed 2 of 5 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)