feat(katana): settlement layer initialization flow improvement #2926
Conversation
kariy
changed the title
WalkthroughOhayo, sensei! This pull request introduces significant changes to the Katana project's dependency management and contract deployment mechanism. The modifications span across multiple files, focusing on enhancing the settlement contract deployment process, adding new dependency management strategies, and improving contract class handling. The changes include updating Changes
Sequence DiagramsequenceDiagram
participant Account
participant DeploymentModule
participant Blockchain
Account->>DeploymentModule: deploy_settlement_contract()
DeploymentModule->>Blockchain: Check contract class declaration
alt Class not declared
DeploymentModule->>Blockchain: Declare contract class
end
DeploymentModule->>Blockchain: Deploy contract
DeploymentModule->>Blockchain: Initialize contract
Blockchain-->>DeploymentModule: Return contract address
DeploymentModule-->>Account: Return contract address
Possibly related PRs
Suggested reviewers
Ohayo and happy coding, sensei! 🍵🥷 🪧 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 (3)
bin/katana/src/cli/init/mod.rs (2)
162-163: Ohayo, sensei! Possible Typo in Comment: 'L1c' instead of 'L1'In the comment, it mentions "The core settlement contract on L1c." Did you mean "L1" instead of "L1c"?
173-174: Ohayo, sensei! Address the TODO: Add validation for settlement contractThere's a TODO at lines 173~ and 174~ to add a check to ensure the provided contract is indeed a valid settlement contract. Implementing this validation will prevent potential misconfigurations.
Would you like me to help implement this check or open a new GitHub issue to track this task?
bin/katana/src/cli/init/deployment.rs (1)
140-141: Ohayo, sensei! Consider parameterizing the fee token addressThe fee token address is currently hardcoded as
0x2e7442625bab778683501c0eadbc1ea17b3535da040a12ac7d281066e915eea. If the fee token may vary between deployments or environments, consider parameterizing this value to enhance flexibility and avoid hardcoding.
📜 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(2 hunks)bin/katana/src/cli/init/deployment.rs(1 hunks)bin/katana/src/cli/init/mod.rs(2 hunks)crates/katana/primitives/src/class.rs(2 hunks)crates/katana/rpc/rpc-types/src/class.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (7)
bin/katana/src/cli/init/deployment.rs (1)
110-114: Ohayo, sensei! Verify initialization of contract's constructor parametersCurrently, the constructor arguments
state_root,block_number, andblock_hashare set toFelt::ZERO. Please verify if these values are appropriate or if they should be initialized with actual blockchain state values to correctly represent the chain's initial state.crates/katana/primitives/src/class.rs (1)
70-95: Ohayo, sensei! ImplementedFromStrforContractClasssuccessfullyThe implementation correctly allows parsing
ContractClassfrom a string, handling both Sierra and Legacy contract classes. Error handling withContractClassFromStrErroris appropriate.crates/katana/rpc/rpc-types/src/class.rs (2)
207-215: Implementation is consistent with Sierra class conversion, sensei!The conversion follows the same pattern as the Sierra class implementation, maintaining consistency throughout the codebase.
187-195: Ohayo! Implementation looks good, sensei!The conversion implementation follows the established pattern in the codebase. While using JSON as an intermediate step isn't the most performant approach, it's acceptable since these conversions are not meant for hot paths.
Let's verify if this conversion is used in any performance-critical paths:
✅ Verification successful
Ohayo! Implementation is safe and sound, sensei!
The conversion from RpcSierraContractClass to FlattenedSierraClass is only used in RPC type conversions and not in any performance-critical paths. The JSON serialization approach is appropriate for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of this conversion in potentially hot paths rg -A 5 "TryFrom<RpcSierraContractClass>\s+for\s+FlattenedSierraClass"Length of output: 605
Script:
#!/bin/bash # Search for try_from/into usage with these types rg -B 2 -A 2 "try_from\(<.*RpcSierraContractClass.*\)|\.into\(\)|FlattenedSierraClass::try_from" # Search for RpcSierraContractClass usage rg "RpcSierraContractClass" --type rust # Search for files containing both types to find potential conversion spots rg -l "RpcSierraContractClass.*FlattenedSierraClass|FlattenedSierraClass.*RpcSierraContractClass"Length of output: 66590
bin/katana/Cargo.toml (3)
14-14: LGTM! katana-rpc-types dependency added correctly, sensei!The addition of katana-rpc-types aligns with the new RPC type conversions implemented in class.rs.
37-37: LGTM! rstest added to dev-dependencies, sensei!The addition of rstest for testing is appropriate.
25-25: Verify the necessity of new dependencies, sensei!The addition of
randandthiserrordependencies:
thiserroris justified for error handlingrandseems unrelated to the PR's objective of computing config hash correctlyLet's check if
randis used in the changes:Also applies to: 30-30
✅ Verification successful
Ohayo! The
randdependency is justified, sensei!The PR uses
randto generate a salt value for the config hash computation, which is a standard security practice to ensure hash uniqueness and prevent collisions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for rand usage in the codebase rg -l "use.*rand::"Length of output: 518
Script:
#!/bin/bash # Check PR changes for rand usage git diff HEAD~1 | rg -i "rand"Length of output: 124
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2926 +/- ##
==========================================
- Coverage 55.99% 55.81% -0.19%
==========================================
Files 449 444 -5
Lines 58020 57308 -712
==========================================
- Hits 32491 31988 -503
+ Misses 25529 25320 -209 ☔ View full report in Codecov by Sentry. |
Currently we're hardcoding the program's config hash, which is terribly wrong as it needs to actually be computed according to the chain's configuration itself.
This PR also includes assertion checks at the end of deployment to make sure the contract has actually been initialized with the intended value (ie program hash, config hash, and fact registry address).