feat(katana): state trie #2607
feat(katana): state trie #2607
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2607 +/- ##
==========================================
- Coverage 55.12% 54.92% -0.21%
==========================================
Files 385 392 +7
Lines 47775 48121 +346
==========================================
+ Hits 26337 26429 +92
- Misses 21438 21692 +254 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo! This pull request introduces significant changes across several Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (14)
crates/katana/trie/Cargo.toml (1)
8-17: Consider using workspace inheritance for slab dependency.Ohayo sensei! While most dependencies properly use workspace inheritance, the
slab = "0.4.9"dependency has a hard-coded version. This could lead to version inconsistencies if slab is used across multiple crates in the workspace.Consider moving the slab dependency to the workspace root and using
.workspace = truehere for consistent version management:-slab = "0.4.9" +slab.workspace = truecrates/katana/storage/provider/src/traits/trie.rs (3)
1-8: Add module-level documentation.Ohayo! Consider adding a module-level documentation comment (
//!) explaining the purpose of these traits and their role in the state trie implementation.+//! Traits for implementing state trie writers for both class and contract states. +//! These traits are used by the blockchain provider to maintain the global state root. + use std::collections::BTreeMap;
10-17: Document trait and method specifications.The trait implementation looks solid, sensei! However, it would benefit from comprehensive documentation:
#[auto_impl::auto_impl(&, Box, Arc)] +/// Trait for writing class updates to a trie structure and computing state roots. pub trait ClassTrieWriter: Send + Sync { + /// Inserts class hash updates for a given block and computes the new state root. + /// + /// # Arguments + /// * `block_number` - The block number for which these updates apply + /// * `updates` - Map of class hashes to their compiled class hashes + /// + /// # Returns + /// * `ProviderResult<Felt>` - The new state root after applying the updates + /// + /// # Errors + /// Returns error if the trie operation fails or if there are invalid state transitions fn insert_updates( &self, block_number: BlockNumber, updates: &BTreeMap<ClassHash, CompiledClassHash>, ) -> ProviderResult<Felt>; }
1-26: Consider future extensibility of the trie system.Ohayo sensei! The trait design looks solid, but here are some architectural considerations:
- Consider adding a common base trait for shared trie operations
- You might want to add methods for trie verification and root computation without updates
- Consider adding batch operation support for better performance
Would you like assistance in designing these extensions?
crates/katana/storage/db/Cargo.toml (1)
28-30: Consider version constraints for new dependencies.Ohayo sensei! While
bitvecfollows workspace versioning,smallvechas a hard-coded version "1.13.2". For consistency:-smallvec = "1.13.2" +smallvec.workspace = trueThis would help maintain version consistency across the workspace and make updates easier.
crates/katana/storage/db/src/models/trie.rs (2)
7-21: Ohayo! Consider adding documentation for public types.The type definitions look clean, but as public API elements, they would benefit from documentation comments explaining their purpose and usage.
Add rustdoc comments like this:
+/// Represents the different types of keys that can be stored in the trie database #[repr(u8)] #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)] pub enum TrieDatabaseKeyType { Trie = 0, Flat, TrieLog, } +/// A composite key used in the trie database, consisting of a type and the actual key bytes #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct TrieDatabaseKey { pub r#type: TrieDatabaseKeyType, pub key: Vec<u8>, }
54-81: Enhance test coverage with error cases.The current tests cover the happy path well, but we should also test error handling.
Add these test cases:
#[test] fn test_decode_empty_buffer() { let result = TrieDatabaseKey::decode(vec![]); assert!(matches!(result, Err(CodecError::InvalidInput(_)))); } #[test] fn test_decode_invalid_type() { let result = TrieDatabaseKey::decode(vec![255, 1, 2, 3]); assert!(matches!(result, Err(CodecError::InvalidInput(_)))); }crates/katana/storage/db/src/lib.rs (1)
16-16: Ohayo sensei! The trie module addition looks good! ✨The new
triemodule aligns perfectly with the PR's objective of implementing state trie functionality for computing the global state root. Its placement alongside other core modules liketables,models, andcodecsfollows the established architectural patterns.Consider documenting the module's purpose and its relationship with other modules in the codebase, especially its interaction with the existing database abstraction layer.
crates/katana/storage/provider/src/lib.rs (1)
399-410: Solid implementation of ContractTrieWriter, sensei!The implementation maintains consistency with the ClassTrieWriter pattern and properly handles state updates for contract trie computation.
A few architectural considerations:
- The returned Felt is likely the root hash, which could be used for state verification
- This implementation enables efficient state root computation as part of the PR objectives
- The delegation pattern allows for future optimizations in the underlying provider
Consider adding documentation comments that explain:
- The relationship between class and contract tries
- The significance of the returned Felt value
- The impact on global state root computation
crates/katana/storage/db/src/tables.rs (1)
173-176: Consider adding documentation for the trie tables, sensei!While the table definitions are correct, they would benefit from documentation explaining their specific roles in state management:
- What kind of data each trie stores
- How they relate to the global state root computation
- Any specific constraints or invariants
Apply this diff to add documentation:
- /// Class trie + /// Stores the merkle patricia trie nodes for class state. + /// Used for computing the global state root for declared classes. ClassTrie: (TrieDatabaseKey) => TrieDatabaseValue, - /// Contract trie + /// Stores the merkle patricia trie nodes for contract state. + /// Used for computing the global state root for deployed contracts. ContractTrie: (TrieDatabaseKey) => TrieDatabaseValue, - /// Contract storage trie + /// Stores the merkle patricia trie nodes for contract storage state. + /// Used for computing the global state root for contract storage. ContractStorageTrie: (TrieDatabaseKey) => TrieDatabaseValueAlso applies to: 233-238
crates/katana/storage/provider/src/providers/fork/mod.rs (1)
589-611: Consider documenting the implementation strategy, senseiBoth
ClassTrieWriterandContractTrieWriterimplementations appear to be placeholders. For better maintainability and clarity, consider:
- Adding TODO comments explaining the implementation plan
- Documenting whether the actual implementation belongs in a different provider
- Creating GitHub issues to track the actual implementation tasks
- Adding tests to verify the expected behavior once implemented
This will help other developers understand the implementation strategy and track progress.
crates/katana/storage/db/src/trie/class.rs (1)
40-40: Ohayo sensei! Clarify the key derivation logicThe slicing operation
[5..]on the bit representation of the hash may not be immediately clear. Adding a comment to explain why the first five bits are skipped would enhance code readability.You could add an explanatory comment like this:
let key: BitVec<u8, Msb0> = hash.to_bytes_be().as_bits()[5..].to_owned(); +// Skip the first 5 bits to align with the specific trie key formatcrates/katana/trie/src/lib.rs (2)
24-24: Consider using a more descriptiveIDENTIFIERconstant.Ohayo sensei, using a more meaningful
IDENTIFIER, instead ofb"1", can improve code readability and maintainability.
62-88: Enhance test coverage with additional unit tests.Ohayo sensei, consider adding more unit tests to cover edge cases and different input scenarios. This will ensure the robustness and reliability of the implemented functions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.toml(3 hunks)crates/katana/storage/db/Cargo.toml(2 hunks)crates/katana/storage/db/src/codecs/postcard.rs(2 hunks)crates/katana/storage/db/src/lib.rs(1 hunks)crates/katana/storage/db/src/models/mod.rs(1 hunks)crates/katana/storage/db/src/models/trie.rs(1 hunks)crates/katana/storage/db/src/tables.rs(7 hunks)crates/katana/storage/db/src/trie/class.rs(1 hunks)crates/katana/storage/db/src/trie/contract.rs(1 hunks)crates/katana/storage/db/src/trie/mod.rs(1 hunks)crates/katana/storage/provider/Cargo.toml(1 hunks)crates/katana/storage/provider/src/lib.rs(3 hunks)crates/katana/storage/provider/src/providers/db/mod.rs(1 hunks)crates/katana/storage/provider/src/providers/db/trie.rs(1 hunks)crates/katana/storage/provider/src/providers/fork/mod.rs(4 hunks)crates/katana/storage/provider/src/traits/mod.rs(1 hunks)crates/katana/storage/provider/src/traits/trie.rs(1 hunks)crates/katana/trie/Cargo.toml(1 hunks)crates/katana/trie/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/katana/storage/db/src/models/mod.rs
- crates/katana/storage/provider/src/providers/db/mod.rs
- crates/katana/storage/provider/src/traits/mod.rs
🔇 Additional comments (19)
crates/katana/trie/Cargo.toml (2)
1-7: LGTM! Package metadata follows workspace conventions.
Ohayo! The package metadata section properly inherits workspace-level configurations while maintaining package-specific name. This is the recommended approach for workspace management.
19-23: Verify git dependency revision and consider vendoring.
The bonsai-trie dependency is fetched from a git repository with a specific revision. While this works, it has several implications:
- Build reproducibility depends on the git repository's availability
- The specific revision
56d7d62needs verification for security and stability - Future maintenance might be affected if the repository becomes unavailable
Let's verify the git dependency:
Consider either:
- Vendoring the dependency in your repository if the upstream is not actively maintained
- Using a published version from crates.io if available
- Adding a comment explaining why this specific revision is needed
✅ Verification successful
Let me gather more information about the dependency and its maintenance status.
Let me check one more thing about the dependency usage in the codebase.
Ohayo sensei! Git dependency is actively maintained but needs documentation
The bonsai-trie repository shows healthy maintenance:
- Active development with recent commits (last commit Oct 23, 2024)
- Specific revision
56d7d62is a stable commit with core functionality - Repository is part of the madara-alliance organization, suggesting official maintenance
However, recommend adding a comment in Cargo.toml explaining why this specific revision is needed for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the bonsai-trie git dependency
# Test: Check if the repository and commit are accessible
gh repo view madara-alliance/bonsai-trie --json url,description
gh api repos/madara-alliance/bonsai-trie/commits/56d7d62 --jq '.sha'
Length of output: 310
Script:
#!/bin/bash
# Check recent activity and maintenance status
gh api repos/madara-alliance/bonsai-trie/commits --jq 'length'
gh api repos/madara-alliance/bonsai-trie/commits --jq '.[0:3] | .[] | {sha: .sha[0:7], date: .commit.author.date, message: .commit.message}'
# Check if it's published on crates.io
curl -s https://crates.io/api/v1/crates/bonsai-trie | jq '.crate?.max_version'
# Check for any cargo.toml comments about this dependency
rg -A 2 "bonsai-trie" "$(fd -t f Cargo.toml)"
Length of output: 4180
Script:
#!/bin/bash
# Check for any comments or documentation about bonsai-trie usage
rg -B 2 -A 2 "bonsai-trie" --type rust
# Check the specific commit's changes
gh api repos/madara-alliance/bonsai-trie/commits/56d7d62 --jq '.files[].filename'
Length of output: 566
crates/katana/storage/db/Cargo.toml (2)
11-11: LGTM! Trie dependency addition aligns with PR objectives.
Ohayo! The addition of katana-trie as a workspace dependency perfectly aligns with the PR's goal of implementing class and contract trie functionality.
21-22: Verify Starknet dependency migration impact.
The starknet dependency has been moved from dev-dependencies to main dependencies, and starknet-types-core has been added. While this makes sense for the trie implementation, we should verify the impact.
✅ Verification successful
Let me check the actual usage of these dependencies in the storage/db crate to verify if they are correctly placed in main dependencies.
Let me check the actual content of these files to understand how these dependencies are used.
Ohayo sensei! After analyzing the usage, I can now provide a conclusive response:
Dependencies are correctly placed in main dependencies ✨
The starknet and starknet-types-core dependencies are used in core functionality of the storage/db crate:
- Used in contract and class trie implementations
- Required for hash computations using
Poseidon - Used in storage models and table definitions
- None of the usages are test-only
These dependencies are essential for the core functionality of the storage/db crate, particularly in the trie implementation and hash computations, so they are correctly placed in the main dependencies section rather than dev-dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if starknet is used in test files only
# If it's only used in tests, it should remain in dev-dependencies
rg -l "use starknet::" --type rust
Length of output: 8623
Script:
#!/bin/bash
# Check actual usage of starknet and starknet-types-core in the storage/db crate
rg -l "use starknet::|use starknet_types_core::" "crates/katana/storage/db/src/"
Length of output: 296
Script:
#!/bin/bash
# Get the content of relevant files to understand the usage context
ast-grep --pattern 'use starknet::$$$' "crates/katana/storage/db/src/trie/class.rs"
Length of output: 83
Script:
#!/bin/bash
# Check if there are any test files using these dependencies
find crates/katana/storage/db/src -type f -name "*test*.rs" -exec rg -l "use starknet::|use starknet_types_core::" {} \;
Length of output: 125
Script:
#!/bin/bash
# Check the actual usage in the files
rg "use starknet::|use starknet_types_core::" -A 5 "crates/katana/storage/db/src/trie/contract.rs" "crates/katana/storage/db/src/trie/class.rs" "crates/katana/storage/db/src/tables.rs" "crates/katana/storage/db/src/models/storage.rs" "crates/katana/storage/db/src/mdbx/mod.rs"
Length of output: 2533
crates/katana/storage/provider/Cargo.toml (2)
12-12: LGTM! Well-organized dependency additions.
Ohayo! The new dependencies are properly organized following the workspace-first pattern, and all are correctly using workspace versions. Nice work, sensei!
Also applies to: 20-21
12-12: Verify version compatibility with existing dependencies.
Let's ensure these new dependencies don't introduce any version conflicts in the workspace.
Also applies to: 20-21
✅ Verification successful
Let me gather more information to verify workspace dependencies.
Dependencies are properly configured in workspace sensei!
The verification shows that:
katana-trieis correctly defined in workspace dependencies and used with.workspace = truebitvecandstarknet-types-coreare also using workspace inheritance with.workspace = true- All dependencies are properly managed at the workspace level with no version conflicts
Ohayo! Everything looks perfectly aligned with the workspace configuration, sensei! 🍜
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential version conflicts in workspace dependencies
# Test: Look for any duplicate dependency versions in workspace
rg -A 1 "^\[dependencies\]|^(katana-trie|bitvec|starknet-types-core)\s*=" Cargo.toml **/Cargo.toml
# Test: Check if workspace versions are properly defined
rg "^\[workspace\.dependencies\]" -A 20 Cargo.toml
Length of output: 1117
Script:
#!/bin/bash
# Check workspace dependencies in the specific crate's Cargo.toml
cat crates/katana/storage/provider/Cargo.toml
# Check for katana-trie in workspace dependencies
rg "katana-trie" Cargo.toml
# Look for any version specifications of these dependencies in workspace
rg "^\[workspace\.dependencies\]" -A 50 Cargo.toml
Length of output: 3687
crates/katana/storage/db/src/codecs/postcard.rs (2)
14-14: LGTM! Clean import addition.
Ohayo! The import follows the existing grouping pattern nicely, sensei.
42-42: LGTM! Consistent implementation for TrieDatabaseValue.
Ohayo! The addition of TrieDatabaseValue to the macro implementation list is well-placed. The implementation will provide consistent serialization behavior with proper error handling through postcard.
Let's verify the TrieDatabaseValue type definition:
✅ Verification successful
Ohayo! The TrieDatabaseValue implementation is properly integrated and used consistently
The verification confirms that TrieDatabaseValue is:
- Defined as a ByteVec type alias in
models/trie.rs - Used consistently in the Trie trait and table definitions
- Properly imported in the postcard codec implementation
The implementation aligns perfectly with the database's type system and serialization requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TrieDatabaseValue type definition and usage
# Expected: Find the type definition and any direct usage
# Search for type definition
echo "=== Searching for TrieDatabaseValue type definition ==="
rg -p "type\s+TrieDatabaseValue"
# Search for usage patterns
echo "=== Searching for TrieDatabaseValue usage patterns ==="
rg -p "TrieDatabaseValue"
Length of output: 965
crates/katana/storage/db/src/models/trie.rs (1)
23-32: LGTM! Clean and efficient encoding implementation.
The encoding logic is straightforward and correctly preserves both the type and key data.
Cargo.toml (2)
38-38: Ohayo! New trie module addition looks good, sensei!
The addition of "crates/katana/trie" to workspace members aligns perfectly with the PR objectives for implementing the state trie functionality.
259-261: Dependencies are well-chosen for trie implementation!
The changes look good:
- Upgrading
starknet-types-coreand adding the "hash" feature is appropriate for trie operations - Adding
bitvecis a solid choice for efficient bit manipulation in trie implementations
However, let's verify the dependency usage:
✅ Verification successful
Dependencies are properly utilized in trie implementation, sensei!
The verification confirms both dependencies are actively used in the implementation:
bitvec: Used for key representation in the trie withBitVec::from_iterstarknet-types-corehash features: Extensively used for Pedersen hashing in contract state computation and merkle root calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new dependencies in the trie implementation
# Check for bitvec usage in the new trie module
echo "Checking bitvec usage:"
rg -l "use.*bitvec" "crates/katana/trie"
# Check for starknet_types_core hash feature usage
echo "Checking starknet_types_core hash feature usage:"
rg -l "use.*starknet_types_core.*hash" "crates/katana/trie"
Length of output: 322
Script:
#!/bin/bash
# Let's check the actual usage patterns of these dependencies in the trie implementation
echo "Checking specific bitvec usage patterns:"
rg "use.*bitvec" -A 5 "crates/katana/trie/src/lib.rs"
echo -e "\nChecking starknet_types_core hash usage patterns:"
rg "use.*starknet_types_core.*hash" -A 5 "crates/katana/trie/src/lib.rs"
echo -e "\nChecking actual usage of bitvec types:"
rg "BitVec|BitSlice" "crates/katana/trie/src/lib.rs"
echo -e "\nChecking actual usage of hash features:"
rg "Hash|hash" "crates/katana/trie/src/lib.rs"
Length of output: 2503
crates/katana/storage/provider/src/lib.rs (1)
22-22: Ohayo! Import looks good, sensei!
The import of trie-related traits aligns perfectly with the PR's objective of implementing class and contract trie functionality.
crates/katana/storage/db/src/tables.rs (3)
13-13: Ohayo! The Trie trait definition looks clean and well-constrained.
The trait is properly bounded to specific key and value types, which ensures type safety and consistency across implementations.
Also applies to: 39-40
241-243: LGTM! Clean trait implementations.
The Trie trait implementations for the three new table types are concise and correct.
276-277: Verify test coverage for trie tables, sensei!
While the basic table existence tests are added, consider enhancing the test coverage:
- Add compress/decompress tests for
TrieDatabaseValue - Add encode/decode tests for
TrieDatabaseKey
Let's verify if these types are already tested elsewhere:
If not found, consider adding these test cases to test_key_encode_decode and test_value_compress_decompress:
assert_key_encode_decode! {
(BlockNumber, 100),
(BlockHash, felt!("0x123456789")),
+ (TrieDatabaseKey, TrieDatabaseKey::default()),
// ... other cases
}
assert_value_compress_decompress! {
(Header, Header::default()),
(BlockHash, BlockHash::default()),
+ (TrieDatabaseValue, TrieDatabaseValue::default()),
// ... other cases
}Also applies to: 302-303
crates/katana/storage/provider/src/providers/fork/mod.rs (3)
4-4: Ohayo! The new imports look good, sensei!
The additions of BTreeMap, Felt, and trie-related traits align perfectly with the PR's objective of implementing class and contract tries for state root computation.
Also applies to: 20-20, 41-41
601-611: Verify if this is intended to be a placeholder implementation, sensei
Similar to the ClassTrieWriter, the ContractTrieWriter::insert_updates implementation doesn't perform any state root computation. Both trait implementations appear to be placeholders.
If you need assistance implementing the actual trie operations and state root computation, I'd be happy to help!
#!/bin/bash
# Check if there are any TODO comments or related issues about implementing this properly
rg -g '!target' -g '!*.lock' "TODO.*contract.*trie|FIXME.*contract.*trie" .
# Check if there are any other implementations of ContractTrieWriter that might be the actual implementation
ast-grep --pattern 'impl ContractTrieWriter for $_ {
$$$
}'589-599: Verify if this is intended to be a placeholder implementation, sensei
The current implementation of ClassTrieWriter::insert_updates doesn't perform any state root computation and simply returns Felt::ZERO. This seems to diverge from the PR's objective of implementing class trie for state root computation.
crates/katana/storage/provider/src/providers/db/trie.rs (1)
108-108: Ohayo sensei! Verify that defaulting to zero is appropriate when provider returns None
When the provider returns None for nonce or class_hash, the code defaults to Felt::default(). Please confirm that defaulting to zero is acceptable in these cases and will not lead to incorrect state computations.
Also applies to: 112-112
| #[auto_impl::auto_impl(&, Box, Arc)] | ||
| pub trait ContractTrieWriter: Send + Sync { | ||
| fn insert_updates( | ||
| &self, | ||
| block_number: BlockNumber, | ||
| state_updates: &StateUpdates, | ||
| ) -> ProviderResult<Felt>; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! After analyzing the implementation details, I can now provide a final response about the ContractTrieWriter trait.
Documentation looks good, but implementation needs validation
The trait implementation shows two concerning patterns:
- ForkedProvider has a no-op implementation that ignores state updates and returns ZERO
- DbProvider has a real implementation but lacks validation of the state updates
Here's what needs attention:
- Add validation of state updates in DbProvider before processing them
- Add error handling for invalid state transitions
- Consider adding validation tests for the contract trie writer
🔗 Analysis chain
Document trait and verify state update handling.
The trait looks good, but needs documentation and potential verification:
#[auto_impl::auto_impl(&, Box, Arc)]
+/// Trait for writing contract state updates to a trie structure and computing state roots.
pub trait ContractTrieWriter: Send + Sync {
+ /// Inserts contract state updates for a given block and computes the new state root.
+ ///
+ /// # Arguments
+ /// * `block_number` - The block number for which these updates apply
+ /// * `state_updates` - Collection of state updates to be applied
+ ///
+ /// # Returns
+ /// * `ProviderResult<Felt>` - The new state root after applying the updates
+ ///
+ /// # Errors
+ /// Returns error if the trie operation fails or if there are invalid state transitions
fn insert_updates(
&self,
block_number: BlockNumber,
state_updates: &StateUpdates,
) -> ProviderResult<Felt>;
}Let's verify the StateUpdates type definition and its handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StateUpdates type definition and usage
# Expected: Find the type definition and any validation logic
# Search for StateUpdates type definition
echo "Searching for StateUpdates definition..."
rg -t rust "struct StateUpdates" -A 10
# Search for any validation logic
echo "Searching for validation logic..."
rg -t rust "impl.*StateUpdates" -A 10
Length of output: 3066
Script:
#!/bin/bash
# Let's check for any validation or error handling in the ContractTrieWriter implementations
# Search for ContractTrieWriter implementations and their error handling
echo "Searching for ContractTrieWriter implementations..."
rg -t rust "impl.*ContractTrieWriter" -B 2 -A 15
# Search for any ProviderResult error definitions
echo "Searching for ProviderResult error definitions..."
rg -t rust "ProviderResult|ProviderError" -A 5
# Check if there are any tests validating state updates
echo "Searching for related tests..."
rg -t rust "test.*insert_updates|test.*ContractTrieWriter" -B 2 -A 10
Length of output: 180029
Script:
#!/bin/bash
# Let's check for any validation or error handling in the implementation
# and any tests related to state updates
# Search for state update validation logic
echo "Searching for state update validation..."
rg -t rust "fn.*validate.*StateUpdates" -B 2 -A 10
# Search for state update error handling
echo "Searching for state update error handling..."
rg -t rust "Error.*StateUpdates|StateUpdates.*Error" -B 2 -A 10
# Search for contract trie writer tests in test modules
echo "Searching for contract trie tests..."
ast-grep --pattern 'mod tests {
$$$
#[test]
fn $_($$) {
$$$
ContractTrieWriter
$$$
}
$$$
}'
Length of output: 1617
Script:
#!/bin/bash
# Let's check the implementation details of ContractTrieWriter
# Search for ContractTrieWriter implementations
echo "Searching for ContractTrieWriter implementations..."
rg -t rust "impl.*ContractTrieWriter" -B 5 -A 15
# Search for state update handling in the trie module
echo "Searching for state update handling in trie..."
rg -t rust "fn insert.*state_updates.*StateUpdates" -B 5 -A 15
# Search for any test files in the trie module
echo "Searching for trie test files..."
fd -t f "trie.*test.*rs$"
Length of output: 4745
| impl Decode for TrieDatabaseKey { | ||
| fn decode<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> { | ||
| let bytes = bytes.as_ref(); | ||
| if bytes.is_empty() { | ||
| panic!("emptyy buffer") | ||
| } | ||
|
|
||
| let r#type = match bytes[0] { | ||
| 0 => TrieDatabaseKeyType::Trie, | ||
| 1 => TrieDatabaseKeyType::Flat, | ||
| 2 => TrieDatabaseKeyType::TrieLog, | ||
| _ => panic!("Invalid trie database key type"), | ||
| }; | ||
|
|
||
| let key = bytes[1..].to_vec(); | ||
|
|
||
| Ok(TrieDatabaseKey { r#type, key }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and code clarity, sensei!
A few suggestions to enhance robustness and maintainability:
- Fix the typo in the panic message
- Consider using Result instead of panic for error cases
- Use enum variants directly in the match statement
Here's how to improve it:
impl Decode for TrieDatabaseKey {
fn decode<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> {
let bytes = bytes.as_ref();
if bytes.is_empty() {
- panic!("emptyy buffer")
+ return Err(CodecError::InvalidInput("empty buffer".into()));
}
let r#type = match bytes[0] {
- 0 => TrieDatabaseKeyType::Trie,
- 1 => TrieDatabaseKeyType::Flat,
- 2 => TrieDatabaseKeyType::TrieLog,
- _ => panic!("Invalid trie database key type"),
+ x if x == TrieDatabaseKeyType::Trie as u8 => TrieDatabaseKeyType::Trie,
+ x if x == TrieDatabaseKeyType::Flat as u8 => TrieDatabaseKeyType::Flat,
+ x if x == TrieDatabaseKeyType::TrieLog as u8 => TrieDatabaseKeyType::TrieLog,
+ _ => return Err(CodecError::InvalidInput("invalid trie database key type".into())),
};📝 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.
| impl Decode for TrieDatabaseKey { | |
| fn decode<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> { | |
| let bytes = bytes.as_ref(); | |
| if bytes.is_empty() { | |
| panic!("emptyy buffer") | |
| } | |
| let r#type = match bytes[0] { | |
| 0 => TrieDatabaseKeyType::Trie, | |
| 1 => TrieDatabaseKeyType::Flat, | |
| 2 => TrieDatabaseKeyType::TrieLog, | |
| _ => panic!("Invalid trie database key type"), | |
| }; | |
| let key = bytes[1..].to_vec(); | |
| Ok(TrieDatabaseKey { r#type, key }) | |
| } | |
| } | |
| impl Decode for TrieDatabaseKey { | |
| fn decode<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> { | |
| let bytes = bytes.as_ref(); | |
| if bytes.is_empty() { | |
| return Err(CodecError::InvalidInput("empty buffer".into())); | |
| } | |
| let r#type = match bytes[0] { | |
| x if x == TrieDatabaseKeyType::Trie as u8 => TrieDatabaseKeyType::Trie, | |
| x if x == TrieDatabaseKeyType::Flat as u8 => TrieDatabaseKeyType::Flat, | |
| x if x == TrieDatabaseKeyType::TrieLog as u8 => TrieDatabaseKeyType::TrieLog, | |
| _ => return Err(CodecError::InvalidInput("invalid trie database key type".into())), | |
| }; | |
| let key = bytes[1..].to_vec(); | |
| Ok(TrieDatabaseKey { r#type, key }) | |
| } | |
| } |
| impl<Db> ClassTrieWriter for BlockchainProvider<Db> | ||
| where | ||
| Db: ClassTrieWriter, | ||
| { | ||
| fn insert_updates( | ||
| &self, | ||
| block_number: BlockNumber, | ||
| updates: &BTreeMap<ClassHash, CompiledClassHash>, | ||
| ) -> ProviderResult<Felt> { | ||
| self.provider.insert_updates(block_number, updates) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Found an issue with the ForkedProvider implementation, sensei!
The ForkedProvider implementation of ClassTrieWriter is not properly handling the updates - it's ignoring the inputs and always returning Felt::ZERO. This could lead to incorrect class trie computation in forked environments.
crates/katana/storage/provider/src/providers/fork/mod.rs: The implementation discards bothblock_numberandupdatesparameters usinglet _ = ...and returns a constant zero value.
🔗 Analysis chain
Clean implementation of ClassTrieWriter, sensei!
The implementation follows the established delegation pattern and correctly handles the class hash updates for trie computation. Let's verify the trait implementation in the underlying provider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ClassTrieWriter trait implementation in the underlying provider
# Expected: Find the trait implementation details in the trie module
# Search for the ClassTrieWriter trait definition and implementation
ast-grep --pattern 'trait ClassTrieWriter {
$$$
}'
# Search for the implementation in the db provider
ast-grep --pattern 'impl ClassTrieWriter for $_ {
$$$
}'
Length of output: 1081
Script:
#!/bin/bash
# Let's search for the trait definition and any other implementations
rg -A 5 "trait ClassTrieWriter"
# Also search for any other implementations
rg -A 10 "impl ClassTrieWriter for"
# Let's also check if there are any tests or documentation
rg -A 5 "ClassTrieWriter.*test"
Length of output: 1526
| pub fn insert(&mut self, hash: ClassHash, compiled_hash: CompiledClassHash) { | ||
| let value = Poseidon::hash(&CONTRACT_CLASS_LEAF_V0, &compiled_hash); | ||
| let key: BitVec<u8, Msb0> = hash.to_bytes_be().as_bits()[5..].to_owned(); | ||
| self.inner.insert(self.bonsai_identifier(), &key, &value).unwrap(); | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Handle errors when inserting into the trie
Using unwrap() in self.inner.insert(...).unwrap(); may lead to a panic if the insertion fails. It's better to handle the error or propagate it to ensure the stability of the application.
Consider modifying the code to handle potential errors:
pub fn insert(&mut self, hash: ClassHash, compiled_hash: CompiledClassHash) {
let value = Poseidon::hash(&CONTRACT_CLASS_LEAF_V0, &compiled_hash);
let key: BitVec<u8, Msb0> = hash.to_bytes_be().as_bits()[5..].to_owned();
- self.inner.insert(self.bonsai_identifier(), &key, &value).unwrap();
+ if let Err(e) = self.inner.insert(self.bonsai_identifier(), &key, &value) {
+ // Handle the error appropriately, e.g., log it or return it
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| pub fn root(&self) -> Felt { | ||
| self.inner.root_hash(self.bonsai_identifier()).unwrap() | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Ensure proper error handling in the root method
Using unwrap() in self.inner.root_hash(...).unwrap(); might lead to a panic if retrieving the root hash fails. It's recommended to handle the potential error to prevent crashes.
Modify the code to handle the error:
pub fn root(&self) -> Felt {
- self.inner.root_hash(self.bonsai_identifier()).unwrap()
+ match self.inner.root_hash(self.bonsai_identifier()) {
+ Ok(root) => root,
+ Err(e) => {
+ // Handle the error, possibly returning a default value or propagating the error
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| fn get_by_prefix( | ||
| &self, | ||
| prefix: &DatabaseKey<'_>, | ||
| ) -> Result<Vec<(ByteVec, ByteVec)>, Self::DatabaseError> { | ||
| let _ = prefix; | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Implement the get_by_prefix method
Ohayo sensei! The get_by_prefix method is marked as todo!() and needs to be implemented to retrieve values by a given prefix. I can assist with implementing this functionality if you'd like.
| type DatabaseError = Error; | ||
| type Transaction = TrieDb<Tb, Tx>; | ||
|
|
||
| fn snapshot(&mut self, _: BasicId) {} |
There was a problem hiding this comment.
Implement the snapshot method
Ohayo sensei! The snapshot method is currently empty and should be implemented to support snapshot functionality for transactions. Happy to help implement this if needed.
| fn merge(&mut self, _: Self::Transaction) -> Result<(), Self::DatabaseError> { | ||
| todo!(); | ||
| } |
There was a problem hiding this comment.
Implement the merge method
Ohayo sensei! The merge method is currently marked as todo!() and requires implementation to handle merging of transactions. I can help with this implementation if needed.
| fn transaction(&self, _: BasicId) -> Option<Self::Transaction> { | ||
| todo!(); | ||
| } |
There was a problem hiding this comment.
Implement the transaction method
Ohayo sensei! The transaction method is currently marked as todo!() and needs to be implemented to support transaction handling. Let me know if you'd like assistance with this.
| fn remove_by_prefix(&mut self, prefix: &DatabaseKey<'_>) -> Result<(), Self::DatabaseError> { | ||
| let mut cursor = self.tx.cursor_mut::<Tb>()?; | ||
| let walker = cursor.walk(None)?; | ||
|
|
||
| let mut keys_to_remove = Vec::new(); | ||
| // iterate over all entries in the table | ||
| for entry in walker { | ||
| let (key, _) = entry?; | ||
| if key.key.starts_with(prefix.as_slice()) { | ||
| keys_to_remove.push(key); | ||
| } | ||
| } | ||
|
|
||
| for key in keys_to_remove { | ||
| let _ = self.tx.delete::<Tb>(key, None)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize remove_by_prefix to improve performance
Ohayo sensei! Currently, remove_by_prefix iterates over all entries in the table, which might be inefficient for large datasets. Consider using cursor.seek to directly navigate to the prefix and iterate from there to improve performance.
Here's how you might adjust the code:
fn remove_by_prefix(&mut self, prefix: &DatabaseKey<'_>) -> Result<(), Self::DatabaseError> {
let mut cursor = self.tx.cursor_mut::<Tb>()?;
- let walker = cursor.walk(None)?;
+ let walker = cursor.walk(Some(to_db_key(prefix)))?;
let mut keys_to_remove = Vec::new();
// iterate over entries starting from the prefix
for entry in walker {
let (key, _) = entry?;
if key.key.starts_with(prefix.as_slice()) {
keys_to_remove.push(key);
} else {
break;
}
}
for key in keys_to_remove {
let _ = self.tx.delete::<Tb>(key, None)?;
}
Ok(())
}Committable suggestion skipped: line range outside the PR's diff.
implement class and contract trie for computing the global state root using bonsai-trie