feat(katana): rollup and dev chain spec #2957
Conversation
Chain Specification and Genesis State RefactoringWalkthroughOhayo, sensei! This pull request introduces a comprehensive refactoring of the chain specification and genesis state initialization across the Katana project. The changes primarily focus on enhancing the flexibility and structure of chain specifications by integrating a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Initialization
participant ChainSpec as Chain Specification
participant Backend as Blockchain Backend
participant Genesis as Genesis State Generator
CLI->>ChainSpec: Create Dev/Rollup Specification
ChainSpec->>Genesis: Generate Allocations
Genesis-->>ChainSpec: Return Genesis State
ChainSpec->>Backend: Initialize Genesis Block
Backend->>Backend: Seal Genesis Block
Backend-->>CLI: Genesis Initialization Complete
Possibly related PRs
Suggested Reviewers
Sensei, these changes represent a significant evolution in how we manage chain specifications and genesis states! The new approach provides more flexibility and clarity in our blockchain initialization process. Ohayo and happy reviewing! 🚀🔧 🪧 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: 1
🧹 Nitpick comments (12)
crates/katana/chain-spec/src/lib.rs (1)
16-18: Minor stylistic observation.The comment banners are a nice touch, but ensure consistency across the project to maintain a polished look. Consider using consistent ASCII or doc-comment styles for code clarity.
crates/katana/core/src/backend/storage.rs (1)
Line range hint
97-165: Forking logic improved, but watch code clarity.The updated
new_from_forkedfunction sets chain genesis details to the forked block data. This is well-structured; just ensure that the commented-out logic (lines 149-164) is either removed if obsolete or promptly restored once finalized, to keep the codebase clean.crates/katana/chain-spec/src/rollup/utils.rs (3)
1-4: Ohayo sensei! Checking import consistency.Ensure all imported items (
OnceCell,RefCell, etc.) have usage in this file. Remove any dead imports for tidy code.
43-55: Constructor rationale.
fn new(chain_spec: &'c ChainSpec) -> Selfsets up everything for transactions. Consider clarifying expected usage in doc comments, especially for external consumers.
57-87: Legacy declarations.
legacy_declareproperly handles older class format. The panic if a non-legacy class is passed is direct; consider returning aResultfor more graceful error handling if you foresee user input.crates/katana/chain-spec/src/rollup/mod.rs (1)
36-52: Consider optimizing gas prices handling, sensei!The implementation could be improved in two areas:
- Gas prices are cloned twice (lines 43-44). Consider reusing the cloned value.
- L1DataAvailabilityMode is hardcoded to Calldata. Consider making this configurable if different modes might be needed in the future.
pub fn block(&self) -> ExecutableBlock { let header = PartialHeader { protocol_version: CURRENT_STARKNET_VERSION, number: self.genesis.number, timestamp: self.genesis.timestamp, parent_hash: self.genesis.parent_hash, l1_da_mode: L1DataAvailabilityMode::Calldata, - l1_gas_prices: self.genesis.gas_prices.clone(), - l1_data_gas_prices: self.genesis.gas_prices.clone(), + l1_gas_prices: { + let prices = self.genesis.gas_prices.clone(); + l1_data_gas_prices: prices.clone(), + prices + }, sequencer_address: self.genesis.sequencer_address, };crates/katana/executor/tests/fixtures/transaction.rs (1)
102-102: Consider improving error handling for genesis allocation access, sensei!The
expect("should have account")message could be more descriptive to help with debugging when the error occurs.- let (addr, alloc) = chain.genesis().allocations.first_key_value().expect("should have account"); + let (addr, alloc) = chain.genesis().allocations.first_key_value() + .expect("No genesis accounts found in chain specification");Also applies to: 125-125
crates/dojo/test-utils/src/sequencer.rs (1)
118-119: Consider parameterizing the chain ID in test configuration, sensei!The hardcoded
ChainId::SEPOLIAin the test configuration limits the flexibility of the test utility. Consider making it configurable through theSequencingConfigparameter.- let mut chain = - katana_chain_spec::dev::ChainSpec { id: ChainId::SEPOLIA, ..Default::default() }; + let mut chain = katana_chain_spec::dev::ChainSpec { + id: sequencing.chain_id.unwrap_or(ChainId::SEPOLIA), + ..Default::default() + };crates/katana/primitives/src/genesis/constant.rs (1)
93-95: Add documentation for the new GENESIS_ACCOUNT_CLASS constant, sensei!Consider adding documentation to explain the purpose and usage of this constant, similar to other constants in this file.
+ /// The default genesis account contract class. + /// This class is used for initializing accounts during genesis state creation. pub static ref GENESIS_ACCOUNT_CLASS: ContractClass = read_legacy_class_artifact(include_str!("../../../contracts/build/account.json"));crates/katana/cli/src/utils.rs (1)
126-143: Ohayo sensei! Consider documenting the removal of fee token printing.The removal of ETH and STRK fee token printing logic represents a significant change in how fee tokens are handled. Consider adding a comment explaining why this information is no longer displayed.
bin/katana/src/cli/init/mod.rs (1)
193-194: Consider documenting the fee token address constant.The
DEFAULT_APPCHAIN_FEE_TOKEN_ADDRESSis a critical constant. Consider adding documentation about its significance and how it was derived.+/// The default fee token address for appchains. +/// This address is used as the primary fee token for transaction processing. const DEFAULT_APPCHAIN_FEE_TOKEN_ADDRESS: Felt = felt!("0x2e7442625bab778683501c0eadbc1ea17b3535da040a12ac7d281066e915eea");crates/katana/chain-spec/Cargo.toml (1)
13-13: Ohayo sensei! Consider using workspace version for dirs crate.Using a fixed version
"6.0.0"for thedirscrate while other dependencies use workspace versions could lead to version conflicts. Consider usingdirs.workspace = truefor consistency.-dirs = "6.0.0" +dirs.workspace = true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
bin/katana/src/cli/init/mod.rs(3 hunks)crates/dojo/test-utils/src/sequencer.rs(4 hunks)crates/katana/chain-spec/Cargo.toml(1 hunks)crates/katana/chain-spec/src/dev.rs(1 hunks)crates/katana/chain-spec/src/lib.rs(1 hunks)crates/katana/chain-spec/src/rollup/file.rs(6 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(1 hunks)crates/katana/cli/src/args.rs(6 hunks)crates/katana/cli/src/utils.rs(3 hunks)crates/katana/core/src/backend/mod.rs(5 hunks)crates/katana/core/src/backend/storage.rs(3 hunks)crates/katana/core/src/service/block_producer_tests.rs(2 hunks)crates/katana/core/src/service/messaging/service.rs(2 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/tests/fixtures/mod.rs(2 hunks)crates/katana/executor/tests/fixtures/transaction.rs(2 hunks)crates/katana/node/src/lib.rs(7 hunks)crates/katana/primitives/src/genesis/constant.rs(1 hunks)crates/katana/primitives/src/lib.rs(1 hunks)crates/katana/primitives/src/transaction.rs(3 hunks)crates/katana/primitives/src/utils/transaction.rs(1 hunks)crates/katana/rpc/rpc/src/dev.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/read.rs(3 hunks)crates/katana/rpc/rpc/src/starknet/trace.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/write.rs(3 hunks)crates/katana/rpc/rpc/tests/proofs.rs(2 hunks)crates/katana/storage/provider/Cargo.toml(2 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)examples/simple/dojo_dev.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/executor/src/abstraction/mod.rs
🧰 Additional context used
🪛 Gitleaks (8.21.2)
examples/simple/dojo_dev.toml
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (74)
crates/katana/chain-spec/src/lib.rs (9)
3-3: Ohayo sensei! Good practice on domain-specific imports.You're importing
ethandContractAddressfromkatana_primitives. Verify that both are needed in this file and remove any unused imports if they are not serving a direct purpose.
7-8: Introducing modular separation.Declaring
pub mod dev;andpub mod rollup;is a clean way to separate development and rollup chain specs. Keep them well-organized and documented to aid readability.
11-14: Enum-based chain specifications simplify branching.Converting
ChainSpecinto an enum withDevandRollupvariants is a robust approach. It cleanly differentiates the behaviors for each variant. Great job!
20-24: Convenient dev initialization.
pub fn dev() -> Selfreturning a clonedDEVspec is handy for quick test or dev environment spins. Just ensure that all dev defaults are continuously updated in one place to avoid confusion.
26-31: Correct approach to unify chain IDs.The
id()method ensures consistent retrieval ofChainIdacross Dev or Rollup specs. This is a straightforward pattern that fosters uniform usage.
33-38: Centralizing genesis retrieval.
genesis()method elegantly abstracts the underlying variant's genesis data. Keep it in sync with changes to dev or rollup specs to avoid stale references.
40-45: Conditional settlement.Returning an
Option<&SettlementLayer>forDevvs.Rollupis logical. Monitor usage to ensure consumers properly handle theNonecase forDev.
48-59: Smooth conversions from dev/rollup specs.Implementing
From<dev::ChainSpec>andFrom<rollup::ChainSpec>simplifies construction, enabling easy integration with other components.
60-63: Sensible default sets dev spec.
impl Default for ChainSpecreturning the dev variant is a thoughtful approach for test scaffolding. If rollup usage increases, consider how defaults might shift or remain dev-focused.crates/katana/core/src/backend/storage.rs (4)
24-24: Ohayo sensei! Confirm usage of MaybePendingBlockWithTxHashes.Ensure that
MaybePendingBlockWithTxHashesis used in your logic. If not, consider removing it.
89-90: Simplified new_with_db method.You replaced
new_with_chainwithnew_with_db, which no longer requires a chain spec. This reduces complexity for dev flows, but ensure you test any downstream code that might have relied on the old signature.
168-221: Dev-only genesis initialization workflow.
new_devcentralizes dev genesis creation, verifying block hashes and injecting state updates if none exists. This is a clean approach, but carefully document the assumptions about the chain environment to guide future expansions.
223-224: Straightforward provider accessor.
pub fn provider(&self) -> &BlockchainProvider<Box<dyn Database>>is minimal but essential for encapsulation. Looks good.crates/katana/chain-spec/src/rollup/utils.rs (12)
5-24: Smart choice of library usage.Incorporating
alloy_primitives::U256,katana_primitivesmodules, and more indicates a well-structured approach. Keep an eye on performance when dealing with large numeric operations.
25-32: Comprehensive docstrings.The doc comments for
GenesisTransactionsBuilderare thorough and help readers understand the genesis-building flow. Maintain them as changes are made.
33-41: Organized builder fields.Storing
fee_token,master_address, anddeclared_classesin cells provides flexibility. Just be vigilant about concurrency if you evolve beyond a single-threaded context.
88-127: Decoupled declaration logic.
declaredifferentiates new classes from legacy, stepping up your protocol adaptation. Keep the ordering of operations well-tested, especially if additional transaction types appear.
128-148: Deploy function with structured approach.Using the
deploy_contractselector is coherent. Monitor the overhead as you chain multiple deploys, ensuring the logic remains consistent for large-scale dev usage.
150-184: High-level approach to invocation.
invokewraps the transaction details well. The single-liner to increment nonce is tidy. Good job checking the function selector.
186-228: Deploying predeployed accounts for dev usage.Keeping addresses consistent with the old implementation is crucial for test reliability. If you plan to scale or alter address derivation, consider a well-documented migration path.
230-267: Master account creation logic.
build_master_accountorganizes a deployment flow for your primary dev account. The direct usage oflegacy_declareindicates a stable pattern. Revisit if you adopt new class versions.
269-288: Core contracts deployment.Great approach to ensure UDC and ERC20 are set up prior to dev account creation. Just be sure your references to these default classes remain updated if new versions roll out.
290-310: Ensuring allocated dev accounts.
build_allocated_dev_accountsmethodically deploys each dev account. The check on the expected class hash helps avoid mistakes. Thumbs up, sensei!
312-320: Balance transfers for dev accounts.
transfer_balanceis straightforward. Keep an eye on big integer performance if usage grows.
322-328: Final build method.Sequencing master account, core contracts, and dev accounts in
build()is coherent. The approach is a good sign of your code's maintainability.crates/katana/core/src/backend/mod.rs (10)
3-3: Ohayo sensei! Clean import additions.
These imports logically tie in with new functionalities for chain specs, state updates, receipts, and more. The usage ofanyhowfor context-based error handling looks good, ensuring messages are more descriptive.Also applies to: 13-15, 20-20
70-76: Ohayo sensei! Great approach to initializing genesis blocks.
Thisinit_genesisfunction neatly delegates logic toinit_dev_genesisorinit_rollup_genesis, promoting clarity and maintainability. The usage ofmatchensures easy extensibility if future chain spec variants are introduced.
85-85: Ohayo sensei! Solid transaction filtering within mine_block.
Only successful transactions are included, which is crucial for block finality. This helps avoid polluting the chain with failed or partially executed transactions. The approach is consistent with typical block production flow.Also applies to: 91-91, 96-97
100-110: Ohayo sensei! PartialHeader creation appears efficient.
Defining the header directly ensures all relevant metadata (block number, timestamp, etc.) is captured early. PassingCURRENT_STARKNET_VERSIONclarifies protocol version usage.
112-115: Ohayo sensei! Good layering of commit_block.
Explicitly passing references for receipts and state updates fosters clarity, ensuring the block is committed with consistent data.
124-124: Ohayo sensei! Proper block storage invocation.
Storing the block after commit is essential for finalizing its data. This design keeps the chain state consistent.
130-142: Ohayo sensei!store_blockencapsulates block insertion logic neatly.
This function centralizes the final insertion into the database, enhancing maintainability and easing future changes.
207-224: Ohayo sensei!commit_blocksignature is well-structured.
Switching fromBlockEnvtoPartialHeaderaligns data more tightly with the immediate block data. This methodically updates state roots and captures context fromstate_updates.
228-279: Ohayo sensei! Dev genesis initialization is thorough.
It checks for a preexisting genesis, compares block hashes, and handles mismatch scenarios properly. This approach helps prevent accidental re-initialization of an incompatible genesis.
280-338: Ohayo sensei! Rollup genesis execution logic is impressive.
Executing the genesis block right away ensures the chain is “hot” from the get-go. The check on stored block hashes avoids unintentional re-initialization. Nicely done.crates/katana/chain-spec/src/dev.rs (8)
1-41: Ohayo sensei! The newChainSpecstruct is well-defined.
Fields likeid,genesis,fee_contracts, and an optionalsettlementare logically grouped, giving a straightforward approach to representing a dev chain environment.
43-116: Kudos sensei! Theblockandstate_updatesmethods are comprehensive.
Creating aBlockfrom the genesis data and combining fee tokens plus the universal deployer contract withinstate_updatesensures a standardized environment for dev usage.
127-131: Ohayo sensei! Smart usage of Default for ChainSpec.
Defaulting toDEVmode is a convenient pattern, reducing repetitive instantiation and enhancing developer productivity.
133-161: Nicely done sensei! Lazy static initialization for dev specs is a tidy approach.
DEV_UNALLOCATEDandDEVclearly separate minimal from fully allocated specs, making it easier to flexibly generate dev chain states.
163-191: Ohayo sensei! Good consolidation of default fee token setup.
By factoring outadd_default_fee_tokens(...), you ensure the chain always has essential tokens for transaction fees. This helps unify dev allocations with consistent token logic.
193-241: Ohayo sensei!add_fee_tokenmethod elegantly handles total supply initialization.
Calculatingtotal_supplyfrom the allocations matches real usage scenarios where supply is the sum of all minted balances. Storing metadata (name, symbol, decimals) is well-structured and predictable.
243-257: Smooth approach for the universal deployer contract (UDC).
Declaring and deploying UDC ensures the dev environment can rely on a standard contract for universal deployment. Great for test networks.
259-647: Thorough tests, sensei!
These unit tests validate classes, storage, fee token balances, and universal deployer correctness. Covering multiple allocations verifies real-world usage. All appear consistent with the chain’s logic.crates/katana/primitives/src/lib.rs (1)
24-24: Ohayo sensei! Publicly re-exportingU256is practical.
HavingU256accessible at this level streamlines usage across the codebase, preventing scattered import statements.crates/katana/chain-spec/src/rollup/mod.rs (2)
14-29: Ohayo! Clean and well-structured ChainSpec definition, sensei!The struct is well-documented and contains all essential components for a rollup chain specification.
54-59: Clean FeeContract implementation, sensei!The struct is concise and uses appropriate types for type safety.
crates/katana/storage/provider/src/test_utils.rs (1)
31-36: Nice improvements to test initialization, sensei!The changes enhance clarity through:
- More descriptive variable naming (
state_updates)- Explicit block creation with
Block::default()- Clear separation of block sealing and state updates
crates/katana/core/src/service/block_producer_tests.rs (1)
15-21: Well-structured test backend initialization, sensei!The changes improve test reliability by:
- Using explicit ChainSpec::dev() initialization
- Adding proper genesis initialization
- Simplifying blockchain creation
crates/katana/rpc/rpc/src/dev.rs (1)
96-96: Clean account retrieval implementation, sensei!The change properly encapsulates genesis access through a method call while maintaining a concise transformation of account data.
crates/katana/rpc/rpc/src/starknet/write.rs (1)
24-24: Ohayo! Nice encapsulation of chain ID access, sensei!The transition from direct field access (
chain_spec.id) to method calls (chain_spec.id()) improves encapsulation and provides better flexibility for future changes to chain ID handling.Also applies to: 43-43, 64-64
crates/katana/cli/src/utils.rs (2)
13-13: LGTM! Import statement updated for specific constants.The import now correctly specifies the exact constants needed from the genesis module.
82-82: Verify the change in genesis access pattern.The code has been updated to use
chain.genesis().accounts()instead of direct property access. This change suggests a broader architectural shift in how genesis data is accessed.✅ Verification successful
Ohayo sensei! The genesis access pattern is consistent across the codebase
The
genesis().accounts()pattern is already the standard way to access genesis accounts throughout the codebase, including RPC, CLI, and test utilities. This change follows the established pattern rather than indicating an architectural shift.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of genesis access patterns rg -A 2 'genesis\(\)\.accounts\(\)'Length of output: 1106
crates/katana/chain-spec/src/rollup/file.rs (2)
176-188: LGTM! Well-structured chainspec test helper.The new
chainspec()function provides a clean way to create test chain specifications with default values.
53-53: Ohayo! Significant architectural change in fee contract handling.The code has been refactored from handling multiple fee contracts to a single fee contract. This is a breaking change that simplifies the fee contract architecture.
Also applies to: 64-64
✅ Verification successful
Ohayo! Verified architectural change in fee contract handling, sensei!
The codebase has been intentionally refactored to support two distinct modes:
- Dev mode: Uses separate ETH and STRK fee contracts
- Rollup mode: Uses a single fee contract
The changes are properly implemented across the codebase with appropriate handling for both variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old fee_contracts structure rg 'fee_contracts'Length of output: 650
Script:
#!/bin/bash # Check if these files are modified in the current PR git diff --name-only HEAD~1 HEAD | grep -E "node/src/lib.rs|chain-spec/src/dev.rs" # Also check the actual changes in these files git diff HEAD~1 HEAD crates/katana/node/src/lib.rs crates/katana/chain-spec/src/dev.rsLength of output: 29050
bin/katana/src/cli/init/mod.rs (2)
45-50: Clean implementation of chain specification initialization.The new implementation clearly separates concerns by explicitly initializing each component of the chain specification.
186-186: LGTM! Enhanced genesis allocation with explicit balance.Using
DEFAULT_PREFUNDED_ACCOUNT_BALANCEmakes the account balance initialization more explicit and maintainable.crates/katana/rpc/rpc/src/starknet/read.rs (1)
28-28: Ohayo sensei! Consistent update to chain ID access pattern.The code has been updated to use
chain_spec.id()consistently across all methods. This change aligns with the new chain specification architecture.Also applies to: 174-174, 237-237
✅ Verification successful
Ohayo sensei! Chain ID access pattern is consistently implemented across the codebase.
The verification shows that all chain ID accesses use the
chain_spec.id()pattern uniformly across node operations, RPC endpoints, and core services.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of the new chain ID access pattern rg 'chain_spec\.id\(\)'Length of output: 1199
crates/katana/executor/tests/fixtures/mod.rs (2)
Line range hint
53-66: Ohayo! LGTM - Clean transition to enum-based ChainSpec.The changes properly handle the new ChainSpec enum structure, correctly cloning and extending the DEV_UNALLOCATED chain spec.
72-73: Nice pattern matching for type safety!The explicit pattern matching on
ChainSpec::Devensures type safety and clear error messaging.crates/katana/node/src/lib.rs (3)
164-171: Ohayo sensei! Clean fee token configuration based on chain spec type.The pattern matching cleanly handles different fee token configurations for Dev and Rollup chain specs.
191-195: Good error handling for forking limitations.Clear error message indicating that forking is only supported in dev mode.
241-241: Nice addition of error context for genesis initialization.The addition of error context using
.context()improves error messages for debugging.crates/katana/core/src/service/messaging/service.rs (1)
88-88: Ohayo! Consistent method call syntax for chain spec id.The changes from field access to method calls (
backend.chain_spec.id()) align with the new ChainSpec implementation.Also applies to: 106-106
crates/katana/rpc/rpc/src/starknet/trace.rs (1)
30-30: Ohayo! This change looks good, sensei!The modification from direct field access to method call (
chain_spec.id()) improves encapsulation and follows good OOP practices.crates/katana/primitives/src/utils/transaction.rs (1)
104-124: Ohayo! Nice implementation of V0 declare transaction hash computation, sensei!The new function is well-documented and follows the established pattern of other transaction hash computations. The reference link provides good traceability to the implementation source.
crates/katana/primitives/src/transaction.rs (3)
12-14: Ohayo! Clean import organization, sensei!The imports are well-organized and properly grouped for the new hash computation functions.
117-124: Nice addition of the calculate_hash method!The implementation properly handles all transaction types and maintains consistency with the existing pattern.
441-448: Good implementation of V0 declare transaction hash calculation!The previous "ignored" comment has been replaced with a proper implementation that matches the pattern of other transaction versions.
crates/katana/cli/src/args.rs (3)
242-245: Ohayo! Nice refactoring of chain spec handling, sensei!The change to use
katana_chain_spec::rollup::fileand proper wrapping withChainSpec::Rollupimproves the code organization and type safety.
Line range hint
249-274: Good separation of development chain spec handling!The implementation properly handles development mode chain specifications with appropriate default values and customization options.
439-441: Clean update of test assertions!The test cases have been properly updated to use method calls instead of direct field access, maintaining consistency with the new encapsulation approach.
Also applies to: 466-467, 570-576, 634-641
crates/katana/chain-spec/Cargo.toml (1)
15-21: LGTM! Clean dependency management.The workspace versioning for
num-traitsandurlaligns well with the project's dependency management strategy.examples/simple/dojo_dev.toml (1)
9-10: Ohayo sensei! Please review the security implications.The private key is exposed in the configuration file. While this appears to be a development configuration (based on the comment "Default account for katana with seed = 0"), we should:
- Verify these are test-only credentials
- Consider using environment variables for sensitive data
- Add a warning comment to never use these credentials in production
Let's verify if these credentials are used only in tests:
✅ Verification successful
Ohayo sensei! Your development credentials are safe!
The credentials in
dojo_dev.tomlare confirmed to be development-only, as they are:
- Located in the examples/simple directory
- Part of a development configuration file
- Only used for local testing purposes
However, it would be helpful to add a comment warning about production usage:
# WARNING: These are development-only credentials. Never use in production.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the account address usage rg "0x1f401c745d3dba9b9da11921d1fb006c96f571e9039a0ece3f3b0dc14f04c3d" -A 2Length of output: 419
Script:
#!/bin/bash # Search for private key and check documentation rg "0x7230b49615d175307d580c33d6fda61fc7b9aec91df0f5c1a5ebe3b8cbfee02" fd "README|CONTRIBUTING" --exec cat {}Length of output: 25953
🧰 Tools
🪛 Gitleaks (8.21.2)
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
crates/katana/storage/provider/Cargo.toml (1)
10-13: Ohayo sensei! Clean modularization approach!The changes to
katana-chain-specdependency management are well-structured:
- Making it optional allows for flexible integration
- Including it in test-utils and dev-dependencies maintains proper separation of concerns
Also applies to: 37-41
kariy
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2957 +/- ##
==========================================
+ Coverage 56.70% 56.89% +0.18%
==========================================
Files 420 423 +3
Lines 55557 56070 +513
==========================================
+ Hits 31504 31900 +396
- Misses 24053 24170 +117 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/katana/chain-spec/src/rollup/mod.rs (2)
15-30: Ohayo! Consider enhancing the documentation for better clarity, sensei.While the struct is well-organized, the documentation could be more comprehensive. Consider adding:
- Purpose and constraints for each field
- Example usage
- Relationship between fields (e.g., how fee_contract interacts with settlement)
43-45: Consider parameterizing L1 data availability mode and gas prices, sensei.The current implementation:
- Hardcodes L1DataAvailabilityMode to Calldata
- Duplicates gas prices for both l1_gas_prices and l1_data_gas_prices
Consider making these configurable through ChainSpec for more flexibility.
bin/katana/src/cli/init/mod.rs (3)
45-51: Consider adding validation and customization options, sensei.The chain specification initialization could be improved:
- Add validation for chain ID format
- Allow customization of genesis parameters
- Document the limitations of the predefined fee token
47-48: Document fee token limitations and track future improvements, sensei.The comment about fee token limitations should be:
- Documented in a more formal way (e.g., in API docs)
- Tracked in an issue for future implementation
Would you like me to help create an issue to track the fee token customization feature?
187-187: Consider making genesis account allocation more flexible, sensei.The current implementation:
- Uses a single predefined balance for all accounts
- Creates only one account
Consider adding support for:
- Multiple accounts with different balances
- Custom balance configuration through CLI
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/katana/src/cli/init/mod.rs(3 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (1)
crates/katana/chain-spec/src/rollup/mod.rs (1)
55-58: Verify the hardcoded fee token address across environments, sensei.The hardcoded
DEFAULT_APPCHAIN_FEE_TOKEN_ADDRESSmight cause issues in different environments. Consider:
- Making it configurable based on the environment
- Adding validation for the address format
- Documenting the source and significance of this specific address
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/katana/chain-spec/src/rollup/utils.rs (5)
58-60: Ohayo sensei! Enhance the error message for better debugging.The panic message could be more descriptive by including the actual class type received.
- panic!("legacy_declare must be called only with legacy class") + panic!("legacy_declare must be called only with legacy class, got {:?}", class)
131-143: Ohayo sensei! Consider extracting magic values to constants.The deploy method uses several magic values that could be moved to named constants for better maintainability:
"deploy_contract"selectorFelt::ZEROfor saltFelt::ONEfor deploy_from_zero+ const DEPLOY_FROM_ZERO: Felt = Felt::ONE; + const DEFAULT_SALT: Felt = Felt::ZERO; const DEPLOY_CONTRACT_SELECTOR: &str = "deploy_contract";
238-238: Ohayo sensei! Enhance error messages for better debugging.The
expecterror messages could be more descriptive by including relevant context:- self.master_address.set(master_address.into()).expect("must be uninitialized"); + self.master_address.set(master_address.into()).expect("master_address must be uninitialized"); - let master_address = *self.master_address.get().expect("must be initialized first"); + let master_address = *self.master_address.get().expect("master_address must be initialized before building core contracts"); - self.fee_token.set(fee_token_address).expect("must be uninitialized"); + self.fee_token.set(fee_token_address).expect("fee_token must be uninitialized when setting up core contracts");Also applies to: 273-273, 287-287
322-328: Ohayo sensei! Add documentation for public methods.The
buildmethod is public but lacks documentation explaining its purpose, parameters, and return value.Consider adding documentation:
+ /// Builds and returns a vector of executable transactions for the genesis block. + /// + /// This method orchestrates the entire genesis setup by: + /// 1. Building the master account + /// 2. Building core contracts + /// 3. Building allocated dev accounts + /// + /// # Returns + /// A vector of executable transactions with their hashes pub fn build(mut self) -> Vec<ExecutableTxWithHash> {
33-41: Ohayo sensei! Consider architectural improvements for better extensibility.The
GenesisTransactionsBuilderhas a good foundation, but consider these architectural improvements:
- Extract the transaction building logic into a trait to allow different implementations
- Consider using the Builder pattern more explicitly with method chaining
- Add a validation step before building transactions
This would make the code more maintainable and extensible for future changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/chain-spec/src/rollup/utils.rs(1 hunks)
🔇 Additional comments (2)
crates/katana/chain-spec/src/rollup/utils.rs (2)
53-53: Ohayo sensei! Consider making the master signer configurable.The master signer is initialized with a hardcoded value
0xa55. This could be a security concern in production environments.Consider making it configurable through the
ChainSpecor constructor parameters.
366-367: Ohayo sensei! Address the empty test function.The
genesis_transactionstest function is empty. Either implement meaningful test cases or remove it.
|
that's nice to have those two possibilities available, and keep this direct access to the database from the genesis. And easier to define the rollup one for easier block 0 proving. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/katana/chain-spec/src/rollup/utils.rs (4)
28-29: Consider making the fee token address configurable.The hardcoded fee token address might be difficult to change in the future. Consider making it configurable through the chain specification.
31-33: Update documentation reference.The documentation references
Genesisbut should referenceChainSpecinstead, as that's what the builder uses./// A convenience builder for creating valid and executable transactions for the genesis block based -/// on the [`Genesis`]. +/// on the [`ChainSpec`].
334-498: Ohayo, sensei! Consider adding more test cases.The test coverage is good, but consider adding these scenarios:
- Error cases (invalid class types, mismatched addresses)
- Edge cases for balance transfers (zero balance, max U256)
- Concurrent execution safety
326-331: Document the transaction execution order.The
buildmethod executes transactions in a specific order (as shown in thetransaction_ordertest), but this order isn't documented. Consider adding documentation about the expected transaction order to help users understand the initialization process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/katana/chain-spec/Cargo.toml(1 hunks)crates/katana/chain-spec/src/rollup/mod.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/chain-spec/src/rollup/mod.rs
🔇 Additional comments (2)
crates/katana/chain-spec/Cargo.toml (2)
24-26: Excellent test coverage setup, sensei!The addition of
katana-executor,katana-providerwith test-utils, andrstestas dev dependencies suggests comprehensive test coverage for both development and rollup chain specifications. This aligns perfectly with the PR's objective of implementing and validating different genesis initialization procedures.
13-13: Ohayo! Consider using workspace version for dirs crate.The
dirscrate is pinned to version 6.0.0 while other dependencies use workspace versions. This could lead to version conflicts if the workspace needs to use a different version.-dirs = "6.0.0" +dirs.workspace = trueLet's check if this is the latest stable version:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/katana/core/src/backend/storage.rs (1)
166-166: Consider addressing the TODO comments about gas price.There are TODO comments about removing gas price from genesis. Would you like me to help create an issue to track this technical debt?
crates/katana/core/src/backend/mod.rs (3)
70-75: Clean implementation of genesis initialization.Good use of pattern matching for different chain specifications. Consider using a custom error type instead of anyhow::Result for more specific error handling.
85-91: Consider optimizing memory allocation.Pre-allocation is good, but we could optimize further by:
- Using
Vec::with_capacity(execution_output.transactions.len())once and cloning the capacity to other vectors- Filtering transactions in-place
- let mut traces = Vec::with_capacity(execution_output.transactions.len()); - let mut receipts = Vec::with_capacity(execution_output.transactions.len()); - let mut transactions = Vec::with_capacity(execution_output.transactions.len()); + let capacity = execution_output.transactions.len(); + let mut traces = Vec::with_capacity(capacity); + let mut receipts = Vec::with_capacity(capacity); + let mut transactions = Vec::with_capacity(capacity);
328-330: TODO: Consider changing insert_block_with_states_and_receipts arguments.The comment suggests changing the method to accept ReceiptWithTxHash directly. Would you like me to help implement this improvement?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/katana/cli/src/utils.rs(3 hunks)crates/katana/core/src/backend/mod.rs(5 hunks)crates/katana/core/src/backend/storage.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/utils.rs
🔇 Additional comments (4)
crates/katana/core/src/backend/storage.rs (3)
25-25: LGTM! Clean import additions.The new imports align well with the enhanced functionality for block handling.
98-98: Type safety improvement in chain specification.The change to use the concrete
katana_chain_spec::dev::ChainSpectype improves type safety.
90-91: Verify the removal of ChainSpec parameter.The method has been simplified, which is good. However, let's verify that the ChainSpec is properly initialized elsewhere.
✅ Verification successful
Ohayo sensei!
The removal of ChainSpec parameter is correct and improves the design
The
new_with_dbmethod is properly focused on basic DB initialization, while chain-specific setup is handled by dedicated methods likenew_devandnew_from_forked. This separation of concerns is cleaner and more maintainable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for new_with_db usage to ensure ChainSpec is properly handled rg -A 3 "new_with_db\("Length of output: 722
Script:
#!/bin/bash # Check implementation of new and related methods ast-grep --pattern 'impl $_ { $$$ pub fn new($$$) { $$$ } $$$ }'Length of output: 78
Script:
#!/bin/bash # Check the storage.rs file for implementation details cat crates/katana/core/src/backend/storage.rsLength of output: 8505
crates/katana/core/src/backend/mod.rs (1)
279-337: Solid implementation of rollup genesis initialization.The implementation includes:
- Proper block execution
- Good error handling with context
- Clear validation of genesis state
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/katana/storage/provider/src/providers/in_memory/state.rs (2)
113-114: Add documentation for EmptyStateProvider, sensei!Consider adding documentation to explain:
- The purpose of this null object implementation
- When and how it should be used
- Why returning empty/default values is the expected behavior
Add this documentation:
+/// A no-op implementation of state-related traits that returns empty/default values. +/// This provider is useful in testing scenarios or when a null implementation is needed. #[derive(Debug)] pub struct EmptyStateProvider;
116-200: Clean trait implementations with consistent behavior!The implementations follow good practices:
- Consistent use of empty/default values
- Proper handling of unused parameters
- Clear and maintainable code structure
One suggestion to consider: Since this is a null implementation, you could reduce boilerplate by deriving Default for the MultiProof type and implementing a blanket method that returns default values.
crates/katana/core/src/backend/mod.rs (4)
70-75: Ohayo! Consider enhancing error handling structure for genesis initialization.The initialization logic is well-implemented, but we could improve error handling by creating dedicated error types instead of using
anyhow::Error. This would make error handling more explicit and maintainable.+#[derive(Debug, thiserror::Error)] +pub enum GenesisError { + #[error("Genesis block hash mismatch: expected {expected}, got {actual}")] + HashMismatch { + expected: BlockHash, + actual: BlockHash, + }, + #[error("Failed to execute genesis block: {0}")] + ExecutionFailed(#[from] ExecutionError), + #[error("Failed to update trie: {0}")] + TrieError(#[from] TrieError), +} -pub fn init_genesis(&self) -> anyhow::Result<()> +pub fn init_genesis(&self) -> Result<(), GenesisError>Also applies to: 227-277, 279-336
Line range hint
78-125: Ohayo sensei! Consider adding debug logs for transaction processing.While the block mining logic is solid, adding debug logs for transaction processing would help with troubleshooting issues in production.
for (tx, res) in execution_output.transactions { + debug!(target: LOG_TARGET, ?tx.hash, "Processing transaction"); if let ExecutionResult::Success { receipt, trace, .. } = res { receipts.push(ReceiptWithTxHash::new(tx.hash, receipt)); transactions.push(tx); traces.push(trace); + } else { + debug!(target: LOG_TARGET, ?tx.hash, "Transaction failed to execute"); } }
130-141: Add documentation for the store_block method.This new method would benefit from documentation explaining its purpose and the relationship between its parameters.
+/// Stores a block and its associated data in the blockchain. +/// +/// # Arguments +/// * `block` - The sealed block with its finality status +/// * `states` - State updates and declared classes from block execution +/// * `receipts` - Transaction receipts from block execution +/// * `traces` - Execution traces for debugging purposes +/// +/// # Errors +/// Returns `BlockProductionError` if storage operations fail fn store_block( &self, block: SealedBlockWithStatus, states: StateUpdatesWithClasses, receipts: Vec<Receipt>, traces: Vec<TxExecInfo>, ) -> Result<(), BlockProductionError>
205-224: Address TODO for block hash registry contract.The code has a TODO comment about creating a dedicated struct for the block hash registry contract. This would improve code organization and maintainability.
Would you like me to help create a dedicated struct for the block hash registry contract? This would include:
- Contract address constants
- Storage slot calculations
- Methods for reading and writing block hashes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/katana/core/src/backend/mod.rs(5 hunks)crates/katana/storage/provider/src/providers/in_memory/state.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (1)
crates/katana/storage/provider/src/providers/in_memory/state.rs (1)
5-12: Ohayo! Clean and well-organized imports, sensei!The imports are properly grouped and scoped, with clear separation between external primitives and local traits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/core/tests/backend.rs (2)
63-63: Consider making the RPC URL configurable, sensei!The hardcoded URL
http://localhost:5050might not be suitable for all test environments.- rpc_url: Url::parse("http://localhost:5050").unwrap(), + rpc_url: std::env::var("STARKNET_RPC_URL") + .map(|url| Url::parse(&url).unwrap()) + .unwrap_or_else(|_| Url::parse("http://localhost:5050").unwrap()),
69-75: Consider enhancing the test assertions, sensei!While the test verifies successful initialization, it could benefit from additional assertions to verify the state after initialization.
fn can_initialize_genesis(#[case] chain: ChainSpec) { let backend = backend(&chain); backend.init_genesis().expect("failed to initialize genesis"); + + // Verify genesis block + let genesis_block = backend.chain().get_block_by_number(0) + .expect("failed to get genesis block"); + assert!(genesis_block.is_some(), "Genesis block should exist"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/core/tests/backend.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/katana/core/tests/backend.rs (2)
18-29: Ohayo! The executor configuration looks good, sensei!The use of maximum values for validation steps and recursion depth is appropriate for testing scenarios, ensuring no artificial limitations during test execution.
31-42: Clean and well-structured backend initialization, sensei!The separation into two functions with different database configurations provides good flexibility for testing scenarios.
crates/katana/core/src/backend/gas_oracle.rs (1)
71-76: Clean implementation of the zero gas oracle, sensei!The function provides a convenient way to create a gas oracle with zero prices for testing scenarios.
crates/katana/core/Cargo.toml (1)
53-53: Good choice of testing framework, sensei!Adding rstest as a workspace dependency is appropriate for implementing parameterized tests.
The main idea of this PR is (1) to define concrete types for the different ways the node chain spec can be configured, and (2) implement different genesis initialization procedures based on the chain spec types.
We define two separate chain spec types (but combined as an enum when it is consumed by the node):-
katana_chain_spec::dev::ChainSpeckatana_chain_spec::rollup::ChainSpecrollup::ChainSpec::block().snos*, as they are meant to be provable.By defining these types separately, the node can handle the different setup processes more clearly.
* Our version of
snosSummary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
New Features
ChainSpecenum supporting development and rollup modes.Improvements
Technical Refinements
Testing
These changes represent significant improvements to the Katana blockchain infrastructure, focusing on flexibility, robustness, and developer experience.