apollo_l1_gas_price: start scraper with a loop to find start block, instead of panic if it fails

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• apollo_l1_gas_price: start scraper with a loop to find start block, instead of panic if it fails #11460 👈 (View in Graphite)
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ShahakShama]

@ShahakShama reviewed 1 file and all commit messages, and made 4 comments.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @guy-starkware).

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 66 at r1 (raw file):

    }

    pub async fn get_first_block_then_run(&mut self) -> L1GasPriceScraperResult<(), B> {

No need to put run logic in this function. It complicates the name of the function, which in turn reduces the readability at the callsite

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 74 at r1 (raw file):

                    let Ok(latest) = latest else {
                        warn!("Failed to get the latest L1 block number at startup: {latest:?}");
                        tokio::time::sleep(self.config.polling_interval).await;

Shouldn't we increase relevant failure metrics?

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 329 at r1 (raw file):

#[tokio::test]
async fn get_first_block_then_run_fails_then_succeeds() {

The test says "fails then succeeds" but I only see the fails section

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 338 at r1 (raw file):

    // The provider will return an error, indicating the scraper has successfully started running.
    // The error will take us out of the endless loop of scraper.run().

I don't understand why do we need the provider to return an error for this test

[guy-starkware]

@guy-starkware made 4 comments.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ShahakShama).

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 66 at r1 (raw file):

Previously, ShahakShama wrote…

No need to put run logic in this function. It complicates the name of the function, which in turn reduces the readability at the callsite

It also makes testing much more complicated. I'm changing it.

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 74 at r1 (raw file):

Previously, ShahakShama wrote…

Shouldn't we increase relevant failure metrics?

Yes!

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 329 at r1 (raw file):

Previously, ShahakShama wrote…

The test says "fails then succeeds" but I only see the fails section

The new, simplified test is much clearer.

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 338 at r1 (raw file):

Previously, ShahakShama wrote…

I don't understand why do we need the provider to return an error for this test

If it returns Ok() we will continue to be in the endless loop if "run". But I will take your advice and separate run from that new function, which will make it all much cleaner.

[ShahakShama]

@ShahakShama reviewed 2 files and all commit messages, made 5 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware).

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 342 at r2 (raw file):

        .returning(move || Err(MockError::MockError));

    // The succeed.

Then

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 66 at r2 (raw file):

    }

    pub async fn get_first_block_number(&mut self) -> L1GasPriceScraperResult<L1BlockNumber, B> {

The return type can be just L1BlockNumber, right?

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 70 at r2 (raw file):

            Some(block) => Ok(block),
            None => {
                loop {

De-nest this. At the beginning of the function write

if let Some(block) = self.config.starting_block {
    return Ok(block)
}
loop {...

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 73 at r2 (raw file):

                    let latest = self.latest_l1_block_number().await;
                    let Ok(latest) = latest else {
                        warn!("Failed to get the latest L1 block number at startup: {latest:?}");

Consider rephrasing this sentence so that startup appears close to the start of it

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 86 at r2 (raw file):

                            * self.config.startup_num_blocks_multiplier,
                    );
                    break Ok(latest);

Now you can change it from break to return!

[guy-starkware]

@guy-starkware made 5 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama).

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 66 at r2 (raw file):

Previously, ShahakShama wrote…

The return type can be just L1BlockNumber, right?

Yes!

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 70 at r2 (raw file):

Previously, ShahakShama wrote…

De-nest this. At the beginning of the function write

if let Some(block) = self.config.starting_block {
    return Ok(block)
}
loop {...

Good idea.

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 73 at r2 (raw file):

Previously, ShahakShama wrote…

Consider rephrasing this sentence so that startup appears close to the start of it

Done.

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper.rs line 86 at r2 (raw file):

Previously, ShahakShama wrote…

Now you can change it from break to return!

Yep.

---

crates/apollo_l1_gas_price/src/l1_gas_price_scraper_test.rs line 342 at r2 (raw file):

Previously, ShahakShama wrote…

Then

Done.

[ShahakShama]

@ShahakShama reviewed 2 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware).

[guy-starkware]

@guy-starkware partially reviewed 4 files.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @guy-starkware).

[guy-starkware]

@guy-starkware reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware).