feat(katana): add l1 data price info in header #2547
Conversation
kariy
changed the title
WalkthroughOhayo! This pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
crates/katana/primitives/src/env.rs (1)
14-15: Ohayo! LGTM, sensei! The addition of L1 data gas prices is spot on.The new
l1_data_gas_pricesfield in theBlockEnvstruct is a great addition that aligns perfectly with the PR objectives. It's consistent with the existing structure and will help address the discrepancies in gas price data.Consider adding a brief doc comment above the new field to explain its purpose, similar to the other fields in the struct. For example:
/// The L1 data gas prices at this particular block. pub l1_data_gas_prices: GasPrices,This will help maintain consistency in documentation and provide clarity for future developers, sensei.
crates/katana/executor/src/implementation/blockifier/mod.rs (2)
105-107: Ohayo, gas price handling looks good, sensei!The change to use
header.l1_gas_pricesaligns well with the PR objective of correcting the gas price data. Good job on usingNonZeroU128with a fallback to ensure the gas price is never zero.Consider extracting the fallback value
1to a named constant for better readability and maintainability. For example:const FALLBACK_GAS_PRICE: u128 = 1; // ... NonZeroU128::new(header.l1_gas_prices.eth).unwrap_or(NonZeroU128::new(FALLBACK_GAS_PRICE).unwrap());
242-245: Ohayo, sensei! Nice addition of L1 data gas prices!The new
l1_data_gas_pricesfield in theBlockEnvstruct provides a more comprehensive view of the gas prices, aligning well with the PR objective. This enhancement will help address the discrepancies in gas price data.For consistency with the
l1_gas_pricesfield above, consider using a similar structure:l1_data_gas_prices: KatanaGasPrices { eth: self.block_context.block_info().gas_prices.eth_l1_data_gas_price.get(), strk: self.block_context.block_info().gas_prices.strk_l1_data_gas_price.get(), },This approach ensures that we're using the
get()method onNonZeroU128, maintaining consistency with howl1_gas_pricesis handled.crates/katana/core/src/backend/storage.rs (1)
161-161: Address the TODO: Remove gas price from genesisThere's a TODO comment indicating that the gas price should be removed from the genesis block. Removing it could simplify the genesis configuration and prevent redundant data.
Would you like me to help implement this change or open a new GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (17)
- crates/katana/core/src/backend/mod.rs (2 hunks)
- crates/katana/core/src/backend/storage.rs (3 hunks)
- crates/katana/core/src/service/block_producer.rs (3 hunks)
- crates/katana/executor/src/implementation/blockifier/mod.rs (2 hunks)
- crates/katana/executor/tests/executor.rs (3 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (4 hunks)
- crates/katana/primitives/src/block.rs (4 hunks)
- crates/katana/primitives/src/chain_spec.rs (5 hunks)
- crates/katana/primitives/src/da/mod.rs (1 hunks)
- crates/katana/primitives/src/env.rs (1 hunks)
- crates/katana/rpc/rpc-types/src/block.rs (11 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (4 hunks)
- crates/katana/storage/provider/src/providers/db/mod.rs (1 hunks)
- crates/katana/storage/provider/src/providers/fork/mod.rs (1 hunks)
- crates/katana/storage/provider/src/providers/in_memory/mod.rs (1 hunks)
- crates/katana/storage/provider/tests/block.rs (2 hunks)
- crates/saya/provider/src/rpc/mod.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (54)
crates/katana/primitives/src/da/mod.rs (1)
7-7: Ohayo, sensei! This change looks great!The addition of
pub use ::starknet::core::types::L1DataAvailabilityMode;is a solid move. It makes theL1DataAvailabilityModetype readily available from this module, which aligns perfectly with our goal of addressing L1 data availability issues mentioned in the PR objectives.This change will make it easier for other parts of the codebase to work with L1 data availability modes, potentially simplifying the implementation of the required changes for correct StarkNet version and gas price data.
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
Line range hint
1-300: Ohayo, sensei! Overall changes look solid!The modifications in this file are well-aligned with the PR objectives of resolving discrepancies in StarkNet version and gas price data. The changes to gas price handling and the addition of L1 data gas prices in the block environment contribute to a more accurate and comprehensive representation of the blockchain state.
To further improve the code:
- Consider adding comments explaining the significance of the new
l1_data_gas_pricesfield and how it differs froml1_gas_prices.- Update any relevant documentation or comments that might reference the old gas price structure.
- Ensure that these changes are consistently applied across other related files in the codebase.
crates/katana/executor/tests/executor.rs (4)
51-51: Ohayo! LGTM, sensei! This change aligns perfectly with our mission.The update from
gas_pricestol1_gas_pricesis spot on. It ensures we're accurately asserting the L1 data gas prices in our block environment, which is crucial for addressing those pesky discrepancies mentioned in issue #2539. Nice work!
130-130: Ohayo again, sensei! Consistency is key, and you've nailed it!Excellent job maintaining the
l1_gas_pricesupdate here for the second block assertion. This consistent change across the test implementation ensures we're thoroughly validating the L1 data gas prices throughout the execution process. Keep up the great work!
177-177: Ohayo once more, sensei! You've achieved the trifecta of awesomeness!Fantastic job completing the
l1_gas_pricesupdate trifecta with this change for the third block assertion. Your thorough implementation ensures we're validating the L1 data gas prices consistently across all three blocks. This level of detail is exactly what we need to squash those gas price discrepancies. You're a true coding ninja!
Line range hint
51-177: Ohayo, sensei! Let's wrap this up with a bow of wisdom!Your changes to the
test_executor_with_valid_blocks_implfunction are spot on! By consistently updatinggas_pricestol1_gas_pricesacross all three block assertions, you've ensured that our test suite accurately validates the L1 data gas prices. This aligns perfectly with our mission to resolve the discrepancies mentioned in issue #2539.These updates make our test implementation more robust and reflective of the actual block structure. Great job on improving the accuracy and reliability of our testing process!
Is there anything else you'd like me to look at, sensei?
crates/katana/storage/provider/tests/block.rs (2)
129-130: Ohayo! LGTM, sensei! These changes align perfectly with our L1 data mission.The addition of
l1_gas_pricesandl1_data_gas_pricesto theBlockEnvstructure ensures that our tests now cover the new L1 gas price fields. This modification is in line with the PR objectives to include L1 data info in the block header. Great job on keeping our tests up-to-date!
235-236: Ohayo again, sensei! Excellent work on maintaining consistency!The addition of
l1_gas_pricesandl1_data_gas_pricesto theBlockEnvstructure in theinsert_block_empty_test_implfunction mirrors the changes made in theinsert_block_test_implfunction. This consistency ensures that both empty and non-empty block tests cover the new L1 gas price fields. Your attention to detail in keeping our test implementations aligned is commendable!crates/katana/storage/provider/src/providers/in_memory/mod.rs (1)
573-574: Ohayo, sensei! LGTM: Enhanced block environment dataThe addition of
l1_data_gas_pricesalongside the existingl1_gas_pricesin theBlockEnvstruct is a great improvement. This change aligns perfectly with the PR objectives to address discrepancies in gas price data when fetching block information.crates/katana/storage/provider/src/providers/fork/mod.rs (2)
579-580: Ohayo! New L1 gas price fields added to BlockEnv, sensei!The addition of
l1_gas_pricesandl1_data_gas_pricesto theBlockEnvstruct aligns well with the PR objectives. This change should help resolve the discrepancies in gas price data when fetching block information from Katana, as mentioned in issue #2539.
579-580: Verify BlockEnv usage in the codebase, sensei!The changes to
BlockEnvlook good, but we should ensure that any code using this struct is updated to handle the newl1_gas_pricesandl1_data_gas_pricesfields.Let's run a quick check to find any potential usage of
BlockEnvthat might need updating:✅ Verification successful
ohayo sensei!
I've thoroughly checked the usage of
BlockEnvacross the codebase, including the newl1_gas_pricesandl1_data_gas_pricesfields. Everything looks good and properly handled.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usage of BlockEnv that might need updating # Search for BlockEnv usage echo "Searching for BlockEnv usage:" rg --type rust "BlockEnv" -C 3 # Search for l1_gas_prices usage echo "\nSearching for l1_gas_prices usage:" rg --type rust "l1_gas_prices" -C 3 # Search for l1_data_gas_prices usage echo "\nSearching for l1_data_gas_prices usage:" rg --type rust "l1_data_gas_prices" -C 3Length of output: 88041
crates/katana/core/src/backend/mod.rs (4)
8-8: Ohayo sensei! Import ofL1DataAvailabilityModeis appropriateThe addition of
use katana_primitives::da::L1DataAvailabilityMode;correctly introduces theL1DataAvailabilityModeenum needed for specifying the data availability mode in the block header.
72-72: Ohayo sensei! Addingl1_da_modetoPartialHeaderThe
l1_da_modefield has been added to thePartialHeaderand initialized withL1DataAvailabilityMode::Calldata. This ensures that the block header accurately reflects the L1 data availability mode, addressing the discrepancies noted in issue #2539.
73-76: Ohayo sensei! Updatinggas_pricestol1_gas_pricesThe
gas_pricesfield has been updated tol1_gas_pricesin thePartialHeader, correctly associating gas prices with Layer 1. This change aligns with the PR objectives to return accurate L1 gas price data when querying block information.
77-80: Ohayo sensei! Includingl1_data_gas_pricesinPartialHeaderThe new field
l1_data_gas_pricesis added to thePartialHeaderand initialized with values fromblock_env.l1_data_gas_prices. This addition ensures that L1 data gas prices are accurately included in the block header, resolving the inconsistencies highlighted in issue #2539.crates/saya/provider/src/rpc/mod.rs (1)
104-104: Ohayo sensei! The addition ofl1_da_modeis correctly implementedThe new field
l1_da_modeis appropriately assigned fromblock.l1_da_mode, aligning with the PR objectives.crates/katana/primitives/src/block.rs (4)
50-52: Ohayo, sensei! The additions toPartialHeaderare well-integratedThe new fields
l1_gas_prices,l1_data_gas_prices, andl1_da_modehave been correctly added to thePartialHeaderstruct. This enhancement aligns with the objective of incorporating L1-specific gas pricing and data availability modes.
84-87: Ohayo, sensei! The additions toHeaderare appropriateThe fields
l1_gas_prices,l1_data_gas_prices, andl1_da_modehave been properly added to theHeaderstruct, ensuring consistency withPartialHeaderand enhancing the block header's functionality.
89-102: Verify the default values inHeader::defaultOhayo, sensei! Please ensure that the default values set in the
Defaultimplementation ofHeaderare suitable for your application's context, especially:
l1_gas_prices: GasPrices::default()l1_data_gas_prices: GasPrices::default()l1_da_mode: L1DataAvailabilityMode::Calldata
114-116: Ohayo, sensei! The updates inHeader::neware consistentThe new fields are correctly initialized from
PartialHeaderin theHeader::newmethod. This ensures that all necessary data is accurately transferred and maintained.crates/katana/executor/tests/fixtures/mod.rs (1)
13-13: Ohayo sensei! The import ofL1DataAvailabilityModeis correctly added.crates/katana/core/src/backend/storage.rs (4)
152-152: Parsing StarkNet version correctlyThe code accurately parses the
starknet_versionfrom theforked_blockand assigns it to the chain's protocol version.
155-159: Verify the impact of updating genesis block propertiesOhayo sensei! Updating the genesis block properties to match the forked block ensures consistency with the source chain. However, this could have implications on chain integrity and future behavior. Please verify that this change aligns with the intended design.
Would you like assistance in assessing the potential impact of these updates?
181-185: Ensure correct initialization of L1 data gas prices and DA modeThe code updates the block header with
l1_data_gas_pricesandl1_da_modefrom theforked_block, which is essential for accurate block processing.Please verify that these fields are correctly utilized in downstream processes.
218-218: ImportingL1DataAvailabilityModefor proper type usageOhayo sensei! The addition of the import for
L1DataAvailabilityModeensures that the code can reference the DA mode enum without issues.crates/katana/rpc/rpc/src/starknet/read.rs (6)
4-4: Ohayo, sensei! Added necessary import forL1DataAvailabilityModeThe import of
L1DataAvailabilityModefromkatana_primitives::dais appropriate for its usage in the code.
87-89: Ohayo, sensei! Correctly updatedPartialHeaderwith new L1 data fieldsAdding
l1_da_mode,l1_data_gas_prices, andl1_gas_pricestoPartialHeaderenhances the block header with necessary L1 data information as per the PR objectives.
174-175: Ohayo, sensei! Consider borrowing instead of cloningSame as lines 83-84.
178-180: Ohayo, sensei! Correctly updatedPartialHeaderwith new L1 data fieldsSame as lines 87-89.
234-235: Ohayo, sensei! Consider borrowing instead of cloningSame as lines 83-84.
238-240: Ohayo, sensei! Correctly updatedPartialHeaderwith new L1 data fieldsSame as lines 87-89.
crates/katana/primitives/src/chain_spec.rs (5)
5-5: Ohayo, sensei! The import ofL1DataAvailabilityModelooks good.
60-62: Ohayo, sensei! Adding the new fieldsl1_da_mode,l1_gas_prices, andl1_data_gas_pricesto the header is appropriate.
222-222: Ohayo, sensei! The import ofL1DataAvailabilityModein the test module is correct.
349-351: Ohayo, sensei! Updating the expected block header with the new fields ensures the tests remain accurate.
367-372: Ohayo, sensei! Asserting the new header fields in tests is a thorough approach.crates/katana/core/src/service/block_producer.rs (2)
14-14: Ohayo, sensei! Good inclusion ofL1DataAvailabilityMode.The import at line 14 is necessary for specifying the data availability mode in the block header.
585-588: Ohayo, sensei! Excellent update to the block header.Including
l1_da_mode,l1_gas_prices, andl1_data_gas_pricesin the block header ensures that the block accurately reflects the L1 data availability mode and gas prices.crates/katana/rpc/rpc-types/src/block.rs (16)
23-25: Ohayo sensei! Correctly initializedl1_gas_priceinBlockWithTxs.The
l1_gas_priceis accurately constructed usingblock.header.l1_gas_prices.ethandstrk, ensuring precise gas price handling.
27-29: Ohayo sensei! Properly set upl1_data_gas_priceinBlockWithTxs.The
l1_data_gas_priceis correctly initialized with values fromblock.header.l1_data_gas_prices.ethandstrk.
50-51: Ohayo sensei! Updatedl1_da_modeto reflect header data.Setting
l1_da_modetoblock.header.l1_da_modeensures the data availability mode aligns with the actual block header.
66-67: Ohayo sensei! Initializedl1_gas_pricecorrectly inPendingBlockWithTxs.The
l1_gas_priceis properly set usingheader.l1_gas_prices.ethandstrk, maintaining consistency with the header information.
105-107: Ohayo sensei! Correctl1_gas_priceinitialization inBlockWithTxHashes.The values are accurately derived from
block.header.l1_gas_prices, ensuring accurate gas pricing.
109-111: Ohayo sensei! Properly initializedl1_data_gas_priceinBlockWithTxHashes.Using
block.header.l1_data_gas_prices.ethandstrkensures correct data gas pricing.
129-130: Ohayo sensei! Updatedl1_da_modeand includedl1_data_gas_price.Setting
l1_da_modetoblock.header.l1_da_modeand properly initializingl1_data_gas_priceenhances consistency.
142-144: Ohayo sensei! Correctly setl1_gas_priceinPendingBlockWithTxHashes.The initialization from
header.l1_gas_pricesis accurate, ensuring consistency in pending blocks.
146-148: Ohayo sensei! Properl1_data_gas_priceinitialization inPendingBlockWithTxHashes.Values are correctly assigned from
header.l1_data_gas_prices, reflecting accurate pricing.
158-159: Ohayo sensei! Updatedl1_da_modein pending block with hashes.Using
header.l1_da_modeensures the data availability mode is up-to-date with header data.
198-200: Ohayo sensei! Correctl1_gas_pricesetup inBlockWithReceipts.The gas prices are accurately initialized from
header.l1_gas_prices, ensuring proper receipt handling.
202-204: Ohayo sensei! Initializedl1_data_gas_priceinBlockWithReceipts.Values from
header.l1_data_gas_pricesare correctly used, maintaining consistency.
227-227: Ohayo sensei! Includedl1_data_gas_pricein block receipts.Adding
l1_data_gas_priceensures comprehensive gas pricing information in the block.
245-247: Ohayo sensei! Properly setl1_gas_priceinPendingBlockWithReceipts.Initialization from
header.l1_gas_pricesensures accurate gas pricing in pending receipts.
249-251: Ohayo sensei! Correctl1_data_gas_priceinitialization in pending receipts.Using
header.l1_data_gas_pricesmaintains consistency in data gas pricing.
269-270: Ohayo sensei! Updatedl1_da_modeinPendingBlockWithReceipts.Setting
l1_da_modetoheader.l1_da_modeensures alignment with the header's data availability mode.
| l1_gas_prices: gas_prices.clone(), | ||
| l1_data_gas_prices: gas_prices.clone(), | ||
| l1_da_mode: L1DataAvailabilityMode::Calldata, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider refactoring repeated initialization of PartialHeader fields
The fields l1_gas_prices, l1_data_gas_prices, and l1_da_mode are initialized with the same values in each ExecutableBlock. To enhance maintainability and reduce code duplication, consider abstracting these common initializations into a helper function or a shared method.
Also applies to: 161-163, 194-196
| // we dont update the `l1_gas_price` bcs its already done when we set the `gas_prices` in | ||
| // genesis. this flow is kinda flawed, we should probably refactor it out of the | ||
| // genesis. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor gas price handling out of genesis
Ohayo sensei! The comments suggest that the current flow for gas price handling is flawed and may benefit from refactoring. Moving gas price initialization out of the genesis block could improve code clarity and maintainability.
Consider abstracting gas price configuration into a dedicated function or module.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2547 +/- ##
==========================================
- Coverage 69.44% 69.42% -0.02%
==========================================
Files 388 388
Lines 49881 49953 +72
==========================================
+ Hits 34640 34682 +42
- Misses 15241 15271 +30 ☔ View full report in Codecov by Sentry. |
fully resolves #2539
ref #2545
the values in the new fields are mostly dumb values rn bcs we arent sampling anything from the L1. so the next todo to make these changes correct, is to create a gas oracle that samples the actual values from the l1.
similar to #2543, the database format for
Headeris changed. considering we already bumped the db version in #2543, we can keep using the same version and defer bumping it again once this commit is included in a release.Summary by CodeRabbit
Release Notes
New Features
L1DataAvailabilityMode, improving the handling of gas prices in block production and execution.Bug Fixes
Documentation