fix(katana): include salt in genesis accounts #3031
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3031 +/- ##
==========================================
- Coverage 56.33% 56.33% -0.01%
==========================================
Files 437 437
Lines 58891 58997 +106
==========================================
+ Hits 33179 33235 +56
- Misses 25712 25762 +50 ☔ View full report in Codecov by Sentry. |
|
Ohayo, sensei! Below is the updated summary of the changes: WalkthroughThis pull request refactors the account creation and deployment processes across multiple modules. The changes separate account instantiation from address retrieval, introduce a dynamic salt parameter, and add a new salt field to various genesis account structures. Method signatures for account creation, deployment, and JSON conversion have been simplified and updated to reflect these enhancements. Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Nitpick comments (1)
crates/katana/primitives/src/genesis/allocation.rs (1)
199-200: Consider adding more context to the DEFAULT_SALT documentation sensei.While the comment mentions "backward compatible reason", it would be helpful to explain why this specific value was chosen.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bin/katana/src/cli/init/slot.rs(1 hunks)crates/katana/chain-spec/src/dev.rs(2 hunks)crates/katana/chain-spec/src/rollup/utils.rs(8 hunks)crates/katana/primitives/src/genesis/allocation.rs(4 hunks)crates/katana/primitives/src/genesis/json.rs(10 hunks)crates/katana/storage/provider/src/test_utils.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
crates/katana/chain-spec/src/rollup/utils.rs
[warning] 227-229: crates/katana/chain-spec/src/rollup/utils.rs#L227-L229
Added lines #L227 - L229 were not covered by tests
[warning] 307-308: crates/katana/chain-spec/src/rollup/utils.rs#L307-L308
Added lines #L307 - L308 were not covered by tests
crates/katana/primitives/src/genesis/allocation.rs
[warning] 210-213: crates/katana/primitives/src/genesis/allocation.rs#L210-L213
Added lines #L210 - L213 were not covered by tests
🔇 Additional comments (13)
crates/katana/storage/provider/src/test_utils.rs (1)
56-56: Ohayo! LGTM sensei!The code has been updated to use the simplified API for account creation, which aligns well with the new design.
bin/katana/src/cli/init/slot.rs (1)
74-82: Ohayo! LGTM sensei!The code has been updated to use the simplified API for account creation, separating account creation from address retrieval, which improves code clarity.
crates/katana/primitives/src/genesis/allocation.rs (2)
194-195: Ohayo! Nice addition of the salt field sensei!The salt field is properly documented and will help in deterministic account deployment.
227-230: Ohayo! Clean implementation of address calculation sensei!The address calculation is properly implemented using Starknet's
get_contract_addressfunction.crates/katana/chain-spec/src/rollup/utils.rs (2)
132-151: Ohayo! Nice refactor of the deploy method sensei!The addition of the salt parameter improves the flexibility of contract deployment.
303-316:❓ Verification inconclusive
Please add test coverage for GenesisAccountAlloc::Account handling sensei.
The static analysis indicates that the Account variant handling is not covered by tests.
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for Account variant handling rg -A 5 'GenesisAccountAlloc::Account.*deploy_predeployed_account' -g '*test*.rs'Length of output: 83
Ohayo sensei – Please add tests for the Account variant handling
It looks like our initial check for the Account variant in the test files didn’t return any results. Could you please verify manually (or run additional checks) that tests covering the
GenesisAccountAlloc::Accountbranch incrates/katana/chain-spec/src/rollup/utils.rs(lines 303-316) are indeed missing? If they are absent, please add the appropriate tests to cover its behavior, ensuring proper verification of both deployment and balance transfer logic.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 307-308: crates/katana/chain-spec/src/rollup/utils.rs#L307-L308
Added lines #L307 - L308 were not covered by testscrates/katana/chain-spec/src/dev.rs (3)
310-310: Ohayo! Test updates look good, sensei!The test has been properly updated to include the new
saltfield with the default value.Also applies to: 333-333
793-793: Test data structures are properly updated, sensei!All test cases have been consistently updated to include the
saltfield with the default value.Also applies to: 804-804, 815-815, 828-828
959-959: Test assertions are properly updated, sensei!The test case correctly verifies the default salt value in the assertions.
crates/katana/primitives/src/genesis/json.rs (4)
159-159: Ohayo! Struct definition looks good, sensei!The
saltfield is properly defined as an optional field with the correct type.
379-413: Account creation logic is well implemented, sensei!The code elegantly handles both cases with and without salt, maintaining consistency across account types.
494-494: JSON conversion is properly updated, sensei!The salt field is correctly included in the JSON conversion logic.
Also applies to: 506-506
1020-1020: Test data is properly updated, sensei!The test case correctly includes the new salt field and verifies its optional nature.
| pub fn new_with_balance(public_key: Felt, class_hash: ClassHash, balance: U256) -> Self { | ||
| let mut account = Self::new(public_key, class_hash); | ||
| account.balance = Some(balance); | ||
| account | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Ohayo! Please add test coverage for new_with_balance sensei.
The static analysis indicates that this method is not covered by tests.
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for new_with_balance method
rg -A 5 'new_with_balance.*balance: U256' -g '*test*.rs'Length of output: 58
Ohayo sensei! It looks like the new new_with_balance method (at crates/katana/primitives/src/genesis/allocation.rs, lines 210–214) isn’t covered by any tests yet. The grep search didn’t return any references in our test files, so please add proper test coverage to verify that:
- A new instance correctly sets the balance.
- The method integrates well with the rest of the account creation API.
Once tests are added, we can be confident that the functionality behaves as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-213: crates/katana/primitives/src/genesis/allocation.rs#L210-L213
Added lines #L210 - L213 were not covered by tests
| fn deploy_predeployed_account(&self, salt: Felt, public_key: Felt) -> ContractAddress { | ||
| self.deploy(DEFAULT_ACCOUNT_CLASS_HASH, vec![public_key], salt) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Please add test coverage for deploy_predeployed_account sensei.
The static analysis indicates that this method is not covered by tests.
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for deploy_predeployed_account method
rg -A 5 'deploy_predeployed_account.*public_key: Felt' -g '*test*.rs'Length of output: 71
Ohayo sensei! It appears that the deploy_predeployed_account method isn’t currently covered by any tests. Since our verification script didn’t turn up any references in the test files, please add the necessary test coverage to ensure this functionality behaves as expected.
🏁 Script executed:
#!/bin/bash
# Searching for any references to deploy_predeployed_account in test files
rg -n 'deploy_predeployed_account' -g '*test*.rs'Length of output: 49
Ohayo sensei! It looks like the deploy_predeployed_account method isn’t covered by any tests yet. Could you add some test cases for this function? Ensuring proper test coverage here will help maintain robust functionality for our rollup deployment logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 227-229: crates/katana/chain-spec/src/rollup/utils.rs#L227-L229
Added lines #L227 - L229 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/primitives/src/genesis/allocation.rs (1)
199-200: Ohayo sensei! Please document the rationale behind the default salt value.The hardcoded value "666" seems arbitrary. Consider adding a comment explaining why this specific value was chosen and its significance for backward compatibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/chain-spec/src/rollup/utils.rs(15 hunks)crates/katana/primitives/src/genesis/allocation.rs(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
crates/katana/primitives/src/genesis/allocation.rs
[warning] 210-213: crates/katana/primitives/src/genesis/allocation.rs#L210-L213
Added lines #L210 - L213 were not covered by tests
🔇 Additional comments (7)
crates/katana/primitives/src/genesis/allocation.rs (3)
210-214: Ohayo sensei! Please add test coverage for new_with_balance.The function lacks test coverage, which could lead to undetected issues.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 210-213: crates/katana/primitives/src/genesis/allocation.rs#L210-L213
Added lines #L210 - L213 were not covered by tests
228-230: LGTM!The address calculation is correctly implemented using the standard pattern.
284-291: LGTM!The account generation logic correctly handles both cases with and without balance.
crates/katana/chain-spec/src/rollup/utils.rs (4)
132-151: LGTM!The contract deployment logic correctly uses the provided salt parameter.
189-225: LGTM!The dev account deployment logic correctly uses the account's salt.
227-229: Ohayo sensei! Please add test coverage for deploy_predeployed_account.The function lacks test coverage, which could lead to undetected issues.
291-318: LGTM!The account building logic correctly handles both dev and regular accounts.
Summary by CodeRabbit
New Features
Refactor