fix(katana-contracts): update piltover #2980
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
==========================================
+ Coverage 57.02% 57.06% +0.03%
==========================================
Files 429 429
Lines 56939 56925 -14
==========================================
+ Hits 32470 32484 +14
+ Misses 24469 24441 -28 ☔ View full report in Codecov by Sentry. |
kariy
force-pushed
the
katana/piltover-update-deser
branch
from
50f0094 to
3a4ffb3
Compare
kariy
changed the title
katana init
kariy
force-pushed
the
katana/piltover-update-deser
branch
from
ae75910 to
9152f2c
Compare
|
Ohayo, sensei! Below is the updated summary of the PR changes: WalkthroughThis PR updates the Piltover dependency configuration and enhances deployment logic. The submodule URL for Piltover has been changed, a new Piltover dependency has been added, and the cainome-cairo-serde dependency has been upgraded. Additionally, deployment initialization now includes new program hash constants with improved validation and error handling. The build process is refined through adjustments in the Makefile, and the RPC server’s configuration now supports larger request and response body sizes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Sensei
participant Deploy as Deployment Module
participant RPC as RpcProvider
CLI->>Deploy: Initiate Deployment
Deploy->>RPC: Retrieve Program Info (Hashes)
RPC-->>Deploy: Return Program Hashes
Deploy->>Deploy: Validate against SNOS and LAYOUT constants
alt Validation Success
Deploy-->>CLI: Confirm Deployment Success
else Validation Failure
Deploy-->>CLI: Return InvalidSnosProgramHash error
end
Possibly related PRs
🪧 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 (1)
crates/katana/node/src/config/rpc.rs (1)
43-45: Add documentation for the new configuration fields.Ohayo sensei! Consider adding doc comments for these new fields to explain:
- The purpose and impact of each field
- The units used (bytes, KB, MB?)
- The behavior when set to
NoneApply this diff to add documentation:
+ /// Maximum number of concurrent connections allowed. + /// When set to None, no limit is enforced. pub max_connections: Option<u32>, + /// Maximum size of a request body in bytes. + /// When set to None, no limit is enforced. pub max_request_body_size: Option<u32>, + /// Maximum size of a response body in bytes. + /// When set to None, no limit is enforced. pub max_response_body_size: Option<u32>,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/dojo/test-utils/src/sequencer.rs(2 hunks)crates/katana/cli/src/args.rs(1 hunks)crates/katana/cli/src/options.rs(3 hunks)crates/katana/node/src/config/rpc.rs(2 hunks)crates/katana/rpc/rpc/src/lib.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/rpc/rpc/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
🔇 Additional comments (2)
crates/dojo/test-utils/src/sequencer.rs (1)
129-129: LGTM!Ohayo sensei! The change to use
..Default::default()aligns well with the updatedRpcConfigstructure wheremax_connectionsis now optional.crates/katana/cli/src/options.rs (1)
109-118: LGTM!Ohayo sensei! The CLI argument definitions for the new configuration options are well-documented and properly integrated into the
ServerOptionsstruct.
| max_request_body_size: None, | ||
| max_response_body_size: None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CLI arguments for request/response body sizes.
Ohayo sensei! The new body size fields are hardcoded to None instead of using the values from self.server. Consider using the CLI arguments:
- max_request_body_size: None,
- max_response_body_size: None,
+ max_request_body_size: self.server.max_request_body_size,
+ max_response_body_size: self.server.max_response_body_size,📝 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.
| max_request_body_size: None, | |
| max_response_body_size: None, | |
| max_request_body_size: self.server.max_request_body_size, | |
| max_response_body_size: self.server.max_response_body_size, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/cli/src/options.rs (1)
108-119: Ohayo! The server configuration enhancements look great, sensei!The changes provide more flexibility in server configuration by:
- Making
max_connectionsoptional- Adding configurable request and response body size limits
These improvements will help prevent resource exhaustion and provide better control over server behavior.
Consider adding validation to ensure that when body size limits are set, they are reasonable values (e.g., not too small to handle typical requests or too large to cause memory issues).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/cli/src/options.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: ensure-wasm
🔇 Additional comments (1)
crates/katana/cli/src/options.rs (1)
143-145: The default implementation is clean and idiomatic, sensei!Setting the new fields to
Noneby default is a good choice, allowing the server to use its internal defaults when no explicit values are provided.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/katana/src/cli/init/deployment.rs (1)
323-327: Consider updating the NOTE comment, sensei.The comment about using the same address for both fee tokens could be more descriptive about when this limitation will be addressed.
Consider updating the comment:
-// NOTE: The reason why we're using the same address for both fee tokens is because we don't yet -// support having native fee token on the chain. +// NOTE: Using the same address for both fee tokens temporarily. +// TODO: Update when native fee token support is implemented (tracked in issue #XXXX)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/deployment.rs(7 hunks)
🔇 Additional comments (3)
bin/katana/src/cli/init/deployment.rs (3)
104-106: Nice optimization for genesis block, sensei!Using
Felt::MAXfor the genesis block number is a good practice as it helps distinguish the genesis block from regular blocks.
142-146: Clean implementation of ProgramInfo, sensei!The ProgramInfo structure provides a clear and organized way to manage program-related information.
230-235: Solid validation for SNOS program hash, sensei!The addition of SNOS program hash validation enhances the security and reliability of the deployment process.
| // TODO: | ||
| // | ||
| // This is the default fee token contract address. We shouldn't hardcode | ||
| // this and need to handle this more elegantly. | ||
| felt!("0x2e7442625bab778683501c0eadbc1ea17b3535da040a12ac7d281066e915eea"), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Another TODO that needs attention, sensei.
The hardcoded fee token address should be handled more elegantly as noted in the TODO comment. Consider making it configurable.
Consider adding a configuration parameter for the fee token address:
- felt!("0x2e7442625bab778683501c0eadbc1ea17b3535da040a12ac7d281066e915eea"),
+ config.fee_token_address,📝 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.
| // TODO: | |
| // | |
| // This is the default fee token contract address. We shouldn't hardcode | |
| // this and need to handle this more elegantly. | |
| felt!("0x2e7442625bab778683501c0eadbc1ea17b3535da040a12ac7d281066e915eea"), | |
| // TODO: | |
| // | |
| // This is the default fee token contract address. We shouldn't hardcode | |
| // this and need to handle this more elegantly. | |
| config.fee_token_address, |
| // TODO: to replace with actual value. | ||
| const LAYOUT_BRIDGE_PROGRAM_HASH: Felt = | ||
| felt!("0x193641eb151b0f41674641089952e60bc3aded26e3cf42793655c562b8c3aa0"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ohayo! TODO comment needs attention, sensei.
The TODO comment indicates that LAYOUT_BRIDGE_PROGRAM_HASH needs to be replaced with the actual value. This could lead to runtime issues if not addressed.
Let me help verify if this hash is used in production:
🏁 Script executed:
#!/bin/bash
# Search for references to this hash in the codebase
rg -l "0x193641eb151b0f41674641089952e60bc3aded26e3cf42793655c562b8c3aa0"Length of output: 111
Ohayo, sensei! The shell script confirms that the hardcoded hash is only referenced in bin/katana/src/cli/init/deployment.rs. Since the constant is marked with a TODO and isn’t used elsewhere, it’s clear that the placeholder value is intended to be updated prior to production—otherwise, you risk potential runtime issues when this value is relied on. Please update LAYOUT_BRIDGE_PROGRAM_HASH with the correct production value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/katana/src/cli/init/deployment.rs (1)
227-269: Ohayo! Comprehensive validation logic, sensei!The validation checks for program hashes are thorough and well-structured. However, consider extracting the fee token address validation into a separate function for better reusability.
+fn validate_fee_token(actual: Felt, expected: Felt) -> Result<(), ContractInitError> { + if actual != expected { + return Err(ContractInitError::InvalidFeeToken { + actual, + expected, + }); + } + Ok(()) +} + pub async fn check_program_info( chain_id: Felt, appchain_address: Felt, provider: &RpcProvider, ) -> Result<(), ContractInitError> { // ... existing code ... + validate_fee_token(actual_fee_token, expected_fee_token)?; // ... rest of the code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/deployment.rs(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
bin/katana/src/cli/init/deployment.rs
[warning] 39-39: Comments have been modified for clarity. Please ensure that the changes maintain the intended meaning.
🔇 Additional comments (5)
bin/katana/src/cli/init/deployment.rs (5)
30-52: Ohayo! Documentation looks good, sensei!The documentation for both program hashes is thorough and includes helpful links to the source code.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 39-39: Comments have been modified for clarity. Please ensure that the changes maintain the intended meaning.
115-126: Ohayo! Good use of constants and documentation, sensei!The use of constants with clear documentation improves code maintainability. The explanation for
INITIAL_BLOCK_NUMBERbeingFelt::MAXis particularly helpful.
164-169: Ohayo! TODO needs attention, sensei.The hardcoded fee token address should be handled more elegantly as noted in the TODO comment. Consider making it configurable.
176-184: Ohayo! Good refactoring with ProgramInfo, sensei!The use of the
ProgramInfostruct improves code organization and type safety.
305-310: Ohayo! Good error handling addition, sensei!The new error variant
InvalidSnosProgramHashfollows the existing error pattern and provides clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/katana/src/cli/init/deployment.rs (1)
261-273: Enhance error handling with more context.Consider adding more context to error messages by including the chain ID in the error variants.
- if actual_program_info.program_hash != LAYOUT_BRIDGE_PROGRAM_HASH { - return Err(ContractInitError::InvalidProgramHash { - actual: actual_program_info.program_hash, - expected: LAYOUT_BRIDGE_PROGRAM_HASH, - }); - } + if actual_program_info.program_hash != LAYOUT_BRIDGE_PROGRAM_HASH { + return Err(ContractInitError::InvalidProgramHash { + chain_id, + actual: actual_program_info.program_hash, + expected: LAYOUT_BRIDGE_PROGRAM_HASH, + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/katana/src/cli/init/deployment.rs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (5)
bin/katana/src/cli/init/deployment.rs (5)
36-37: LGTM! Well-documented constant with source reference.The SNOS program hash constant is well-documented with a clear explanation and source link.
119-129: LGTM! Well-documented initial values with clear explanations.The initial values for state root, block number, and block hash are well-documented, especially the explanation for using
Felt::MAXas the initial block number with a reference to the source code.
168-173: Ohayo! Another TODO that needs attention, sensei.The hardcoded fee token address should be handled more elegantly as noted in the TODO comment. Consider making it configurable.
180-188: LGTM! Clean program info initialization.The program info initialization is clean and well-structured using the new
ProgramInfostruct.
309-314: LGTM! Clear error variant for SNOS program hash validation.The new error variant for SNOS program hash validation is well-structured and follows the existing error pattern.
| const LAYOUT_BRIDGE_PROGRAM_HASH: Felt = | ||
| felt!("0x193641eb151b0f41674641089952e60bc3aded26e3cf42793655c562b8c3aa0"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the Layout Bridge program hash value.
The hardcoded hash appears to be truncated (missing a digit) compared to typical Starknet hashes.
🏁 Script executed:
#!/bin/bash
# Verify the hash length
echo "0x193641eb151b0f41674641089952e60bc3aded26e3cf42793655c562b8c3aa0" | wc -cLength of output: 83
Ohayo sensei, based on our check:
- The hardcoded hash "0x193641eb151b0f41674641089952e60bc3aded26e3cf42793655c562b8c3aa0" prints a total of 65 characters (excluding the newline), indicating it’s missing one digit compared to the usual Starknet hash format (which should be "0x" plus 64 hex digits, or 66 characters total).
Please update the constant value with the full hash.
@kariy this PR contains the contract class built on your PR cartridge-gg/piltover#5 rebased on main.
Also added katana init --list and katana init --show-config <chain_id> which I found useful for dev purposes. At your consideration if they are worth being kept.Feature added in #2983 .Summary by CodeRabbit
New Features
Chores