feat(katana-rpc-types): untagged enum #2820
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
crates/katana/storage/provider/src/providers/fork/mod.rs (1)
Line range hint
613-621: Critical: Implement proper trie updates for contracts.Similar to the declared classes implementation, this method ignores its parameters and returns
ZERO. This needs to be properly implemented to ensure correct state management.Would you like me to help implement the proper trie update logic for both contract updates and declared classes?
🧹 Nitpick comments (55)
crates/katana/trie/src/storages.rs (2)
8-11: Ohayo sensei! Ensure consistent naming and clarity in your docstrings.This struct name (StoragesTrie) is perfectly descriptive. However, adding a brief docstring explaining its role in storing per-contract storage tries could be beneficial for new contributors.
22-29: Ohayo sensei! Check multiproof usage and edge cases.
- This logic is correct for building a multiproof. However, watch out for scenarios where an empty list of storage keys is passed in. Verify how the Bonsai trie handles an empty multiproof.
- The debug print (dbg!(&keys)) is typically removed in production code. Though it’s sometimes helpful for development, consider removing it or gating it behind a debug feature.
Would you like a quick patch removing the debug print for production builds?
crates/katana/trie/src/contracts.rs (3)
8-10: Ohayo sensei! Document the separation of contract vs. storage tries.Both the name and structure are self-evident. However, consider adding a short docstring clarifying that this trie is specifically for storing active contracts rather than contract storage.
25-29: Ohayo sensei! Remove or hide debugging statements in multiproof.Line 27 has a dbg! macro. Typically, debug prints are stripped out for production. While this can be helpful during development, it can clutter logs in a live environment.
- dbg!(&keys);
40-42: Ohayo sensei! Nice commit structure.Everything looks correct. If your system expands to multiple tries and each tries to commit at the same time, consider whether you need atomic commits across all tries.
crates/katana/trie/src/classes.rs (2)
10-13: Ohayo sensei! ClassesTrie is consistent with other trie structures.Nice approach to unify the pattern for classes, contracts, and storages. Keep an eye on duplication—shared logic could be refactored into a helper trait or function if necessary.
42-44: Ohayo sensei! Another straightforward commit method.No apparent issues. If you plan on supporting offline merges or forks, a multi-step commit approach or conflict resolution may be needed in the future.
crates/katana/storage/provider/src/traits/state.rs (2)
14-20: Ohayo sensei! Great simplification removing the block ID from state_root.
- This approach is more direct since you always compute the global state root.
- If you reintroduce block-specific logic later, ensure each block has a snapshot or a separate instance to re-derive the root if needed.
85-95: Ohayo sensei! Splitting out the StateProofProvider is a clean approach.
- The three multiproof methods keep the responsibilities distinct.
- Carefully handle large vectors for multiproof calls—either by limiting the maximum or streaming out partial proofs for performance reasons.
[performance]
crates/katana/storage/db/src/models/trie.rs (3)
7-11: Ohayo sensei! Great job introducing TrieHistoryEntry.
The struct’s clarity is commendable. It might be helpful to add doc comments to describe when it is used and how.
13-22: Ohayo sensei! The compression strategy is succinct.
Consider prefixing the encoded length of the entire compressed result before concatenation to facilitate potential streaming decompression.
59-59: Ohayo sensei! Encoding the key length as a single byte is a potential limitation.
Consider switching to a variable-length integer for robust expansion if keys exceed 255 bytes.crates/katana/storage/db/src/trie/snapshot.rs (6)
15-23: Ohayo sensei! Good snapshot structure.
Doc comments on SnapshotTrieDb usage would help other devs understand its immutability constraints.
59-67: Ohayo sensei! create_batch is empty.
This is consistent with the read-only snapshot approach, but consider a doc comment clarifying why it’s unsupported.
69-71: Ohayo sensei! remove_by_prefix is unimplemented.
Again, it’s correct for a read-only snapshot. Document the usage constraints in method docs.
92-97: Ohayo sensei! get_by_prefix is a TODO.
Would you like help implementing partial key queries?
99-106: Ohayo sensei! insert is unimplemented for snapshots.
No issues, but a clear doc comment on read-only usage is recommended.
116-118: Ohayo sensei! contains is unimplemented.
Implementing it for read-only snapshots might be beneficial in the future if quick existence checks are needed.crates/katana/storage/provider/src/providers/db/trie.rs (5)
15-16: Ohayo sensei! Introducing the TrieWriter trait is a neat abstraction.
Consider finer-grained methods for partial updates if that’s relevant to performance.
31-40: Ohayo sensei! Committing the ClassesTrie after insertion is good.
If partial commits or rollbacks are needed, consider a separate error-handling path.
54-55: Ohayo sensei! Using a HashMap for ContractLeaf is clear.
If the expected usage requires concurrency or large scale, consider parallel structures.
56-64: Ohayo sensei! The logic for applying storage updates is correct.
No issues spotted. Possibly add diagnostic logs for debugging complex updates.
78-80: Ohayo sensei! Replacing classes is well-handled.
No issues. Possibly log major class changes for auditing.crates/katana/trie/src/lib.rs (4)
10-11: Ohayo sensei! Poseidon usage for hashing.
No problems spotted. Just keep an eye out for performance overhead if used very frequently under load.
18-21: Ohayo sensei! Public re-exports are well organized.
A short doc comment on each pub use could help readers discover these modules.
23-33: Ohayo sensei! BonsaiTrie struct.
The approach is neat. Possibly rename to BonsaiTrieShim for clarity that it’s a wrapper.
46-50: Ohayo sensei! root method is concise.
Ensure root_hash is not called too frequently in tight loops—any caching mechanism?crates/katana/rpc/rpc-types/src/trie.rs (2)
9-15: Struct well-defined, consider additional validationsThe ContractStorageKeys struct is straightforward. If you foresee a scenario where invalid keys might be passed, consider basic validations or checks.
68-75: ContractsProofStoring the nodes plus contract leaves data is a good approach. Consider verifying the size of contract_leaves_data to avoid large memory usage.
crates/katana/rpc/rpc/src/starknet/read.rs (1)
271-282: New get_storage_proof methodThis is a highly valuable addition for verifying on-chain data. The usage of an asynchronous approach is aligned with other methods. Good job sensei.
As these proofs can be large, consider adding timeouts or concurrency controls if there's a risk of blocking the endpoint for a while.
crates/katana/storage/provider/src/lib.rs (2)
22-22: Using StateWriterWriting states is a big deal. Provide tests ensuring newly inserted states remain consistent across restarts.
353-370: TrieWriter usageCombining declared classes and contract updates is a clear improvement to unify your trie management. The naming feels consistent (trie_insert_declared_classes, etc.).
Consider checks or metrics around these insert methods to ensure performance remains healthy for large updates.
crates/katana/storage/db/src/trie/mod.rs (2)
202-214: Ohayo sensei! Write cache usage is a good design, but keep an eye on memory growth.Using a HashMap to store key-value pairs in-memory could grow significantly for large-scale updates. Consider implementing partial flushing or chunking if you anticipate very large transactions.
321-367: Ohayo sensei! Merging logic is unimplemented.The merge(...) function is left unimplemented, which could lead to incomplete features if merges are required in production.
Would you like me to open an issue or provide a draft implementation?
crates/katana/storage/provider/src/providers/fork/state.rs (3)
105-124: Ohayo sensei! The multiproof stubs return empty proofs.This is acceptable for a placeholder, but in a scenario requiring real proofs, you may need a more complete implementation.
177-196: Ohayo sensei! Good fallback in LatestStateProvider.Same comment applies: returning MultiProof(Default::default()) is fine for stubs, but ensure you handle real proof logic in the future.
280-299: Ohayo sensei! The same pattern for snapshot multiproofs.These placeholders will need revisiting. If actual validation or bridging is required, updating the default MultiProof is essential.
crates/katana/storage/db/src/tables.rs (1)
56-56: Ohayo sensei! NUM_TABLES expanded to 33.Make sure you maintain this constant if you dynamically add or remove tables.
crates/katana/executor/src/implementation/blockifier/state.rs (2)
241-267: Ohayo sensei! Ensure unimplemented StateProofProvider methods won't be called unexpectedly.Currently, these methods are unimplemented and will panic if invoked. Consider returning an explicit error or removing them if not used.
269-277: Ohayo sensei! Handle unimplemented StateRootProvider methods carefully.Returning an error or fallback value will help avoid runtime panics for unsupported operations.
crates/katana/storage/provider/src/providers/fork/backend.rs (2)
570-593: Ohayo sensei! Add graceful handling for unimplemented multiproof logic.These placeholders could lead to unexpected panics if called. Consider explicit error messages or fallback logic.
595-603: Ohayo sensei! Consider a fallback for unimplemented root provider methods.Providing a stub response or returning an error can help avoid confusion.
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
1131-1210: Ohayo sensei! Large get_proofs method introduced.Consider splitting this logic into smaller helper functions for clarity, and watch for performance if many keys are requested.
crates/katana/trie/src/id.rs (1)
23-25: Consider documenting the byte representation format.The
to_bytes()implementation uses big-endian byte ordering. Consider adding a doc comment to make this explicit for future maintainers.+ /// Converts the CommitId to its byte representation using big-endian ordering. fn to_bytes(&self) -> ByteVec { ByteVec::from(&self.0.to_be_bytes() as &[_]) }crates/katana/storage/db/Cargo.toml (1)
Line range hint
25-25: Consider using workspace version for smallvec, sensei!The specific version pin
smallvec = "1.13.2"deviates from the workspace pattern used by other dependencies. Consider usingsmallvec.workspace = truefor consistency.crates/katana/node/src/config/rpc.rs (1)
13-14: Ohayo! Consider enhancing the constant documentationThe documentation comment has a typo ("maximmum") and could benefit from explaining why 100 was chosen as the default value.
-/// Default maximmum number of keys for the `starknet_getStorageProof` RPC method. +/// Default maximum number of keys for the `starknet_getStorageProof` RPC method. +/// This limit helps prevent excessive resource consumption while still accommodating +/// typical proof request sizes.crates/katana/rpc/rpc-types/src/lib.rs (1)
16-16: Ohayo sensei! The trie module addition looks good!The new
triemodule fits well with the existing module structure. However, consider adding module-level documentation to explain its purpose and relationship with state proofs.Add documentation above the module declaration:
+/// Module containing types for state and storage proof related functionality. pub mod trie;crates/katana/storage/db/src/mdbx/tx.rs (1)
36-36: Ohayo! The explicit initialization looks good, sensei!The change from
Default::default()to explicit[None; NUM_TABLES]improves code clarity by making the initialization intent more obvious.Consider adding a brief comment explaining that the handles are lazily initialized when first accessed.
crates/katana/rpc/rpc/tests/proofs.rs (2)
43-47: Consider making the test data size configurable, sensei!The hardcoded value 35 (used to generate 105 total keys) makes the test less flexible. Consider parameterizing this value based on
DEFAULT_RPC_MAX_PROOF_KEYSfor better maintainability.- for i in 0..35 { + let keys_per_type = DEFAULT_RPC_MAX_PROOF_KEYS / 3 + 2; // Ensures total exceeds limit + for i in 0..keys_per_type {
83-83: Consider using a more robust test data path, sensei!The hardcoded path to test data could make the test brittle. Consider using a test utility function or environment variable to manage test data paths.
- let path: PathBuf = PathBuf::from("tests/test_data/cairo1_contract.json"); + let path = std::env::var("TEST_DATA_DIR") + .unwrap_or_else(|_| "tests/test_data".to_string()) + .pipe(|dir| PathBuf::from(dir).join("cairo1_contract.json"));crates/katana/executor/src/implementation/noop.rs (1)
174-207: Clean implementation of the noop pattern, sensei!The implementations for
StateProofProviderandStateRootProvidercorrectly follow the no-op pattern by returning default values and properly ignoring inputs.Consider adding doc comments explaining that these implementations are intentionally no-op and return default values:
+/// No-op implementation that returns default proofs impl StateProofProvider for NoopStateProvider { // ... existing implementation } +/// No-op implementation that returns zero roots impl StateRootProvider for NoopStateProvider { // ... existing implementation }crates/katana/storage/db/src/abstraction/transaction.rs (1)
72-92: Ohayo sensei! Clean reference-based transaction trait design.The
DbTxReftrait provides an elegant reference-based interface to database transactions, with appropriate cursor types and methods mirroring the originalDbTxtrait. TheClonerequirement is well-suited for reference types.This reference-based design pattern enhances flexibility by allowing transaction references to be passed around without transferring ownership, which is particularly useful in scenarios involving shared database access.
crates/katana/storage/provider/tests/block.rs (1)
32-32: Ohayo sensei! Consider tracking ignored tests.The tests are properly ignored with clear reasons, but we should track when they'll be re-enabled.
Would you like me to create a GitHub issue to track the implementation of trie computation support for forked mode?
Also applies to: 49-49
crates/katana/core/src/backend/storage.rs (1)
48-48: Ohayo, sensei! Clean trait consolidation.The replacement of multiple trie-related traits with a single
TrieWritertrait simplifies the interface while maintaining functionality.This consolidation improves maintainability and reduces the complexity of implementing the
Databasetrait.crates/katana/storage/provider/src/providers/fork/mod.rs (1)
413-423: Ohayo sensei! Remove commented out code.The commented out
StateRootProviderimplementation should be removed entirely rather than left as comments. This keeps the codebase clean and prevents confusion about whether this code might be needed in the future.-// impl StateRootProvider for ForkedProvider { -// fn state_root( -// &self, -// block_id: BlockHashOrNumber, -// ) -> ProviderResult<Option<katana_primitives::Felt>> { -// let state_root = self.block_number_by_id(block_id)?.and_then(|num| { -// self.storage.read().block_headers.get(&num).map(|header| header.state_root) -// }); -// Ok(state_root) -// } -// }
🛑 Comments failed to post (9)
crates/katana/storage/db/src/models/trie.rs (2)
81-87: 🛠️ Refactor suggestion
Ohayo sensei! Check buffer boundaries thoroughly.
Similar to the earlier comment, consider converting further “panic!” calls into error returns to maintain consistent, graceful error handling.
68-70:
⚠️ Potential issueOhayo sensei! Avoid panic for short buffers.
Returning a custom CodecError is preferable for resilient error handling.-if bytes.len() < 2 { - panic!("emptyy buffer") +if bytes.len() < 2 { + return Err(CodecError::Decoding("Buffer too short for type and length".to_string())) }Committable suggestion skipped: line range outside the PR's diff.
crates/katana/trie/src/lib.rs (2)
34-44: 🛠️ Refactor suggestion
Ohayo sensei! BonsaiTrie::new is well-structured.
The snapshot_interval of 1 might grow overhead. Evaluate if you want more control over snapshot intervals.
88-88:
⚠️ Potential issueOhayo sensei! The BasicId usage might conflict with CommitId usage.
Double-check that converted IDs remain consistent once you commit.crates/katana/storage/provider/src/providers/db/state.rs (1)
342-379:
⚠️ Potential issueOhayo sensei! Historical multiproof fetching is properly integrated.
The .expect("should exist") calls might crash if the block is unavailable. Consider graceful error handling or a custom error type for missing historical states.
crates/katana/trie/Cargo.toml (1)
20-23: 💡 Codebase verification
Ohayo sensei! Using commit
9108a126would be safer for reproducibility.The branch
kariy/public-pathhas recent activity (last commit from December 16, 2024) and contains important changes. To ensure build reproducibility, consider updating the dependency to use the specific commit hash:git = "https://github.com/dojoengine/bonsai-trie/" rev = "9108a1264fd82f5e68f5056f8df767042440825f"🔗 Analysis chain
Ohayo sensei! Consider using a specific commit hash instead of a branch.
Using a branch reference (
kariy/public-path) instead of a specific commit hash could lead to reproducibility issues if the branch is updated. Consider pinning to a specific commit hash to ensure consistent builds.Let's check the branch stability:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check branch commit history gh api repos/dojoengine/bonsai-trie/commits?sha=kariy/public-path --jq '.[0:3] | .[] | {sha: .sha, date: .commit.author.date, message: .commit.message}'Length of output: 663
crates/katana/rpc/rpc-types-builder/src/state_update.rs (2)
45-49: 🛠️ Refactor suggestion
Similar error handling improvement needed here
The same expect pattern is used for historical state retrieval.
- .expect("should exist if block exists") + .ok_or_else(|| anyhow::anyhow!( + "Historical state not found for block {} - possible state sync issue", + block_num - 1 + ))?📝 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._ => self .provider .historical((block_num - 1).into())? .ok_or_else(|| anyhow::anyhow!( "Historical state not found for block {} - possible state sync issue", block_num - 1 ))? .state_root()?,
33-37: 🛠️ Refactor suggestion
Consider strengthening error handling
The expect message "should exist if block exists" might not always hold true in edge cases or during state sync.
- .expect("should exist if block exists") + .ok_or_else(|| anyhow::anyhow!( + "State not found for block {:?} - possible state sync issue", + self.block_id + ))?📝 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.let new_root = self .provider .historical(self.block_id)? .ok_or_else(|| anyhow::anyhow!( "State not found for block {:?} - possible state sync issue", self.block_id ))? .state_root()?;crates/katana/core/src/backend/mod.rs (1)
255-263: 💡 Codebase verification
Ohayo sensei! After analyzing the codebase, I can now provide the final response:
Error handling improvement needed for trie operations
The trie operations return
ProviderResult<Felt>, which can propagate meaningful errors throughProviderError. Using.expect()here discards this error handling mechanism and could cause panics in production.
crates/katana/core/src/backend/mod.rs: Replace.expect()calls with proper error propagation using?operator- Consider adding a new variant to
BlockProductionErrorfor trie-related failuresExample fix:
.map_err(|e| BlockProductionError::TrieError(format!("failed to update class trie: {}", e)))?🔗 Analysis chain
Verify error handling in trie operations.
While the implementation looks correct, the
.expect()calls could potentially panic if trie operations fail. Consider proper error handling for production code.Consider propagating errors instead:
- .expect("failed to update class trie"); + .map_err(|e| BlockProductionError::TrieError(format!("failed to update class trie: {}", e)))?;
Let me check the error handling patterns in the trie implementations.
Let me check the trait definitions and implementations for these trie operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other error handling patterns in trie operations # Test: Search for proper error handling patterns in trie operations rg -A 3 "trie_insert_" | grep -v "expect"Length of output: 3741
Script:
#!/bin/bash # Check error handling in the actual implementations ast-grep --pattern 'fn trie_insert_$_($$$) -> Result<$_, $_> { $$$ }'Length of output: 73
Script:
#!/bin/bash # Check for any Result/Error types defined rg "pub (type|struct|enum) \w+Error"Length of output: 3679
Script:
#!/bin/bash # Check the context around trie operations in backend rg -B 5 -A 5 "trie_insert_" crates/katana/core/src/backend/mod.rsLength of output: 728
Script:
#!/bin/bash # Check the trait definitions for trie operations rg -A 10 "trait TrieProvider" crates/katana/storage/provider/src/traits/trie.rs # Check the implementation in the db provider rg -A 20 "impl TrieProvider" crates/katana/storage/provider/src/providers/db/trie.rsLength of output: 168
Script:
#!/bin/bash # Let's check the error types in the core module and the return type of trie operations cat crates/katana/core/src/service/block_producer.rs | rg "BlockProductionError" # Check the trait definition file content cat crates/katana/storage/provider/src/traits/trie.rsLength of output: 1748
Script:
#!/bin/bash # Check the ProviderResult type definition rg "type ProviderResult" crates/katana/storage/provider/src/ # Check the ProviderError enum cat crates/katana/storage/provider/src/error.rsLength of output: 4854
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## katana/dev #2820 +/- ##
=============================================
Coverage ? 55.22%
=============================================
Files ? 444
Lines ? 56339
Branches ? 0
=============================================
Hits ? 31116
Misses ? 25223
Partials ? 0 ☔ View full report in Codecov by Sentry. |
The default implementation of
serdefor enum is to tag it based on the variant name. But the RPC specs doesn't require a tag.https://github.com/starkware-libs/starknet-specs/blob/c94df2c5866e11c866abd3d234b0d5df681073c3/api/starknet_api_openrpc.json#L3699-L3710