refactor(katana): move messaging into its own crate #3013
Conversation
|
Ohayo, sensei! WalkthroughThis pull request expands the workspace by adding a new messaging package and updating several Cargo.toml files to include or adjust dependencies and feature flags. It refines the settlement contract deployment process by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant CLI as Init Module
participant Deploy as deploy_settlement_contract
participant Receipt as Receipt Service
participant Block as BlockNumber Service
User->>CLI: Initiate settlement contract deployment
CLI->>Deploy: Call deploy_settlement_contract()
Deploy->>Receipt: Request deployment receipt concurrently
Deploy->>Block: Query current block number via tokio::join!
Receipt-->>Deploy: Return contract address & block (if available)
Block-->>Deploy: Return current block number
Deploy->>CLI: Return DeploymentOutcome { contract_address, block_number }
sequenceDiagram
participant App as Node/Application
participant MS as MessagingService
participant Prov as Provider (BlockNumber & Receipt)
App->>MS: new(config, chain_spec, pool, provider)
MS->>Prov: Fetch block number and receipts
Prov-->>MS: Return latest block data and receipts
MS-->>App: MessagingService initialized and ready
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: 0
🧹 Nitpick comments (5)
crates/katana/messaging/src/service.rs (2)
18-18: Ohayo sensei, consider removing commented code.
Leaving commented-out imports can clutter the code and confuse future maintainers.
132-137: Ohayo sensei, robust approach to reading receipts by block.
However, usingunwrap()could lead to panics ifproviderfails; consider a more graceful error handling.crates/katana/sync/stage/src/sequencing.rs (1)
10-10: Ohayo! Nice architectural improvement, sensei!The refactoring to use
chain_specandproviderdirectly improves dependency injection and makes the messaging service more modular. The updated import path also reflects better code organization.Also applies to: 42-45
bin/katana/src/cli/init/prompt.rs (1)
120-124: Add block number validation, sensei!While the prompt for block number is well-documented, consider adding validation to ensure:
- The block number is not in the future
- The contract exists at the specified block
let block_number = CustomType::<BlockNumber>::new("Settlement contract deployment block") .with_help_message("The block at which the settlement contract was deployed") + .with_validator(|input: &BlockNumber| { + // Fetch latest block number + let latest = l1_provider.block_number().await?; + if *input > latest { + return Err("Block number cannot be in the future".into()); + } + // Verify contract exists at this block + let _ = l1_provider.get_class_hash_at( + BlockId::Number(*input), + address.into() + ).await?; + Ok(()) + }) .prompt()?;crates/katana/messaging/src/lib.rs (1)
130-163: Ohayo! Nice implementation of from_chain_spec, sensei!The method handles both Ethereum and Starknet settlement layers cleanly. However, consider extracting the magic number
2for the interval into a named constant for better maintainability.+const DEFAULT_MESSAGING_INTERVAL: u64 = 2; pub fn from_chain_spec(spec: &katana_chain_spec::rollup::ChainSpec) -> Self { match &spec.settlement { katana_chain_spec::SettlementLayer::Ethereum { rpc_url, core_contract, account, block, .. } => Self { chain: CONFIG_CHAIN_ETHEREUM.to_string(), rpc_url: rpc_url.to_string(), contract_address: core_contract.to_string(), sender_address: account.to_string(), private_key: Felt::ZERO.to_string(), from_block: *block, - interval: 2, + interval: DEFAULT_MESSAGING_INTERVAL, },
📜 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 (24)
Cargo.toml(3 hunks)bin/katana/src/cli/init/deployment.rs(4 hunks)bin/katana/src/cli/init/mod.rs(6 hunks)bin/katana/src/cli/init/prompt.rs(4 hunks)crates/katana/chain-spec/Cargo.toml(1 hunks)crates/katana/chain-spec/src/lib.rs(3 hunks)crates/katana/chain-spec/src/rollup/file.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(5 hunks)crates/katana/cli/src/file.rs(1 hunks)crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/service/mod.rs(0 hunks)crates/katana/core/tests/backend.rs(1 hunks)crates/katana/messaging/Cargo.toml(1 hunks)crates/katana/messaging/src/lib.rs(4 hunks)crates/katana/messaging/src/service.rs(10 hunks)crates/katana/node/Cargo.toml(1 hunks)crates/katana/node/src/config/mod.rs(1 hunks)crates/katana/rpc/rpc/Cargo.toml(1 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/storage/provider/Cargo.toml(1 hunks)crates/katana/sync/stage/Cargo.toml(1 hunks)crates/katana/sync/stage/src/sequencing.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/src/service/mod.rs
✅ Files skipped from review due to trivial changes (3)
- crates/katana/chain-spec/Cargo.toml
- crates/katana/rpc/rpc/tests/messaging.rs
- crates/katana/messaging/Cargo.toml
🔇 Additional comments (33)
crates/katana/messaging/src/service.rs (10)
7-9: Ohayo sensei, these new imports align perfectly with the new provider approach.
They properly reflect the shift from a backend-based approach to a flexible provider-based interface. Great job!
25-28: Ohayo sensei, nice approach to generics!
EnsuringPimplementsBlockNumberProvider,ReceiptProvider, andClonefosters a clean architecture.
31-32: Ohayo sensei, these new fields look good.
Storing the provider and chain_spec is essential for the messaging logic.
46-49: Ohayo sensei, consistent approach to implementing the service with generics.
This is a straightforward reflection of the struct's constraints.
54-56: Ohayo sensei, verifyingchain_specandproviderin the constructor.
This helps ensure the service is properly decoupled from a specific backend. Great job!
72-75: Ohayo sensei, correct usage ofproviderandchain_specin struct construction.
Everything is consistent and properly assigned.
83-86: Ohayo sensei, consistent usage ofchain_idin gathering messages.
This clarifies which chain is used to fetch receipts.
96-113: Ohayo sensei, multi-chain support is well-handled here.
Passingchain_idto gather messages in different modes is thoughtful and extends flexibility.
181-184: Ohayo sensei, theStreamimplementation is neatly structured.
The generics ensure that both block numbers and receipts integrate seamlessly.
196-205: Ohayo sensei, great polling logic.
Still, consider safely handlingprovider.latest_number()errors instead of relying onunwrap().crates/katana/node/src/config/mod.rs (1)
16-16: Ohayo sensei, nice refactoring of theMessagingConfigimport.
Switching tokatana_messagingensures consistent architecture across the codebase.crates/katana/cli/src/file.rs (1)
4-4: Ohayo sensei, consistentMessagingConfigimport path update.
This harmonizes well with the new messaging module changes.crates/katana/chain-spec/src/lib.rs (1)
84-85: Ohayo! Clean addition of block tracking, sensei!The addition of the
blockfield to track contract deployment blocks is well-structured and consistently implemented across both Ethereum and Starknet variants.Also applies to: 101-102
crates/katana/core/tests/backend.rs (1)
61-67: Consider validating the default block value, sensei!While initializing
block: 0works for testing, consider adding test cases that verify behavior with non-zero block numbers to ensure the system handles real deployment scenarios correctly.bin/katana/src/cli/init/mod.rs (3)
46-52: Ohayo! Nice addition of block number tracking, sensei!The new
settlement_contract_deployed_blockfield with its corresponding requirements ensures that we always have the block number when a settlement contract is provided. This improves data consistency and traceability.
115-126: Clean implementation of DeploymentOutcome, sensei!Good job encapsulating both the contract address and block number in a single structure. The code properly handles the block number extraction from the arguments.
73-74: Excellent propagation of block information, sensei!The block number is correctly propagated to the settlement layer, maintaining consistency with the new deployment outcome structure.
crates/katana/messaging/src/lib.rs (1)
247-250: Great trait abstraction, sensei!The change from a concrete ExecutorFactory to trait bounds (BlockNumberProvider + ReceiptProvider) improves flexibility and follows good design principles.
crates/katana/chain-spec/src/rollup/file.rs (1)
338-338: Good test coverage maintenance, sensei!The test has been properly updated to include the new block field initialization, maintaining test coverage.
bin/katana/src/cli/init/deployment.rs (3)
74-80: Clean struct definition with good documentation, sensei!The DeploymentOutcome struct is well-designed and documented, clearly capturing both the contract address and deployment block number.
174-192: Excellent concurrent block number handling, sensei!Smart use of
tokio::join!to minimize timing discrepancies, with proper handling of the pending block case. The code effectively prevents race conditions.
265-268: Clear and informative success message, sensei!The success message now includes both the contract address and block number, providing complete deployment information.
crates/katana/chain-spec/src/rollup/utils.rs (1)
370-370: Ohayo! The block field initialization looks good, sensei!Initializing the block field to 0 for new settlement layer deployments is a sensible default.
crates/katana/cli/src/args.rs (3)
14-14: Ohayo! Clean import update, sensei!Moving the messaging config to its own crate improves modularity.
83-85: Nice CLI argument constraints, sensei!The
conflicts_with = "chain"attribute prevents confusing configurations by ensuring messaging config comes from either CLI or chain spec, but not both.
189-194: Good handling of messaging config precedence, sensei!The logic correctly prioritizes messaging config from chain spec over CLI arguments, with clear comments explaining the temporary nature of this approach.
crates/katana/sync/stage/Cargo.toml (1)
12-12: Ohayo! Clean dependency addition, sensei!Adding katana-messaging as a workspace dependency aligns with the messaging module refactoring.
crates/katana/cli/Cargo.toml (1)
11-11: Ohayo! Clean dependency addition, sensei!Adding katana-messaging as a workspace dependency aligns with the messaging module refactoring.
crates/katana/storage/provider/Cargo.toml (1)
18-21: Ohayo sensei! Dependencies reorganization looks good!The dependencies have been properly moved from feature-gated to direct dependencies, making the dependency tree more explicit and easier to maintain.
Also applies to: 33-33
crates/katana/core/Cargo.toml (1)
14-15: Ohayo sensei! Feature flags are properly configured!The katana-provider dependency now correctly specifies required features, and development dependencies have been appropriately added.
Also applies to: 39-44
crates/katana/rpc/rpc/Cargo.toml (1)
38-38: Ohayo sensei! Dev-dependency addition is well-placed!Adding katana-messaging as a dev-dependency is appropriate for testing the RPC layer's interaction with messaging functionality.
crates/katana/node/Cargo.toml (1)
13-13: Ohayo sensei! Core dependency addition is spot on!Adding katana-messaging as a direct dependency is essential for the node to handle messaging functionality.
Cargo.toml (1)
24-24: Ohayo sensei! Workspace configuration is well-structured!The changes properly:
- Establish the new messaging crate in the workspace
- Configure dependencies with appropriate feature flags
- Reintroduce necessary torii components
Also applies to: 102-102, 108-110, 124-128
kariy
force-pushed
the
katana/messaging-move
branch
from
d9d515a to
0ea3a76
Compare
kariy
force-pushed
the
katana/messaging-move
branch
from
0ea3a76 to
f24f437
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/katana/cli/src/args.rs (1)
190-193: Consider documenting the deprecation timeline.While the comment explains that this is temporary, it would be helpful to document when this messaging configuration will be removed.
Add a TODO comment with the expected deprecation timeline:
-// the `katana init` will automatically generate a messaging config. so if katana is run -// with `--chain` then the `--messaging` flag is not required. this is temporary and -// the messagign config will eventually be removed slowly. +// TODO(messaging): The messaging configuration will be deprecated and removed in future versions. +// Timeline for deprecation: +// 1. Mark as deprecated in v0.X +// 2. Remove in v0.Y +// +// For now, katana init automatically generates a messaging config, so the --messaging flag +// is not required when using --chain.
🛑 Comments failed to post (1)
crates/katana/messaging/src/service.rs (1)
135-139: 🛠️ Refactor suggestion
Consider enhancing error handling.
The unwrap call on line 135 could be replaced with proper error handling:
- let Some(messages) = provider.receipts_by_block(block_num.into()).unwrap().map(|r| { + let Some(messages) = provider.receipts_by_block(block_num.into()) + .map_err(|e| Error::Provider(e.into()))? + .map(|r| {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let Some(messages) = provider.receipts_by_block(block_num.into()) .map_err(|e| Error::Provider(e.into()))? .map(|r| { r.iter().flat_map(|r| r.messages_sent().to_vec()).collect::<Vec<MessageToL1>>() }) else { return Ok(None); };
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/messaging/src/lib.rs (1)
247-250: Excellent architectural improvement, sensei!The shift from
ExecutorFactoryto specific provider traits (BlockNumberProvider + ReceiptProvider) improves:
- Modularity: Components can implement just the traits they need
- Testability: Easier to mock specific provider behaviors
- Flexibility: More implementation options for consumers
crates/katana/cli/src/args.rs (1)
190-193: Consider enhancing the comment clarity.While the comment explains the current behavior, it could be more explicit about:
- The timeline for removing messaging config
- The migration path for users
-// the `katana init` will automatically generate a messaging config. so if katana is run -// with `--chain` then the `--messaging` flag is not required. this is temporary and -// the messagign config will eventually be removed slowly. +// TODO(messaging): The messaging config is being deprecated in favor of chain-spec-based configuration. +// Currently, if katana is run with `--chain`, the `--messaging` flag is not required as the config +// is automatically generated. This dual-configuration support is temporary and will be removed in +// future versions.
📜 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 (24)
Cargo.toml(3 hunks)bin/katana/src/cli/init/deployment.rs(4 hunks)bin/katana/src/cli/init/mod.rs(6 hunks)bin/katana/src/cli/init/prompt.rs(4 hunks)crates/katana/chain-spec/Cargo.toml(1 hunks)crates/katana/chain-spec/src/lib.rs(3 hunks)crates/katana/chain-spec/src/rollup/file.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(5 hunks)crates/katana/cli/src/file.rs(1 hunks)crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/service/mod.rs(0 hunks)crates/katana/core/tests/backend.rs(1 hunks)crates/katana/messaging/Cargo.toml(1 hunks)crates/katana/messaging/src/lib.rs(4 hunks)crates/katana/messaging/src/service.rs(10 hunks)crates/katana/node/Cargo.toml(1 hunks)crates/katana/node/src/config/mod.rs(1 hunks)crates/katana/rpc/rpc/Cargo.toml(1 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/storage/provider/Cargo.toml(1 hunks)crates/katana/sync/stage/Cargo.toml(1 hunks)crates/katana/sync/stage/src/sequencing.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/src/service/mod.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- crates/katana/sync/stage/Cargo.toml
- crates/katana/cli/Cargo.toml
- crates/katana/node/src/config/mod.rs
- crates/katana/chain-spec/Cargo.toml
- crates/katana/chain-spec/src/rollup/utils.rs
- crates/katana/node/Cargo.toml
- crates/katana/core/tests/backend.rs
- crates/katana/cli/src/file.rs
- crates/katana/rpc/rpc/tests/messaging.rs
- bin/katana/src/cli/init/prompt.rs
- crates/katana/chain-spec/src/rollup/file.rs
- crates/katana/messaging/Cargo.toml
- crates/katana/chain-spec/src/lib.rs
- crates/katana/rpc/rpc/Cargo.toml
- crates/katana/storage/provider/Cargo.toml
- crates/katana/sync/stage/src/sequencing.rs
- crates/katana/core/Cargo.toml
- Cargo.toml
- crates/katana/messaging/src/service.rs
- bin/katana/src/cli/init/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (7)
crates/katana/messaging/src/lib.rs (2)
1-1: Ohayo! Clean dependency management and provider traits.The crate attribute helps catch unused dependencies, and the new imports support the shift to a provider-based interface.
Also applies to: 54-57
130-163: Verify the security implications of using a zero private key.The
from_chain_specmethod usesFelt::ZEROas the private key, which could be a security risk in production environments.Consider:
- Is this intended only for development/testing?
- Should there be a warning or assertion to prevent accidental use in production?
crates/katana/cli/src/args.rs (2)
83-84: Ohayo! Good conflict prevention.The
conflicts_with = "chain"attribute prevents confusion by ensuring users don't try to use both chain and messaging options simultaneously.
250-256: Nice UX improvement, sensei!The method now automatically generates messaging config from chain specs when available, reducing manual configuration needs.
bin/katana/src/cli/init/deployment.rs (3)
74-80: Ohayo! Clean and well-documented struct definition, sensei!The
DeploymentOutcomestruct is well-designed with clear field documentation.
264-268: Nice success message update, sensei!The updated success message now provides comprehensive deployment information including both the contract address and block number.
174-192: Ohayo sensei! Let's verify the block number calculation for edge cases.While the concurrent fetching of deployment receipt and block number is a good improvement, there might be edge cases where the block number could be off by more than 1 if there are multiple pending transactions.
Could you verify if this edge case needs handling by checking if there are any tests or documentation addressing this scenario? You might want to search for:
✅ Verification successful
🌐 Web query:
How to handle multiple pending transactions when determining block numbers in Starknet?💡 Result:
To handle multiple pending transactions when determining block numbers in Starknet:
Transaction Status & Block Association
- Pending transactions exist in Starknet's mempool but haven't been finalized in a closed block[1][10]
- They belong to either:
- The pending block (block under active construction)
- The transaction pool (awaiting inclusion)[4]
Key Implementation Strategies
Block Number Queries
UseblockNumber=pendingparameter in API calls to:account.get_nonce(block_number="pending") # From starknet.py[3]This reflects state changes from all pending transactions[1]
Transaction Ordering
Pending transactions don't have final ordering until:
- Block reaches
ACCEPTED_ON_L2status[1][5]- Block hash gets assigned[2]
Nonce Management
Handle potential conflicts using:async def handle_pending_txs(): current_nonce = await account.get_nonce(block_number="pending") for i, tx in enumerate(pending_transactions): tx.nonce = current_nonce + iRequires monitoring mempool for nonce gaps[7]
Block Construction Analysis
Query current pending block details:SELECT hash, transaction_count FROM starknet.blocks WHERE status = 'PENDING' # Example Dune Analytics query[8]Critical Considerations
- Transaction Replacement: Higher fee transactions can replace stuck ones with same nonce[7]
- State Diff Tracking: Use
state_diff_commitmentfield to monitor pending changes[2][10]- Sequencer Behavior: Blocks transition from
PENDING→ACCEPTED_ON_L2→ACCEPTED_ON_L1[5][10]Best practice is to treat pending blocks as provisional until reaching
ACCEPTED_ON_L2status, using Starknet's specific pending-block APIs rather than assuming L1-style finality[1][3][5]. Transactions in the same pending block share temporal sequencing but lack canonical ordering until block closure[2][10].Citations:
- 1: https://community.starknet.io/t/cairo-v0-6-2-api-change-pending-block/195
- 2: https://docs.starknet.io/architecture-and-concepts/network-architecture/block-structure/
- 3: https://starknetpy.readthedocs.io/en/latest/api/account.html
- 4: https://docs.blastapi.io/blast-documentation/apis-documentation/core-api/starknet/starknet_pendingtransactions
- 5: https://www.quicknode.com/docs/starknet/starknet_getBlockWithReceipts
- 6: https://github.com/keep-starknet-strange/madara/blob/main/crates/client/rpc/src/lib.rs
- 7: https://forum.stacks.org/t/pending-transaction-more-than-10-hours/16596
- 8: https://dev.to/lordghostx/data-to-queries-exploring-starknet-data-with-dune-ihd
- 9: https://docs.chainstack.com/docs/ethereum-how-to-analyze-pending-blocks
- 10: https://docs.starknet.io/architecture-and-concepts/network-architecture/transaction-life-cycle/
Ohayo sensei! After checking the latest documentation on how Starknet handles pending transactions, it turns out that pending transactions are grouped into a single pending block—meaning that regardless of how many transactions are waiting, the pending block’s number is still just the latest block number plus one. This aligns with your current logic, which defaults to using
block_number + 1when the receipt doesn't include a block number.No additional edge-case handling appears necessary based on standard practices for Starknet. However, it's always a good idea to ensure your tests cover scenarios where the receipt is missing the block number.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3013 +/- ##
==========================================
+ Coverage 56.38% 56.40% +0.01%
==========================================
Files 434 434
Lines 58075 58077 +2
==========================================
+ Hits 32748 32759 +11
+ Misses 25327 25318 -9 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
katana-messagingadded, enhancing messaging functionalities across components.Refactor