papyrus_base_layer: remove finality from base layer

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• papyrus_base_layer: delete l1_endpoint_monitor crate #10736
• papyrus_base_layer: delete monitored base layer #10731
• apollo_l1_provider: re-write FinalityTooHigh error as LatestBlockNumberTooLow #10729
• apollo_l1_provider: use cyclic base layer wrapper instead of monitored base layer #10703
• apollo_l1_provider: make sure wrong event types do not get scraped #10700
• papyrus_base_layer: gate integration tests #10671
• apollo_l1_provider: add test for each type of L1 event we filter on #10595
• papyrus_base_layer: test util for creating an ethereum base layer using a mock provider #10588
• papyrus_base_layer: extract check and cycle URLs into a helper function with logging #10618
• papyrus_base_layer: add CyclicBaseLayerWrapper #10412
• papyrus_base_layer: make BaseLayer trait functions mutable #10404
• papyrus_base_layer: add the option to cycle the URL directly into the base layer #10357
• papyrus_base_layer: add ordered list of provider URLs to the config #10354
• apollo_l1_provider: remove finality_too_high helper function, add MissingBlock error #10348
• apollo_l1_gas_price: add test for low latest L1 block number #10346
• apollo_l1_provider: add test for low latest L1 block number #10345
• apollo_l1_provider: replace mutex with atomic u64 in a scraper test #10344
• papyrus_base_layer: remove finality from base layer #10303 👈 (View in Graphite)
• papyrus_base_layer: remove latest_l1_block from base layer #10300
• apollo_central_sync: remove base layer call to latest_proved_block #10246
• papyrus_base_layer: add test for a base layer without a starknet contract #10242
• main-v0.14.1

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

[ShahakShama]

@ShahakShama reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guy-starkware)

---

crates/apollo_central_sync/src/sources/base_layer.rs line 52 at r1 (raw file):

            return Err(BaseLayerSourceError::FinalityTooHigh { finality, latest_l1_block_number });
        }
        let latest_l1_block_number = latest_l1_block_number.saturating_sub(finality);

Use checked_sub and ok_or instead

---

crates/apollo_l1_provider/src/l1_scraper.rs line 82 at r1 (raw file):

            .await
            .map_err(L1ScraperError::BaseLayerError)?;
        if latest_l1_block_number < finality {

Consider extracting to a utility function. You do this code a lot

---

crates/apollo_l1_provider/src/l1_scraper.rs line 88 at r1 (raw file):

            });
        }
        let latest_l1_block_number = latest_l1_block_number.saturating_sub(finality);

Same here

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 144 at r1 (raw file):

        )
        .await??;
        // let Some(block_number) = block_number.checked_sub(finality) else {

Remove commented code

---

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

    L1_GAS_PRICE_SCRAPER_SUCCESS_COUNT,
};

Now that I think of it, why do we care about finality in the gas price scraper? Worst case is that we have a few blocks in the average that don't really exist, and they will be erased in a few hours. Doesn't sound like a problem to me
Maybe we should consider removing finality to make the code flow easier? Anyway, later PR

---

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

            });
        }
        let latest_l1_block_number = latest_l1_block_number.saturating_sub(self.config.finality);

Same here

[guy-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_central_sync/src/sources/base_layer.rs line 52 at r1 (raw file):

Previously, ShahakShama wrote…

Use checked_sub and ok_or instead

Done.

---

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

Previously, ShahakShama wrote…

Now that I think of it, why do we care about finality in the gas price scraper? Worst case is that we have a few blocks in the average that don't really exist, and they will be erased in a few hours. Doesn't sound like a problem to me
Maybe we should consider removing finality to make the code flow easier? Anyway, later PR

Maybe you are right. But there's an issue with what happens when the gas price provider gets the same L1 block twice (after the reorg). Currently we panic on getting unexpected block number (either skipping ahead or going back).

We could fix that, in a general sense that "we don't really care if we lose a block or add a block" but that deserves some more consideration.

---

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

Previously, ShahakShama wrote…

Same here

Done.

---

crates/apollo_l1_provider/src/l1_scraper.rs line 82 at r1 (raw file):

Previously, ShahakShama wrote…

Consider extracting to a utility function. You do this code a lot

I think "checked_sub" is the utility function (given that the error returned is a different type in each place).

Suppose I did want to extract a helper function, where in the code would I put it (that is accessible to all three crates)?

---

crates/apollo_l1_provider/src/l1_scraper.rs line 88 at r1 (raw file):

Previously, ShahakShama wrote…

Same here

Done.

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 144 at r1 (raw file):

Previously, ShahakShama wrote…

Remove commented code

Done.

[ShahakShama]

@ShahakShama reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

---

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

Previously, guy-starkware wrote…

Maybe you are right. But there's an issue with what happens when the gas price provider gets the same L1 block twice (after the reorg). Currently we panic on getting unexpected block number (either skipping ahead or going back).

We could fix that, in a general sense that "we don't really care if we lose a block or add a block" but that deserves some more consideration.

Food for thought

---

crates/apollo_l1_provider/src/l1_scraper.rs line 82 at r1 (raw file):

Previously, guy-starkware wrote…

I think "checked_sub" is the utility function (given that the error returned is a different type in each place).

Suppose I did want to extract a helper function, where in the code would I put it (that is accessible to all three crates)?

With checked_sub it becomes less code so nvm

---

crates/apollo_l1_provider/src/l1_scraper.rs line 88 at r1 (raw file):

Previously, guy-starkware wrote…

Done.

There were 2 places in this file that needed this fix. You only fixed one of them

[guy-starkware]

Reviewable status: 12 of 14 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper.rs line 88 at r1 (raw file):

Previously, ShahakShama wrote…

There were 2 places in this file that needed this fix. You only fixed one of them

Done.

[ShahakShama]

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

[graphite-app]

Merge activity

• Nov 23, 12:58 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.