feat(katana): add STRK fee token #2536
Conversation
kariy
changed the title
Genesis methods to ChainSpec
kariy
changed the title
Genesis methods to ChainSpec
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2536 +/- ##
==========================================
+ Coverage 69.20% 69.51% +0.31%
==========================================
Files 388 388
Lines 49819 49868 +49
==========================================
+ Hits 34478 34667 +189
+ Misses 15341 15201 -140 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request introduces significant modifications across various files, primarily focusing on the transition from using Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
🪧 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: 13
🧹 Outside diff range and nitpick comments (13)
crates/katana/executor/benches/utils.rs (1)
12-12: Ohayo again, sensei! Thetxfunction update looks great!The change to
DEFAULT_ETH_FEE_TOKEN_ADDRESSis spot on and keeps us consistent with our new naming convention. It's like we're laying the groundwork for a multi-token future!For future enhancement, consider parameterizing the fee token address in this function. This could make our benchmarks more flexible, allowing us to test with different fee tokens easily. What do you think, sensei?
crates/katana/storage/provider/src/test_utils.rs (2)
37-42: Ohayo again, sensei! These changes look solid!The transition from
genesistochainand the corresponding method updates align well with the shift toChainSpec. It's a clear improvement in terms of code clarity and consistency.A small suggestion to consider:
Consider renaming
chaintochain_specfor even more clarity:- let chain = create_chain_for_testing(); + let chain_spec = create_chain_for_testing(); - let block = chain.block().seal_with_hash_and_status(hash, status); - let states = chain.state_updates(); + let block = chain_spec.block().seal_with_hash_and_status(hash, status); + let states = chain_spec.state_updates();This minor change would make it absolutely clear that we're working with a chain specification.
Line range hint
58-58: Ohayo, sensei! Good catch on the TODO!The TODO comment about creating a genesis builder is a great idea for future improvement. It could significantly simplify the genesis creation process and enhance maintainability.
Would you like assistance in creating a GitHub issue to track this TODO? I can help draft the issue description for implementing a genesis builder.
crates/katana/executor/tests/fixtures/transaction.rs (2)
Line range hint
101-107: Ohayo, sensei! Theexecutable_txfunction looks great!The transition from
GenesistoChainSpecis well-implemented. The function now correctly uses thechainparameter to access allocations.A small suggestion to enhance readability:
Consider using pattern matching to destructure the allocation directly:
let (&addr, GenesisAllocation::Account(account)) = chain.genesis.allocations .first_key_value() .expect("should have account");This would eliminate the need for the separate
elsepanic and make the code more concise.
Line range hint
122-129: Excellent work onexecutable_tx_without_max_fee, sensei!The transition from
GenesistoChainSpecis well-implemented here too. The function correctly uses thechainparameter to access allocations.As with the
executable_txfunction, here's a small suggestion to enhance readability:Consider using pattern matching to destructure the allocation directly:
let (&addr, GenesisAllocation::Account(account)) = chain.genesis.allocations .first_key_value() .expect("should have account");This would eliminate the need for the separate
elsepanic and make the code more concise.crates/katana/primitives/src/block.rs (1)
Line range hint
169-176: Ohayo, sensei! The new struct looks great!The
SealedBlockWithStatusstruct is well-designed and aligns perfectly with the PR objectives. It effectively combines a sealed block with its finality status, which is exactly what we need for the new fee token support.One small suggestion to enhance the documentation:
Consider adding a brief explanation of how this struct relates to the new fee token support. For example:
/// A sealed block along with its status. /// /// Block whose commitment has been computed. +/// This struct is particularly useful for tracking blocks in the context of different fee tokens. #[derive(Debug, Clone)] pub struct SealedBlockWithStatus { pub block: SealedBlock, /// The block status. pub status: FinalityStatus, }This addition would provide more context for developers working with this struct in the future.
crates/katana/primitives/src/genesis/constant.rs (1)
20-23: Ohayo again, sensei! This addition is spot on!The new
DEFAULT_STRK_FEE_TOKEN_ADDRESSconstant is exactly what we need to support STRK as a fee token. Great job on maintaining consistency with the ETH constant format!A tiny suggestion to make it even better:
Consider adding a brief explanation of what STRK is in the documentation comment, similar to how ETH is commonly known. This could help developers who might not be familiar with STRK. For example:
/// The default STRK (Starknet Token) fee token contract address. /// Corresponds to 0x04718f5a0Fc34cC1AF16A1cdee98fFB20C31f5cD61D6Ab07201858f4287c938Dcrates/katana/executor/tests/fixtures/mod.rs (1)
Line range hint
51-65: Ohayo, sensei! Thechainfunction looks sharp!The transition from
genesistochainis smooth like a well-forged Katana. The logic for generating accounts remains intact, which is great for consistency.A small suggestion to make it even better:
Consider adding a comment explaining the purpose of the
seedinitialization. It might help future ninjas understand the reasoning behind this specific seed value.let mut seed = [0u8; 32]; +// Initialize the seed to generate a consistent set of test accounts seed[0] = b'0';crates/katana/executor/tests/executor.rs (1)
309-312: Ohayo again, sensei! The assertion update looks good!The change to use
DEFAULT_ETH_FEE_TOKEN_ADDRESSin the assertion is consistent with the import statement modification. It maintains the test's functionality while adapting to the new constant name.Consider enhancing this test to cover scenarios where STRK is used as the fee token. This could involve adding a new test case or modifying the existing one to handle both ETH and STRK fee token scenarios. Here's a potential refactor suggestion:
-assert!( - actual_storage_updates.contains_key(&DEFAULT_ETH_FEE_TOKEN_ADDRESS), - "fee token storage must get updated" -); +for &fee_token in &[DEFAULT_ETH_FEE_TOKEN_ADDRESS, DEFAULT_STRK_FEE_TOKEN_ADDRESS] { + assert!( + actual_storage_updates.contains_key(&fee_token), + "fee token storage for {} must get updated", + if fee_token == DEFAULT_ETH_FEE_TOKEN_ADDRESS { "ETH" } else { "STRK" } + ); +}This change would ensure that the test covers both ETH and STRK fee token scenarios. Don't forget to import
DEFAULT_STRK_FEE_TOKEN_ADDRESSif you decide to implement this suggestion.crates/katana/primitives/src/genesis/mod.rs (2)
Line range hint
118-125: Ohayo, sensei! Typo in Comment: "depoyer" Should Be "deployer"In the comment before the universal deployer contract class, there's a minor typo:
// universal depoyer contract classPlease correct "depoyer" to "deployer" to maintain clarity.
Apply this diff to fix the typo:
-// universal depoyer contract class +// universal deployer contract class
9-9: Ohayo, sensei! Consider Organizing Imports for ReadabilityThe imports from
constantare spread across multiple statements:
Line 9~:
use constant::DEFAULT_ACCOUNT_CLASS;Lines 16~ to 19~:
use self::constant::{ DEFAULT_ACCOUNT_CLASS_CASM, DEFAULT_ACCOUNT_CLASS_HASH, DEFAULT_ACCOUNT_COMPILED_CLASS_HASH, DEFAULT_LEGACY_ERC20_CASM, DEFAULT_LEGACY_ERC20_CLASS_HASH, DEFAULT_LEGACY_ERC20_COMPILED_CLASS_HASH, DEFAULT_LEGACY_UDC_CASM, DEFAULT_LEGACY_UDC_CLASS_HASH, DEFAULT_LEGACY_UDC_COMPILED_CLASS_HASH, };Consolidating these imports can enhance readability and maintainability. Grouping related constants together or using glob imports where appropriate might help.
Also applies to: 16-19
crates/katana/core/src/backend/storage.rs (1)
Line range hint
71-93: Update documentation to reflectChainSpecusageThe
new_with_genesisfunction now accepts&ChainSpecinstead of&Genesis. Please update the function's documentation and any relevant comments to reflect this change for clarity.crates/katana/primitives/src/chain_spec.rs (1)
42-47: Add Documentation forFeeContractsStructOhayo, sensei! To improve code readability and maintainability, consider adding Rustdoc comments to the
FeeContractsstruct and its fieldsethandstrk. This will help other developers understand the purpose and usage of the fee token contracts in the chain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
- bin/katana/src/cli/node.rs (8 hunks)
- crates/katana/core/src/backend/storage.rs (11 hunks)
- crates/katana/executor/benches/utils.rs (3 hunks)
- crates/katana/executor/src/implementation/blockifier/utils.rs (1 hunks)
- crates/katana/executor/tests/executor.rs (2 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (4 hunks)
- crates/katana/executor/tests/fixtures/transaction.rs (5 hunks)
- crates/katana/node/src/lib.rs (3 hunks)
- crates/katana/primitives/src/block.rs (1 hunks)
- crates/katana/primitives/src/chain_spec.rs (1 hunks)
- crates/katana/primitives/src/genesis/constant.rs (2 hunks)
- crates/katana/primitives/src/genesis/json.rs (5 hunks)
- crates/katana/primitives/src/genesis/mod.rs (5 hunks)
- crates/katana/primitives/src/genesis/test-genesis-with-class.json (0 hunks)
- crates/katana/primitives/src/genesis/test-genesis-with-duplicate-name.json (0 hunks)
- crates/katana/primitives/src/genesis/test-genesis.json (0 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (14 hunks)
- crates/katana/storage/provider/src/test_utils.rs (4 hunks)
💤 Files with no reviewable changes (3)
- crates/katana/primitives/src/genesis/test-genesis-with-class.json
- crates/katana/primitives/src/genesis/test-genesis-with-duplicate-name.json
- crates/katana/primitives/src/genesis/test-genesis.json
🧰 Additional context used
🔇 Additional comments (27)
crates/katana/executor/benches/utils.rs (1)
3-3: Ohayo, sensei! LGTM on the import statement update!The change from
DEFAULT_FEE_TOKEN_ADDRESStoDEFAULT_ETH_FEE_TOKEN_ADDRESSaligns perfectly with our mission to support different fee-paying tokens. It's a small but crucial step towards greater flexibility in our fee structure.crates/katana/storage/provider/src/test_utils.rs (2)
6-6: Ohayo, sensei! These import changes look great!The new imports for
ChainSpecand the combined import foraddressandchain_specalign perfectly with the transition fromGenesistoChainSpec. This change enhances code organization and readability.Also applies to: 14-14
52-52: Ohayo once more, sensei! This function signature update is spot on!The renaming of
create_genesis_for_testingtocreate_chain_for_testingand the change in return type fromGenesistoChainSpecperfectly align with the overall transition in the codebase. It accurately represents the new functionality and maintains consistency.crates/katana/executor/tests/fixtures/transaction.rs (2)
2-2: Ohayo, sensei! LGTM on the import changes.The transition from
GenesistoChainSpecis well-reflected in the import statements. This change aligns perfectly with the PR objectives of updating the fee payment mechanism.
6-6: Excellent constant renaming, sensei!The change from
DEFAULT_FEE_TOKEN_ADDRESStoDEFAULT_ETH_FEE_TOKEN_ADDRESSis spot-on. It clearly indicates that this constant is specifically for the ETH fee token address, which aligns perfectly with the PR's goal of supporting both ETH and STRK as fee tokens.crates/katana/primitives/src/block.rs (1)
Line range hint
1-176: Ohayo again, sensei! Overall, the changes look solid!The addition of the
SealedBlockWithStatusstruct is well-integrated with the existing code. The minimal changes demonstrate a focused approach to implementing the new feature for supporting different fee tokens.The new struct complements the existing
SealedBlockandFinalityStatustypes, providing a clean way to associate a block with its finality status. This will be particularly useful for managing blocks with different fee tokens.Great job on maintaining code consistency and clarity!
crates/katana/primitives/src/genesis/constant.rs (2)
15-18: Ohayo, sensei! This change looks great!The renaming of
DEFAULT_FEE_TOKEN_ADDRESStoDEFAULT_ETH_FEE_TOKEN_ADDRESSenhances clarity and aligns perfectly with the PR's objective of supporting multiple fee tokens. It's a small change, but it makes a big difference in code readability.
113-113: Ohayo, sensei! Let's chat about this visibility change.I noticed that
get_fee_token_balance_base_storage_addressis nowpubinstead ofpub(super). While this might be intentional to allow broader use of this utility function, it's worth considering the implications:
- Does this change align with the module's encapsulation principles?
- Are there any security considerations in making this function more widely accessible?
- Are there other modules that now need this function, justifying the visibility change?
Could you please clarify the reasoning behind this change? It would be helpful to understand how this fits into the larger architecture and if any additional safeguards are needed.
crates/katana/executor/tests/fixtures/mod.rs (3)
10-10: Ohayo, sensei! LGTM on this import change.The addition of
ChainSpecimport is spot-on for the transition fromGenesistoChainSpec. It's like adding a new weapon to our Katana arsenal!
70-71: Ohayo, sensei! Thestate_provideradaptation is on point!The shift from
&Genesisto&ChainSpecin the function signature is as smooth as a Katana slice. The use ofchain.state_updates()maintains the functionality while embracing the newChainSpecstructure. Well done, fellow ninja!
226-229: Ohayo, sensei! Thecfgfunction update looks promising!The use of
DEFAULT_ETH_FEE_TOKEN_ADDRESSaligns perfectly with our mission to support STRK as a fee token. It's like sharpening our Katana for a new battle!However, let's make sure our blade is properly forged:
Could you confirm that
DEFAULT_ETH_FEE_TOKEN_ADDRESSis correctly defined and imported? Let's run a quick check:This will help us ensure that our new constant is properly in place across the codebase.
✅ Verification successful
Ohayo, sensei! The
DEFAULT_ETH_FEE_TOKEN_ADDRESSis correctly defined and imported across the codebase. Everything looks sharp and ready for battle!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the definition and import of DEFAULT_ETH_FEE_TOKEN_ADDRESS rg --type rust "DEFAULT_ETH_FEE_TOKEN_ADDRESS" crates/Length of output: 4034
crates/katana/node/src/lib.rs (4)
176-177: Ohayo, sensei! LGTM: Fee token addresses updated as per PR objectives.The changes to the FeeTokenAddresses struct now source both ETH and STRK addresses from
config.chain.fee_contracts. This aligns perfectly with the PR's goal of supporting STRK as a fee token and provides a more flexible approach to managing fee token addresses.
Line range hint
176-243: Ohayo, sensei! Let's wrap up this review with a bow.The changes in this file represent a significant shift in how chain configurations and fee tokens are handled. By moving from
genesis-specific configurations to broaderchainconfigurations, and explicitly including STRK alongside ETH as fee tokens, these modifications align perfectly with the PR objectives of supporting STRK for fee payments.These changes seem to be part of a larger refactoring effort, and while they look good in isolation, it's crucial to ensure that all related parts of the codebase have been updated consistently. Keep an eye out for any other files or components that might need similar updates to maintain consistency across the entire system.
Great work on implementing this feature, sensei! It's a solid step towards supporting Starknet transactions v3.
243-243: Ohayo once more, sensei! The genesis Blockchain initialization also looks good, but let's verify.The change from
&config.chain.genesisto&config.chainin theBlockchain::new_with_genesiscall is consistent with the previous change and aligns with the broader approach to chain configuration. However, we should also verify that theBlockchain::new_with_genesismethod implementation has been updated to handle this new parameter correctly.Let's confirm the
Blockchain::new_with_genesismethod signature:#!/bin/bash # Description: Verify the Blockchain::new_with_genesis method signature rg --type rust "fn new_with_genesis.*&ChainSpec" crates/
241-241: Ohayo again, sensei! The Blockchain initialization looks good, but let's double-check something.The change from
&config.chain.genesisto&config.chainin theBlockchain::new_with_dbcall looks good and aligns with the broader changes in chain configuration handling. However, we should verify that theBlockchain::new_with_dbmethod implementation has been updated to handle this new parameter correctly.Let's confirm the
Blockchain::new_with_dbmethod signature:crates/katana/executor/tests/executor.rs (1)
10-12: Ohayo, sensei! LGTM on the import statement update.The renaming of
DEFAULT_FEE_TOKEN_ADDRESStoDEFAULT_ETH_FEE_TOKEN_ADDRESSaligns well with the PR objective of supporting STRK as a fee token. It provides better clarity by explicitly mentioning ETH in the constant name.To ensure consistency across the codebase, let's verify if this renaming has been applied uniformly:
✅ Verification successful
Ohayo, sensei! The renaming of
DEFAULT_FEE_TOKEN_ADDRESStoDEFAULT_ETH_FEE_TOKEN_ADDRESShas been fully verified across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of DEFAULT_FEE_TOKEN_ADDRESS # and verify the usage of DEFAULT_ETH_FEE_TOKEN_ADDRESS echo "Checking for any remaining instances of DEFAULT_FEE_TOKEN_ADDRESS:" rg "DEFAULT_FEE_TOKEN_ADDRESS" --type rust echo "\nVerifying usage of DEFAULT_ETH_FEE_TOKEN_ADDRESS:" rg "DEFAULT_ETH_FEE_TOKEN_ADDRESS" --type rustLength of output: 4325
crates/katana/primitives/src/genesis/mod.rs (1)
Line range hint
109-116: Ohayo, sensei! Ensure Fee Token Configurability Aligns with PR ObjectivesThe default configuration currently hardcodes the fee token class as
DEFAULT_LEGACY_ERC20_CLASS_HASH(ETH):( DEFAULT_LEGACY_ERC20_CLASS_HASH, GenesisClass { sierra: None, casm: DEFAULT_LEGACY_ERC20_CASM.clone().into(), compiled_class_hash: DEFAULT_LEGACY_ERC20_COMPILED_CLASS_HASH, }, ),Given our objective to support STRK as a fee-paying token via a configuration flag, please verify that this hardcoded value does not impede users from specifying STRK as the fee token.
To confirm that fee token configurability is properly implemented, you can run:
✅ Verification successful
Fee Token Configurability Verified
Ohayo sensei! The fee token configurability aligns with PR objectives. Both ETH and STRK are supported as fee-paying tokens, and the hardcoded
DEFAULT_LEGACY_ERC20_CLASS_HASHfor ETH does not restrict the specification of STRK.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of fee-paying token configuration options. # Test: Search for command-line arguments or configurations related to the fee-paying token. # Expect: Occurrences of "--fee-paying-token" or similar options. rg --type rust 'fee[_-]paying[_-]token' --context 5Length of output: 53
Script:
#!/bin/bash # Description: Search for alternative fee token configuration options. # Test: Look for variations of fee token configuration keywords. # Expect: Occurrences of terms like "--fee-token", "--payment-token", "feeToken", or "paymentToken". rg --type rust 'fee[_-]?token|payment[_-]?token' --context 5Length of output: 29721
crates/katana/core/src/backend/storage.rs (4)
4-4: Ohayo, sensei! ImportingChainSpecis appropriateThe addition of
use katana_primitives::chain_spec::ChainSpec;is necessary for the updated function signatures and ensures that theChainSpectype is available.
284-287: Review fee token constants in testsThe tests are using
DEFAULT_ETH_FEE_TOKEN_ADDRESSand related constants fromgenesis::constant. Since the objective is to supportSTRKas a fee-paying token, ensure that tests cover scenarios usingSTRKand that the constants are updated or parameterized as needed.Do you need help updating the tests to include
STRKas a fee-paying token?
107-113: Confirm parameter update innew_from_forkedThe function
new_from_forkednow acceptschain: &ChainSpecinstead ofgenesis: &Genesis. Ensure that all internal references within this function have been updated fromgenesistochainto maintain consistency.Run the following script to check for lingering references to
genesis:#!/bin/bash # Description: Check for residual uses of `genesis` in `new_from_forked` # Test: Search within the `new_from_forked` method for `genesis` # Expect: No occurrences of `genesis` should be found rg 'fn new_from_forked' -A 15 | rg 'genesis'
99-102: Ensure consistent updates innew_with_dbThe
new_with_dbfunction now takeschain: &ChainSpecas a parameter. Verify that all calls to this function have been updated accordingly to prevent any mismatches.Run the following script to find all calls to
new_with_dband ensure they pass&ChainSpec:✅ Verification successful
Ohayo, sensei!
It appears that the initial search didn't find any calls to
new_with_db. To ensure we haven't missed any, let's refine our search pattern. Please run the following script to comprehensively find all invocations ofnew_with_db:[/run_scripts]
Ohayo, sensei!
To ensure we've thoroughly checked all usages of
new_with_db, please run the following script to find every call to this function, regardless of its arguments:[/run_scripts]
Ohayo, sensei!
All calls to
new_with_dbcorrectly pass&ChainSpecas the second argument.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `new_with_db` use `&ChainSpec` # Test: Search for function calls to `new_with_db` # Expect: All calls pass `&ChainSpec` as the second argument rg 'new_with_db\(([^,]+),\s*&\w+\)' -A 2 -B 2Length of output: 250
Script:
#!/bin/bash # Description: Verify all calls to `new_with_db` use `&ChainSpec` # Refined Test: Search for function calls to `new_with_db` with any second argument rg 'new_with_db\([^,]+,\s*&ChainSpec\)' --type rustLength of output: 53
Script:
#!/bin/bash # Description: Find all calls to `new_with_db` regardless of arguments rg 'new_with_db\(' --type rustLength of output: 481
bin/katana/src/cli/node.rs (6)
155-155: Update todefault_value_tis AppropriateOhayo, sensei! Great job updating
#[arg(default_value = "10")]to#[arg(default_value_t = 10)]fortotal_accounts. This change ensures that the default value is correctly interpreted as a numeric type.
416-429: Confirm STRK Fee Token IntegrationOhayo, sensei! The
print_genesis_contractsfunction now includes details for the STRK Fee Token. Please verify thatchain.fee_contracts.strkcorrectly references the STRK token's address and that the class hash is accurate.Would you like assistance in verifying these values or updating the display to ensure all fee tokens are correctly represented?
Line range hint
368-438: Ensure Accurate Data Display in Intro MessagesOhayo, sensei! The modifications to
print_introandprint_genesis_contractsaim to display the correct information based onChainSpec. Please confirm that all data, such as predeployed contracts and account details, are accurately retrieved and displayed.
312-338: Validatechain_specInitialization and Account GenerationOhayo, sensei! The
chain_spec()function has been refactored to initialize fromDEV_UNALLOCATEDand conditionally set thechain_idandgenesis. Please ensure that:
- When a
chain_idis provided, it's correctly assigned tochain_spec.id.- If a
genesisconfiguration is provided, it's properly integrated.- The development accounts are generated and allocated as expected.
Here's a script to review how
chain_specis built and modified:#!/bin/bash # Description: Inspect `chain_spec` initialization and modifications rg --type rust 'fn chain_spec\(' -A 50
236-236: Ensure Consistency with Updatedprint_introFunctionOhayo, sensei! The
print_introfunction now takes&ChainSpecinstead of&Genesis. Please verify that all calls toprint_introare updated accordingly and that the function processes theChainSpecdata correctly.Here's a script to check all calls to
print_intro:#!/bin/bash # Description: Find all calls to `print_intro` to ensure they pass `&ChainSpec` rg --type rust 'print_intro\(' -A 2
185-185:⚠️ Potential issueHandle Optional
chain_idCorrectlyOhayo, sensei! The
chain_idfield has been changed toOption<ChainId>. Please ensure that all usages ofchain_idin the code handle theNonecase properly to avoid potentialNoneunwraps or unexpected behaviors.Here's a script to verify how
chain_idis used throughout the codebase:
| eth: DEFAULT_ETH_FEE_TOKEN_ADDRESS, | ||
| strk: DEFAULT_ETH_FEE_TOKEN_ADDRESS, |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! Found some inconsistencies with STRK fee token addresses.
I noticed that in benches/utils.rs, both eth and strk are set to DEFAULT_ETH_FEE_TOKEN_ADDRESS. However, elsewhere in the codebase, strk has its own distinct address (DEFAULT_STRK_FEE_TOKEN_ADDRESS). This inconsistency might lead to unexpected behavior during benchmarking.
🔗 Analysis chain
Ohayo, sensei! We might have a small hiccup in the envs function.
I noticed that both eth and strk fee token addresses are set to DEFAULT_ETH_FEE_TOKEN_ADDRESS. This seems a bit odd, considering our grand plan to support STRK as a separate fee-paying token.
Could you clarify if this is intentional? If not, we might want to introduce a separate constant for the STRK fee token address.
To verify the current usage of fee token addresses, let's run this quick check:
This will help us ensure consistency across the codebase, sensei!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of fee token address usage
rg --type rust "DEFAULT_ETH_FEE_TOKEN_ADDRESS" -C 3
Length of output: 19572
| let mut chain = chain_spec::DEV_UNALLOCATED.clone(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! These changes are looking good!
The transition to creating a ChainSpec object while preserving the existing genesis creation logic is well-implemented. It maintains backward compatibility while allowing for future extensions to the ChainSpec.
A suggestion for potential improvement:
Consider extracting the genesis creation logic into a separate function for better modularity:
fn create_genesis() -> Genesis {
let mut genesis = Genesis::default();
// ... (existing genesis creation logic)
genesis
}
pub fn create_chain_for_testing() -> ChainSpec {
let mut chain = chain_spec::DEV_UNALLOCATED.clone();
chain.genesis = create_genesis();
chain
}This refactoring would improve readability and make it easier to maintain or modify the genesis creation logic independently of the ChainSpec creation.
Also applies to: 79-81
| impl core::fmt::Debug for GenesisClass { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("GenesisClass") | ||
| .field("compiled_class_hash", &self.compiled_class_hash) | ||
| .field("casm", &"...") | ||
| .field("sierra", &"...") | ||
| .finish() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Clarify the Debug Implementation for GenesisClass
In the custom Debug implementation for GenesisClass, the casm and sierra fields are represented as "...":
.field("casm", &"...")
.field("sierra", &"...")While this simplifies the debug output, consider whether this meets our debugging needs. Including more informative placeholders or actual summaries of these fields could enhance debugging effectiveness.
| DEFAULT_ETH_FEE_TOKEN_ADDRESS, DEFAULT_LEGACY_ERC20_CASM, DEFAULT_LEGACY_ERC20_CLASS_HASH, | ||
| DEFAULT_LEGACY_UDC_CASM, DEFAULT_LEGACY_UDC_CLASS_HASH, DEFAULT_UDC_ADDRESS, | ||
| }; | ||
| use katana_primitives::genesis::Genesis; | ||
| use katana_primitives::receipt::{InvokeTxReceipt, Receipt}; | ||
| use katana_primitives::state::StateUpdatesWithDeclaredClasses; | ||
| use katana_primitives::trace::TxExecInfo; | ||
| use katana_primitives::transaction::{InvokeTx, Tx, TxWithHash}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor test imports to use chain_spec module
The test module imports from katana_primitives::genesis::Genesis. Since the main code now utilizes ChainSpec, consider updating the tests to import from katana_primitives::chain_spec for consistency.
| }; | ||
|
|
||
| let mut chain = chain_spec::DEV.clone(); | ||
| chain.genesis = genesis; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modify test setup to align with ChainSpec usage
In the blockchain_from_fork test, you're creating a Genesis instance and then assigning it to chain.genesis. To maintain consistency with the updated codebase, consider creating a custom ChainSpec directly or providing a helper function to generate a ChainSpec with the desired parameters.
| println!("creating invoke transaction v3..."); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider replacing println! with a proper logging mechanism
Using println! for logging is not recommended in production code. It's better to use a logging framework to manage log levels and outputs.
Apply this diff to replace println! with an appropriate logging macro:
- println!("creating invoke transaction v3...");
+ log::debug!("creating invoke transaction v3...");📝 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.
| println!("creating invoke transaction v3..."); | |
| log::debug!("creating invoke transaction v3..."); |
| // craft a raw v3 transaction, should probably use abigen for simplicity | ||
| let to = DEFAULT_STRK_FEE_TOKEN_ADDRESS.into(); | ||
| let selector = selector!("transfer"); | ||
| let calldata = vec![felt!("0x1"), felt!("0x1"), Felt::ZERO]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider using abigen to simplify transaction crafting
In line 910, there's a comment suggesting that using abigen could simplify the crafting of the raw v3 transaction. Leveraging abigen would enhance code readability and maintainability by automatically generating contract bindings.
| @@ -171,7 +171,7 @@ | |||
|
|
|||
| // Fund the new account | |||
| abigen_legacy!(FeeToken, "crates/katana/rpc/rpc/tests/test_data/erc20.json"); | |||
| let contract = FeeToken::new(DEFAULT_FEE_TOKEN_ADDRESS.into(), &funding_account); | |||
| let contract = FeeToken::new(DEFAULT_ETH_FEE_TOKEN_ADDRESS.into(), &funding_account); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider parameterizing tests to include the STRK fee token
The tests at these lines currently use DEFAULT_ETH_FEE_TOKEN_ADDRESS as the fee token. With the introduction of STRK support, it would be beneficial to parameterize these tests to run with both ETH and STRK fee tokens. This ensures comprehensive test coverage and verifies functionality across both fee payment options.
Also applies to: 248-248, 313-313, 367-367, 398-398, 477-477, 523-523, 594-594, 680-680, 768-768, 818-818
| @@ -1011,33 +842,20 @@ mod tests { | |||
| let expected_genesis = Genesis { | |||
| classes: expected_classes, | |||
| number: 0, | |||
| fee_token: expected_fee_token, | |||
| // fee_token: expected_fee_token, | |||
There was a problem hiding this comment.
Ohayo sensei! Update tests to reflect the removal of fee_token
In the test, the line // fee_token: expected_fee_token, is commented out. Ensure that all tests are updated to accommodate the removal of fee_token from the GenesisJson struct. This includes removing or modifying test cases that are no longer applicable to maintain comprehensive test coverage.
| // pub fee_token: FeeTokenConfigJson, | ||
| // pub universal_deployer: Option<UniversalDeployerConfigJson>, |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Impact of Removing fee_token Detected
- References to
fee_tokenfound in the following files:crates/katana/primitives/src/chain_spec.rscrates/katana/node/src/lib.rscrates/katana/executor/tests/executor.rscrates/katana/executor/tests/fixtures/mod.rscrates/katana/executor/benches/utils.rscrates/katana/executor/src/implementation/blockifier/utils.rscrates/katana/core/src/backend/storage.rs
Please update or remove these references to align with the removal of the fee_token field from GenesisJson.
🔗 Analysis chain
Ohayo sensei! Verify the impact of removing fee_token from GenesisJson
The fee_token field has been removed from the GenesisJson struct. This change may affect how fees are configured during genesis initialization. Please confirm that this aligns with the PR objectives and that all dependent code and configurations have been updated accordingly.
Run the following script to check for any remaining references to fee_token in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to 'fee_token' outside this file.
rg --type rust 'fee_token' --glob '!crates/katana/primitives/src/genesis/json.rs'
Length of output: 5762
kariy
force-pushed
the
katana/strk-fee-token
branch
from
2e4f96d
to
8f186d6
Compare
WalkthroughOhayo, sensei! This pull request introduces significant changes to the configuration and handling of blockchain parameters within the Katana project. Key modifications include the restructuring of the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (18)
💤 Files with no reviewable changes (3)
🧰 Additional context used🪛 GitHub Check: codecov/patch
🔇 Additional comments (24)
🪧 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
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (19)
crates/katana/executor/benches/utils.rs (1)
12-12: Ohayo again, sensei! This change looks good, but let's think ahead!The update to use
DEFAULT_ETH_FEE_TOKEN_ADDRESSin thecalldatavector is spot on. It's consistent with our import changes and ensures we're using the correct ETH fee token address.However, to make our code more flexible for future updates, we might want to consider using a configurable fee token address here. This could allow for easier switching between ETH and STRK in our benchmarks.
What do you think about introducing a parameter to the
tx()function that allows specifying the fee token address? This could make our benchmarks more versatile. Here's a quick example:pub fn tx(fee_token_address: Felt) -> ExecutableTxWithHash { let invoke = InvokeTx::V1(InvokeTxV1 { sender_address: felt!("0x1").into(), calldata: vec![ fee_token_address.into(), // ... rest of the calldata ], // ... rest of the struct }); // ... rest of the function }This way, we could easily benchmark with different fee tokens. What do you think, sensei?
crates/katana/executor/tests/fixtures/transaction.rs (3)
46-46: Ohayo again, sensei! Theinvoke_executable_txfunction looks good!The update to use
DEFAULT_ETH_FEE_TOKEN_ADDRESSis consistent with the earlier changes. It maintains the current behavior while paving the way for future enhancements.For future iterations, consider parameterizing the fee token address to allow for easy switching between ETH and STRK based on user configuration.
101-102: Ohayo once more, sensei! Theexecutable_txfunction changes look solid!The update to use
ChainSpecinstead ofGenesisaligns perfectly with the PR objectives. It provides more flexibility for future enhancements related to fee token configuration.Consider adding a comment explaining the relationship between
ChainSpecandGenesisfor future maintainers.
122-125: Ohayo yet again, sensei! Theexecutable_tx_without_max_feefunction changes are on point!The updates here mirror those in the
executable_txfunction, maintaining consistency across the codebase. Great job!Consider extracting the common logic for accessing allocations from
ChainSpecinto a helper function to reduce code duplication betweenexecutable_txandexecutable_tx_without_max_fee.crates/katana/primitives/src/block.rs (1)
Line range hint
169-175: Ohayo, sensei! The new struct looks good, but let's add a bit more flavor to the docs!The
SealedBlockWithStatusstruct is a great addition and aligns well with the PR objectives. It nicely encapsulates a sealed block along with its finality status.Consider expanding the documentation comment to provide more context. Here's a suggestion:
- /// Block whose commitment has been computed. + /// Represents a sealed block along with its finality status. + /// This struct is used to track blocks that have been processed and sealed, + /// providing information about their acceptance status on L1 or L2.This additional context helps developers understand the purpose and usage of the struct more clearly.
crates/katana/primitives/src/genesis/constant.rs (1)
20-24: Ohayo again, sensei! Excellent addition of the STRK fee token constant.The new
DEFAULT_STRK_FEE_TOKEN_ADDRESSconstant perfectly aligns with the PR objective of supporting STRK as a fee token. The formatting and style are consistent with the existing codebase.Consider adding a brief comment explaining the significance of STRK in the context of Starknet's recent transaction updates. This could help future developers understand the rationale behind this addition.
crates/katana/executor/tests/fixtures/mod.rs (1)
226-229: Ohayo for the last time, sensei! Thecfg()function is now fee-token-aware!The switch to
DEFAULT_ETH_FEE_TOKEN_ADDRESSshows we're on the right path to fee token enlightenment. It's like upgrading from a abacus to a calculator!However, I noticed we're still using a hardcoded value for the STRK address. Perhaps we should consider using a constant for this as well? Something like:
let fee_token_addresses = FeeTokenAddressses { eth: DEFAULT_ETH_FEE_TOKEN_ADDRESS, - strk: ContractAddress(222u64.into()), + strk: DEFAULT_STRK_FEE_TOKEN_ADDRESS, };This would make our code more consistent and easier to maintain, sensei. What do you think?
crates/katana/node/src/lib.rs (1)
243-243: Ohayo, sensei! LGTM with a refactor suggestionThe update to
Blockchain::new_with_genesislooks good and is consistent with the changes made tonew_with_db. It's great to see the broader&config.chainbeing used here as well.To reduce duplication and improve maintainability, we could consider extracting the common
&config.chainparameter into a separate variable or method. What do you think about this refactor suggestion, sensei?Here's a possible refactor:
let chain_config = &config.chain; // ... other code ... match (...) { // ... other cases ... _ => (Blockchain::new_with_genesis(InMemoryProvider::new(), chain_config)?, None) }crates/katana/executor/tests/executor.rs (1)
Line range hint
1-341: Ohayo again, sensei! A word on the big picture.The change we've seen is small but significant. While our tests look solid for the current setup, we might want to consider expanding them in the future to cover scenarios with STRK as the fee token. This could involve:
- Adding test cases that use STRK for fee payments.
- Verifying that the executor correctly handles both ETH and STRK fee scenarios.
- Ensuring that the
cfg_env.fee_token_addressesis correctly populated and used throughout the tests.What do you think, sensei? Shall we open an issue to track these potential enhancements?
crates/katana/storage/provider/src/test_utils.rs (1)
Line range hint
52-81: Ohayo sensei! Consider implementing a genesis builderTo enhance maintainability and readability, consider using a genesis builder to construct the
ChainSpecmore elegantly, as mentioned in the TODO comment. This will make the code more modular and easier to extend in the future.Would you like assistance in creating a genesis builder or should I open a GitHub issue to track this task?
crates/katana/primitives/src/genesis/mod.rs (1)
118-118: Typo in comment: 'depoyer' should be 'deployer'.Ohayo, sensei! There's a minor typo in the comment at line 118. It should read "
universal deployer contract class" instead of "universal depoyer contract class".crates/katana/core/src/backend/storage.rs (3)
173-175: Ohayo, sensei! Add test cases for STRK fee token.In the
blockchain_from_genesis_statestest, the state check usesDEFAULT_ETH_FEE_TOKEN_ADDRESS. To fully validate the new feature, consider adding test cases where the fee token isSTRK.Would you like assistance in writing additional tests to cover the
STRKfee token scenario?
194-196: Ohayo, sensei! Clone and modifyChainSpeccarefully.In lines 194-196, you clone
chain_spec::DEVand modify itsgenesis. Ensure that this does not unintentionally affect other tests or parts of the code that rely onchain_spec::DEV.Consider creating a new
ChainSpecinstance or using a testing utility to prevent side effects.
284-286: Ohayo, sensei! Fix potential typo in test assertion.In the test, the class comparison uses
DEFAULT_LEGACY_ERC20_CASM.clone(). Verify thatclone()is necessary here and that it doesn't affect the assertion outcome.If
DEFAULT_LEGACY_ERC20_CASMis already a clone or a value type, theclone()method might be redundant.bin/katana/src/cli/node.rs (3)
236-236: Ohayo, sensei! Consider adding tests forprint_intro.The call to
print_introis crucial for displaying startup information. Adding tests can help ensure that the introduction outputs as expected, enhancing reliability.Would you like assistance in creating unit tests for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 236-236: bin/katana/src/cli/node.rs#L236
Added line #L236 was not covered by tests
368-369: Ohayo, sensei! Adding tests for account display would be beneficial.The
print_introfunction now processes account information. Including tests will help ensure account details are correctly presented to users.Would you like help in drafting tests for displaying account information?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-369: bin/katana/src/cli/node.rs#L368-L369
Added lines #L368 - L369 were not covered by tests
399-399: Ohayo, sensei! Ensureprint_genesis_contractsis tested.The function
print_genesis_contractsoutputs important contract information. Adding tests can help maintain correctness as the codebase evolves.Can I assist in creating tests for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 399-399: bin/katana/src/cli/node.rs#L399
Added line #L399 was not covered by testscrates/katana/primitives/src/chain_spec.rs (2)
50-61: Consider adding unit tests for the newblockmethod.The
blockmethod provides a crucial function to generate the genesis block. To ensure its reliability, please consider adding unit tests that verify its correctness.Would you like assistance in creating unit tests for this method, sensei?
64-131: Ensure thorough testing of thestate_updatesmethod.The
state_updatesmethod is essential for initializing state updates with declared classes and deployed contracts. Adding comprehensive unit tests will enhance the robustness of this method and ensure the chain state is correctly established.I'm happy to help draft test cases for this method if you'd like, sensei.
🛑 Comments failed to post (13)
crates/katana/executor/benches/utils.rs (1)
37-38: 💡 Codebase verification
⚠️ Potential issueUpdate STRK Fee Token Address
Ohayo, sensei!
I noticed that
DEFAULT_STRK_FEE_TOKEN_ADDRESSis defined incrates/katana/primitives/src/genesis/constant.rs. Currently, bothethandstrkinfee_token_addressesare set toDEFAULT_ETH_FEE_TOKEN_ADDRESS. It would be best to update thestrkfield to useDEFAULT_STRK_FEE_TOKEN_ADDRESSinstead:fee_token_addresses: FeeTokenAddresses { eth: DEFAULT_ETH_FEE_TOKEN_ADDRESS, strk: DEFAULT_STRK_FEE_TOKEN_ADDRESS, },This ensures that STRK is correctly associated with its designated fee token address.
🔗 Analysis chain
Ohayo, sensei! We might have a small hiccup here.
I noticed that both
ethandstrkfields in thefee_token_addressesare set toDEFAULT_ETH_FEE_TOKEN_ADDRESS. This seems a bit odd, considering our noble quest to support STRK as a separate fee token.Is this intentional for testing purposes, or should we be using a different address for
strk? If it's intentional, perhaps we could add a comment explaining why both are set to the ETH address for now.If it's not intentional, we might want to introduce a separate constant for the STRK fee token address. Something like this:
fee_token_addresses: FeeTokenAddressses { eth: DEFAULT_ETH_FEE_TOKEN_ADDRESS, strk: DEFAULT_STRK_FEE_TOKEN_ADDRESS, },What are your thoughts on this, sensei? Should we keep it as is or make a change here?
To double-check if we have a
DEFAULT_STRK_FEE_TOKEN_ADDRESSconstant defined elsewhere, we can run:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DEFAULT_STRK_FEE_TOKEN_ADDRESS constant rg --type rust "const DEFAULT_STRK_FEE_TOKEN_ADDRESS"Length of output: 164
crates/katana/node/src/lib.rs (1)
241-241: 💡 Codebase verification
Ohayo, sensei!
It looks like there are no existing tests for
Blockchain::new_with_db. To ensure the new configuration changes are properly validated, could you please add appropriate test coverage for this method?🔗 Analysis chain
Ohayo, sensei! LGTM with a small suggestion
The update to
Blockchain::new_with_dblooks good, using the broader&config.chaininstead of just the genesis configuration. This change aligns well with our new flexible fee token setup.However, our codecov sensei noticed that this line isn't covered by tests yet. Would you mind adding some test coverage for this change to ensure it behaves as expected?
Ohayo, sensei! Let me check for any tests related to
Blockchain::new_with_dbelsewhere in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests related to Blockchain::new_with_db rg --type rust "new_with_db.*config\.chain" crates/katana/node/src/testsLength of output: 143
Script:
#!/bin/bash # Search for tests related to Blockchain::new_with_db across the codebase rg --type rust "new_with_db.*config\.chain"Length of output: 147
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 241-241: crates/katana/node/src/lib.rs#L241
Added line #L241 was not covered by testscrates/katana/storage/provider/src/test_utils.rs (1)
37-42: 💡 Codebase verification
Ohayo sensei!
Several remaining references to
genesiswere found outside the expected locations. Please update these tochainto ensure consistency and prevent potential issues:
crates/saya/core/src/lib.rscrates/katana/node-bindings/src/lib.rscrates/katana/executor/tests/fixtures/mod.rscrates/katana/storage/provider/src/traits/block.rscrates/katana/controller/src/lib.rs🔗 Analysis chain
Ohayo sensei! Verify all references to
genesisare updated tochainIn updating to use
ChainSpec, please ensure that all references togenesishave been replaced withchainto prevent any potential issues.You can run the following script to check for any remaining references to
genesisoutside of expected locations:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining usages of 'genesis' in the codebase, excluding definitions and the genesis module. # Test: Search for 'genesis' in Rust files, excluding the genesis module and test utilities. rg --type rust --word-regexp '\bgenesis\b' --glob '!*genesis*' --glob '!**/genesis/**' --glob '!**/test_utils.rs'Length of output: 12408
crates/katana/core/src/backend/storage.rs (3)
142-145: 🛠️ Refactor suggestion
Ohayo, sensei! Parameterize fee token address constants in tests.
The test uses
DEFAULT_ETH_FEE_TOKEN_ADDRESS, but since the fee-paying token is now configurable (eitherETHorSTRK), it would be beneficial to parameterize the fee token address to reflect this change.Consider introducing a variable or a test parameter that allows the tests to run with both fee token addresses. This enhances test coverage for different configurations.
284-287:
⚠️ Potential issueOhayo, sensei! Update class hash checks for configurable fee tokens.
The test verifies the
actual_fee_token_class_hashagainstDEFAULT_LEGACY_ERC20_CLASS_HASH, which corresponds toETH. With the introduction ofSTRKas a fee token, the test should account for the correct class hash based on the selected fee token.Consider modifying the test to check the class hash dynamically, depending on the configured fee token. This ensures that the test remains valid regardless of the fee token used.
71-78:
⚠️ Potential issueOhayo, sensei! Update error message to reflect
ChainSpec.In the
new_with_genesisfunction, the error message on line 78 still refers to "Genesis block hash mismatch". Since we are now usingChainSpec, consider updating the error message for clarity.Apply this diff to update the error message:
- Err(anyhow!( - "Genesis block hash mismatch: expected {genesis_hash:#x}, got {db_hash:#}", - )) + Err(anyhow!( + "Block hash mismatch in ChainSpec: expected {genesis_hash:#x}, got {db_hash:#x}", + ))Committable suggestion was skipped due to low confidence.
bin/katana/src/cli/node.rs (2)
413-444: 🛠️ Refactor suggestion
Ohayo, sensei! Dynamic display of fee tokens could enhance UX.
Currently, both ETH and STRK fee tokens are displayed regardless of the user's selection. Consider adjusting
print_genesis_contractsto show only the fee token in use, making the output more relevant.You might implement a check based on the configured fee token and conditionally display the corresponding information.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 413-413: bin/katana/src/cli/node.rs#L413
Added line #L413 was not covered by tests
[warning] 416-429: bin/katana/src/cli/node.rs#L416-L429
Added lines #L416 - L429 were not covered by tests
[warning] 432-438: bin/katana/src/cli/node.rs#L432-L438
Added lines #L432 - L438 were not covered by tests
[warning] 443-444: bin/katana/src/cli/node.rs#L443-L444
Added lines #L443 - L444 were not covered by tests
185-185: 💡 Codebase verification
Ohayo, sensei! 🌸 It seems there are instances where
chain_idis used without handling theNonecase. Please review the following locations to ensure properNonehandling:
crates/saya/provider/src/rpc/mod.rs:
.map(|tx_rpc| tx_converter::tx_from_rpc(tx_rpc, self.chain_id))
crates/katana/executor/src/implementation/blockifier/utils.rs:
let chain_info = ChainInfo { fee_token_addresses, chain_id: to_blk_chain_id(cfg_env.chain_id) };
bin/saya/src/args/mod.rs:
chain_id: cairo_short_string_to_felt(&args.starknet_account.chain_id)?🔗 Analysis chain
Ohayo, sensei! Ensure proper handling of optional
chain_id.By changing
pub chain_id: ChainIdtopub chain_id: Option<ChainId>, you've made thechain_idoptional. Please verify that all usages ofchain_idhandle theNonecase appropriately to prevent potentialNonedereferencing issues.You can run the following script to check for unhandled
Nonecases:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for instances where `chain_id` is used without checking for `None` rg --type rust "(\.chain_id\s*;|\.\s*chain_id\s*\))" --context 2Length of output: 1928
crates/katana/primitives/src/chain_spec.rs (1)
162-209:
⚠️ Potential issueHandle potential errors in
add_fee_tokenfunction gracefully.In the
add_fee_tokenfunction, the use of.unwrap()oncairo_short_string_to_feltcould lead to a panic if the conversion fails. Although the inputs are hardcoded and unlikely to fail, it's good practice to handle potential errors to improve reliability.Consider modifying the code to handle errors without panicking:
-let name = cairo_short_string_to_felt(name).unwrap(); -let symbol = cairo_short_string_to_felt(symbol).unwrap(); +let name = cairo_short_string_to_felt(name)?; +let symbol = cairo_short_string_to_felt(symbol)?;This change will require updating the function signature to return a
Result.Committable suggestion was skipped due to low confidence.
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
216-217:
⚠️ Potential issueOhayo, sensei! Consider replacing
println!with a logging mechanismUsing
println!may not be ideal for production code as it writes directly to standard output. It's better to use a logging library to control log levels and outputs effectively.Apply this diff to replace
println!with a logging statement:- println!("creating invoke transaction v3..."); + log::debug!("creating invoke transaction v3...");Ensure that the appropriate logging crate (e.g.,
logortracing) is included and properly configured.📝 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.log::debug!("creating invoke transaction v3...");crates/katana/rpc/rpc/tests/starknet.rs (1)
174-174: 🛠️ Refactor suggestion
Parameterize the fee token address in test contracts
Ohayo, sensei! Multiple test cases are initializing
Erc20ContractandFeeTokeninstances withDEFAULT_ETH_FEE_TOKEN_ADDRESS. To fully support testing with both ETH and STRK fee tokens, consider parameterizing the fee token address based on the configuration.Refactor the code to use the configured fee token address:
-let contract = Erc20Contract::new(DEFAULT_ETH_FEE_TOKEN_ADDRESS.into(), &account); +let fee_token_address = if use_strk_fee_token { + DEFAULT_STRK_FEE_TOKEN_ADDRESS +} else { + DEFAULT_ETH_FEE_TOKEN_ADDRESS +}; +let contract = Erc20Contract::new(fee_token_address.into(), &account);Ensure that
use_strk_fee_tokenreflects the actual configuration used in the tests.Also applies to: 248-248, 313-313, 367-367, 398-398, 477-477, 523-523, 594-594, 680-680, 768-768, 818-818
crates/katana/primitives/src/genesis/json.rs (2)
236-237:
⚠️ Potential issueRemove commented-out code for a cleaner codebase
Ohayo, sensei! The
fee_tokenanduniversal_deployerfields in theGenesisJsonstruct are commented out on lines 236-237. It's best to remove unused code instead of commenting it out to maintain code cleanliness and readability.Apply this diff to remove the commented-out code:
- // pub fee_token: FeeTokenConfigJson, - // pub universal_deployer: Option<UniversalDeployerConfigJson>,📝 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.
845-845:
⚠️ Potential issueEliminate commented-out line to maintain clarity
Ohayo, sensei! The
fee_tokenfield is commented out on line 845. Removing it entirely would help keep the codebase clean and avoid confusion.Apply this diff to remove the unnecessary line:
- // fee_token: expected_fee_token,📝 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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/katana/primitives/src/chain_spec.rs (9 hunks)
🧰 Additional context used
🔇 Additional comments (4)
crates/katana/primitives/src/chain_spec.rs (4)
590-592: Handle Potential Errors Instead of Using.unwrap()Ohayo, sensei! As previously mentioned, using
.unwrap()withcairo_short_string_to_feltcan cause a panic if the conversion fails. Consider handling the potential errors gracefully to enhance robustness.
630-632: Ensure Consistent Balance Checks for STRK Fee TokenOhayo, sensei! Great job adding balance checks for the STRK fee token. Please ensure that all allocations with balances are correctly accounted for in both ETH and STRK to maintain consistency across fee tokens.
651-661: Verify Total Supply Calculation for STRK Fee TokenOhayo, sensei! Please verify that the total supply for the STRK fee token is accurately calculated from the allocation balances, just as it is for ETH. This ensures that the STRK fee token reflects the correct total supply based on initial allocations.
To confirm the total supply calculation, you can run the following script:
#!/bin/bash # Description: Verify that the total supply of STRK is calculated correctly from allocations. # Expected: The total supply should match the sum of all allocation balances. # Calculate total allocated STRK balance alloc_total=$(rg 'balance: Some\((U256::from_str\("0x[0-9a-fA-F]+"\)\.unwrap\(\))\)' crates/katana/primitives/src/chain_spec.rs | sed -E 's/.*"0x([0-9a-fA-F]+)".*/0x\1/' | awk '{ sum += strtonum($1) } END { printf("%x\n", sum) }') # Display total supply from STRK fee token storage rg 'assert_eq!\(.*strk_fee_token_storage.*ERC20_TOTAL_SUPPLY_STORAGE_SLOT.*' -A 5 crates/katana/primitives/src/chain_spec.rs echo "Calculated total allocated STRK balance: $alloc_total"
110-119: Verify Correct Class Hash for STRK Fee TokenOhayo, sensei! Please verify that
DEFAULT_LEGACY_ERC20_CLASS_HASHis the appropriate class hash for the STRK fee token. If STRK uses a different ERC20 contract class, you might need to update the class hash accordingly to ensure proper functionality.To assist with verification, please run the following script:
✅ Verification successful
Verified: Correct Class Hash for STRK Fee Token
Ohayo, sensei! I've confirmed that
DEFAULT_LEGACY_ERC20_CLASS_HASHis the appropriate class hash for the STRK fee token, as no separate class hash exists for STRK. Everything looks good!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there is a specific class hash defined for STRK fee token. # Expected: Find if a separate class hash exists for STRK. # Search for STRK-specific class hash definitions rg 'DEFAULT_STRK.*CLASS_HASH' crates/katana/primitives/src/* # Display the current value of DEFAULT_LEGACY_ERC20_CLASS_HASH rg 'pub const DEFAULT_LEGACY_ERC20_CLASS_HASH' crates/katana/primitives/src/*Length of output: 470
| @@ -60,8 +60,7 @@ | |||
| Block { header, body: Vec::new() } | |||
| } | |||
|
|
|||
| // this method will include the the ETH fee token, and the UDC. Declaring and deploying the | |||
| // necessary classes and contracts. | |||
| // this method will include the the ETH and STRK fee tokens, and the UDC | |||
There was a problem hiding this comment.
Typographical Error in Comment
Ohayo, sensei! There's a minor typo in the comment. The word "the" is duplicated. Please remove the extra "the" for clarity.
resolves #2405
ref #2524
depends on #2541
deploy the STRK fee token by default. all the generated predeployed accounts will be prefunded in both ETH and STRK.
Summary by CodeRabbit
Release Notes
New Features
ChainSpecstructure for enhanced blockchain configuration.SealedBlockWithStatusstruct for improved block representation.InvokeTx::V3.Improvements
Bug Fixes