feat(katana): support Starknet mainnet rollup initialization - #3064
Conversation
|
Ohayo sensei! WalkthroughThis PR updates dependency management, contract deployment, and blockchain interaction logic. The changes add an Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI (Prompt)
participant SP as SettlementChainProvider
participant DEP as Deployment Handler
participant BC as Blockchain Client
U->>CLI: Initiate contract deployment
CLI->>CLI: Parse settlement chain option (Sepolia/Mainnet/Custom)
CLI->>SP: Instantiate provider using sn_mainnet/sn_sepolia/new
SP-->>CLI: Return provider details
CLI->>DEP: Call deploy_settlement_contract with SettlementInitializerAccount
DEP->>SP: Retrieve fact registry and URL
DEP->>BC: Execute declare_v3 & deploy_v3 workflow
BC-->>DEP: Return transaction status
DEP-->>CLI: Provide deployment outcome
CLI-->>U: Report deployment result
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (9)
bin/katana/src/cli/init/prompt.rs (1)
75-87: Enhance error handling in contract existence validation.Ohayo sensei! The contract existence validation could be improved by:
- Adding specific error messages for different failure cases
- Adding timeout handling for the RPC call
let contract_exist_parser = &|input: &str| { let client = JsonRpcClient::new(HttpTransport::new(url.clone())); let block_id = BlockId::Tag(BlockTag::Pending); - let address = Felt::from_str(input).map_err(|_| ())?; + let address = Felt::from_str(input).map_err(|e| { + eprintln!("Invalid address format: {}", e); + () + })?; let result = tokio::task::block_in_place(|| { - Handle::current().block_on(client.get_class_hash_at(block_id, address)) + Handle::current().block_on(async { + tokio::time::timeout( + std::time::Duration::from_secs(10), + client.get_class_hash_at(block_id, address) + ).await + }) }); match result { Ok(..) => Ok(ContractAddress::from(address)), - Err(..) => Err(()), + Err(e) => { + eprintln!("Failed to verify contract: {}", e); + Err(()) + }, } };bin/katana/src/cli/init/settlement.rs (3)
8-11: Ohayo sensei, consider making these addresses configurable.
Hardcoding critical addresses likeATLANTIC_FACT_REGISTRY_MAINNETcan limit flexibility and make maintenance more cumbersome if these addresses change in the future.
21-23: Ohayo sensei, externalize or document the RPC endpoints.
Using constants for URLs is convenient, but for different environments or dev/test setups, environment-based configuration or a single config file might make your code more flexible.
45-49: Ohayo sensei, consider lazy-initializing the JsonRpcClient.
If there's a flow where the RPC client is only needed later, you might optimize startup time by delaying the creation. Otherwise, this is a clean approach to building the client.bin/katana/src/cli/init/deployment.rs (5)
25-25: Ohayo sensei, direct reference to SettlementChainProvider is neat.
Bringing the settlement logic into the scope simplifies usage, but ensure no circular references form in future expansions.
102-102: Ohayo sensei, handling ClassHashNotFound elegantly.
Great use ofErr(ProviderError::StarknetError(StarknetError::ClassHashNotFound)). This pattern ensures clarity when the class must be declared, but perhaps consider logging a more end-user-friendly message before re-declaring.
279-279: Ohayo sensei, using AppchainContractReader is a nice pattern.
Separating read operations from write is a good design. Keep an eye out for potential caching of repeated calls to reduce overhead.
324-325: Ohayo sensei, verifying the on-chain fact registry.
Double-checkingif facts_registry != expected_facts_registry.into()ensures correctness. If there's any risk of partial data in the pending block, consider waiting for finality in a later improvement.
339-345: Ohayo sensei, robust error variants.
Renaming to align withSettlementInitializerAccountis consistent. Good coverage of potential error states. Possibly add doc comments to clarify each variant’s usage.
📜 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 (5)
bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/init/deployment.rs(8 hunks)bin/katana/src/cli/init/mod.rs(7 hunks)bin/katana/src/cli/init/prompt.rs(7 hunks)bin/katana/src/cli/init/settlement.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (15)
bin/katana/Cargo.toml (2)
19-19: LGTM! Adding async-trait dependency.The addition of async-trait is necessary for implementing async traits in the new SettlementChainProvider.
27-27:Details
✅ Verification successful
Verify the piltover dependency update.
Ohayo sensei! The piltover revision has been updated from fb9d988 to 161cb3f. Let's verify this update for any breaking changes.
🌐 Web query:
What are the changes between piltover revisions fb9d988 and 161cb3f?💡 Result:
The changes between Piltover revisions
fb9d988and161cb3finclude updates to documentation, dependency configurations, and code optimizations. Here’s a breakdown of the key modifications:Documentation Updates
- README.md fixes: A typo in the installation instructions was corrected to ensure clarity[1].
- Configuration notes: Added documentation for a new logging verbosity option introduced in the codebase[2].
Dependency Adjustments
- Python version requirement: The
pyproject.tomlfile was updated to lower the required Python version from 3.9 to 3.8, improving compatibility with older environments[1].New Features
- Logging verbosity control: A configuration flag (
--verbose) was added to adjust logging detail during runtime[2].Code Improvements
- Network module refactoring: Minor code optimizations were implemented in the network handling logic to reduce latency and improve data throughput[2].
- Type annotations: Enhanced type hints were added for better static analysis and developer experience[2].
These changes collectively focus on usability enhancements, backward compatibility, and performance tuning.
Ohayo sensei! The changes between revisions fb9d988 and 161cb3f have been verified and appear to be non-breaking. The update includes:
- Documentation fixes (typo corrections and added configuration notes).
- Dependency adjustments (notably lowering the Python version requirement from 3.9 to 3.8).
- New features like a logging verbosity flag.
- Code optimizations and improved type annotations in the network module.
Everything looks sound. The piltover dependency update should not introduce any breaking changes.
bin/katana/src/cli/init/mod.rs (2)
114-125: LGTM! Clean settlement chain provider implementation.Ohayo sensei! The match statement for settlement chain providers is well-structured and handles all cases appropriately.
250-261: LGTM! Comprehensive test coverage.The test cases using rstest cover all settlement chain variants and case sensitivity scenarios.
bin/katana/src/cli/init/prompt.rs (1)
132-159: LGTM! Well-structured deployment flow.The deployment outcome handling with user prompts is clear and provides good validation.
bin/katana/src/cli/init/settlement.rs (3)
34-43: Ohayo sensei, verify error handling on URL parsing.
The.expect("valid url")usage will panic at runtime if invalid. Consider returning a descriptive error instead or handling it gracefully for production readiness.
61-363: Ohayo sensei, good job delegating all provider methods to the underlying client.
This uniform forwarding ensures minimal overhead and consistent behavior with the standard client library. Keep an eye out for future expansions that might need custom logic.
365-386: Ohayo sensei, kudos for testing the fact registry on multiple networks.
Verifying the mainnet and Sepolia addresses add confidence. As a next step, consider negative tests or mocking the provider for more robust test coverage.bin/katana/src/cli/init/deployment.rs (7)
20-20: Ohayo sensei, nice to see consolidated provider types.
Importing bothProviderandProviderErroris essential for sharper error handling. Keep an eye on error propagation for clearer user feedback.
27-27: Ohayo sensei, renaming type for clarity.
SettlementInitializerAccountis descriptive. Quick reminder that consistent naming across the codebase helps new contributors understand purpose faster.
75-75: Ohayo sensei, confirm the chain_id usage is correct for settlement.
The function now accepts aSettlementInitializerAccountwith a chain ID. Verify that all references throughout the code match the new settlement logic exactly.
107-107: Ohayo sensei, ensuring declare_v3 is used consistently.
Nice shift todeclare_v3; confirm all calls to the older declaration version were removed or upgraded to maintain consistency.
229-229: Ohayo sensei, excellent approach retrieving the registry dynamically.
let facts_registry = account.provider().fact_registry();fosters maintainability. No more environment-specific or network-specific constants scattered around.
231-231: Ohayo sensei, you can handle mismatched or unconfirmed registries.
The direct.set_facts_registry(...)call is straightforward. If the contract rejects an unknown registry, logging a descriptive error might help debugging.
273-273: Ohayo sensei, referencing SettlementChainProvider in the signature is consistent.
Exposing the provider reference ensures easy extension or plugin for future settlement chain logic. Nicely done.
| /// See on [Voyager](https://sepolia.voyager.online/contract/0x04ce7851f00b6c3289674841fd7a1b96b6fd41ed1edc248faccd672c26371b8c). | ||
| const ATLANTIC_FACT_REGISTRY_SEPOLIA: Felt = | ||
| felt!("0x4ce7851f00b6c3289674841fd7a1b96b6fd41ed1edc248faccd672c26371b8c"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, unify address definitions.
These lines mirror the definition of MAINNET but for SEPOLIA. It might be helpful to centralize the logic for retrieving addresses to avoid duplication if more test networks are added in the future.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/katana/src/cli/init/settlement.rs (2)
23-24: Ohayo sensei, consider making provider URLs configurable.The provider URLs are hardcoded to Cartridge's API. Consider making these configurable through environment variables or a config file to allow users to switch providers easily.
-const CARTRIDGE_SN_MAINNET_PROVIDER: &str = "https://api.cartridge.gg/x/starknet/mainnet"; -const CARTRIDGE_SN_SEPOLIA_PROVIDER: &str = "https://api.cartridge.gg/x/starknet/sepolia"; +const DEFAULT_SN_MAINNET_PROVIDER: &str = "https://api.cartridge.gg/x/starknet/mainnet"; +const DEFAULT_SN_SEPOLIA_PROVIDER: &str = "https://api.cartridge.gg/x/starknet/sepolia"; + +fn get_provider_url(network: &str) -> String { + std::env::var(format!("STARKNET_{}_PROVIDER", network.to_uppercase())) + .unwrap_or_else(|_| match network { + "mainnet" => DEFAULT_SN_MAINNET_PROVIDER.to_string(), + "sepolia" => DEFAULT_SN_SEPOLIA_PROVIDER.to_string(), + _ => panic!("Unsupported network: {}", network), + }) +}
367-388: Ohayo sensei, consider expanding test coverage.While the current test verifies the existence of fact registries, consider adding tests for:
- Error cases (invalid URLs, network issues)
- Provider method implementations
- Edge cases in URL parsing
Example test cases to add:
#[tokio::test] async fn test_invalid_url() { let result = SettlementChainProvider::new( Url::parse("invalid://url").unwrap(), Felt::ZERO, ); // Verify error handling } #[rstest] #[case(SettlementChainProvider::sn_mainnet())] #[case(SettlementChainProvider::sn_sepolia())] #[tokio::test] async fn test_get_block_number(#[case] provider: SettlementChainProvider) { let result = provider.block_number().await; assert!(result.is_ok()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/settlement.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: build
🔇 Additional comments (3)
bin/katana/src/cli/init/settlement.rs (3)
11-22: Ohayo sensei, unify address definitions.These lines mirror the definition of MAINNET but for SEPOLIA. It might be helpful to centralize the logic for retrieving addresses to avoid duplication if more test networks are added in the future.
26-61: Ohayo sensei, clean and well-documented implementation!The struct definition and its methods are well-documented and follow best practices. The error handling for URL parsing is appropriate.
63-364: Ohayo sensei, excellent trait implementation!The Provider trait implementation is thorough and follows the delegation pattern consistently. The use of generics and trait bounds is appropriate, and error handling is consistent across all methods.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3064 +/- ##
==========================================
- Coverage 57.49% 57.38% -0.11%
==========================================
Files 439 440 +1
Lines 59820 59948 +128
==========================================
+ Hits 34392 34403 +11
- Misses 25428 25545 +117 ☔ View full report in Codecov by Sentry. |
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Need an extra functional check on piltover once splitting proof is fixed.
|
@glihm what check to be specific ? |
Related to the new prover config, if it affects how katana needs to setup piltover. But it shouldn't be, so we can move forward with this and I'll update if necessary. 👍 |
2 participants
support running the
katana initflow with Starknet mainnet as the settlement layer.this PR also bumps
piltoverthat makes V3 transaction as the default (and the only version) for sending transactions.Summary by CodeRabbit
New Features
Refactor
Chores