refactor(katana): return error as default for state commitment provider #2967
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes to the state management and proof handling in the Katana executor. The modifications primarily involve removing multiproof and state root functionality across several components. Key changes include simplifying trait implementations, adding new error variants for state proofs and roots, and providing default error-based implementations for previously unimplemented methods. Changes
Possibly related PRs
🪧 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: 1
🧹 Nitpick comments (4)
crates/katana/executor/src/implementation/blockifier/state.rs (1)
239-239: Ohayo sensei, empty trait implementation might lead to potential confusion!
Returning a default error or at least logging a message may help clarify that these proofs are not supported. Otherwise, calls to these traits might seem to succeed but provide no meaningful results.crates/katana/storage/provider/src/traits/state.rs (2)
36-39: Ohayo sensei, returningOk(None)for storage root!
The new default might preserve backward compatibility. However, ensure that code callingstorage_root()expects an optional or no-op response.
113-116: Ohayo sensei, unify the class multiproof behavior with other methods!
All multiproof methods consistently respond withStateProofNotSupported. This is a neat approach but watch for partial usage in advanced features or test code.Would you like me to open an issue to track removing references to multiproof from any residual code?
crates/katana/storage/provider/src/error.rs (1)
119-119: Ohayo sensei, corrected the typo in “Other” error message!
Fixing “soemthing” grammar improves codebase quality and user readability. Thanks for addressing this.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/katana/executor/src/implementation/blockifier/state.rs(1 hunks)crates/katana/storage/provider/src/error.rs(2 hunks)crates/katana/storage/provider/src/providers/fork/state.rs(3 hunks)crates/katana/storage/provider/src/traits/state.rs(3 hunks)
🔇 Additional comments (9)
crates/katana/executor/src/implementation/blockifier/state.rs (1)
241-241: Ohayo sensei, verify invocation points!
Since this trait is also empty, verify if there are other parts of the code that expect a valid state root. Returning a default error or clarifying the unsupported nature might avoid misleading usage downstream.crates/katana/storage/provider/src/traits/state.rs (3)
26-28: Ohayo sensei, returningStateRootNotFoundensures a clear error!
This is a valid approach as long as no existing consumer code depends on a valid root. Make sure to handle or log this error in higher-level calls to maintain clarity on why the root is unavailable.
31-33: Ohayo sensei, consistent error for contract root retrieval!
Similar toclasses_root(), returningStateRootNotFoundclarifies unsupported state root queries. Confirm that’s the intended behavior across the codebase.
102-106: Ohayo sensei, raise error for unsupported storage multiproof!
TheStateProofNotSupportederror is explicit and avoids silent failures. This is good, but confirm that no older path relies on multiproofs being partially implemented.crates/katana/storage/provider/src/error.rs (2)
99-101: Ohayo sensei, introducing “StateProofNotSupported”!
Explicitly enumerating the lack of support for state proofs is beneficial and clarifies user feedback. Nice addition!
102-104: Ohayo sensei, new “StateRootNotFound” error variant!
This variant is coherent with returning an error when the root is unavailable. Great for debugging root-dependent logic.crates/katana/storage/provider/src/providers/fork/state.rs (3)
149-150: LGTM! Consistent implementation pattern.Empty implementations maintain consistency with ForkedStateDb's approach, ensuring uniform behavior across provider types.
105-106: Ohayo! Empty trait implementations align with refactoring goals.The removal of explicit implementations in favor of using trait defaults aligns with the PR objective of returning errors as default for state commitment provider.
Let's verify the default implementations in the trait definition:
✅ Verification successful
Ohayo! Empty trait implementations are safe and working as intended, sensei!
The empty implementations in
ForkedStateDbcorrectly inherit the default trait implementations:
StateRootProviderprovides a default state root calculation using Poseidon hashStateProofProviderincludes a default storage multiproof implementation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify default implementations in StateProofProvider and StateRootProvider traits rg -A 5 "trait State(Proof|Root)Provider" $(fd -t f "state\.rs$" "crates/katana")Length of output: 1588
224-225: Ohayo sensei! Implementation pattern completed consistently.Empty implementations align with the pattern established for other providers. However, we should verify test coverage for the default error cases.
Let's check the test coverage:
✅ Verification successful
Ohayo sensei! Empty implementations are correct and safe here!
The empty trait implementations for
StateProofProviderandStateRootProviderare intentional as they inherit the default implementations from the traits. The default implementations provide standard Starknet state behavior:
StateRootProvideruses Poseidon hash with "STARKNET_STATE_V0"StateProofProviderincludes default multiproof generationNo additional test coverage is needed as these are using the trait's default implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for tests covering state proof and root provider error cases rg -l "test.*state.*(proof|root)" $(fd -t f "test.*\.rs$" "crates/katana")Length of output: 178
Script:
#!/bin/bash # Search for ForkedSnapshot in test files and look for state-related tests echo "=== Searching for ForkedSnapshot tests ===" rg -l "ForkedSnapshot.*test" crates/katana echo -e "\n=== Checking implementation file for tests ===" rg "^#\[test\]" -A 10 crates/katana/storage/provider/src/providers/fork/state.rs echo -e "\n=== Looking for any state-related tests ===" rg -l "impl.*State.*(Provider|Root)" crates/katanaLength of output: 1185
Script:
#!/bin/bash # Look for trait-level tests and integration tests echo "=== Checking for trait tests ===" rg -p "trait.*State.*(Provider|Root)" -A 5 crates/katana/storage/provider/src/traits echo -e "\n=== Looking for integration tests ===" fd -t f "integration.*test.*\.rs$" crates/katana/storage/provider/ fd -t f "test.*\.rs$" crates/katana/storage/provider/tests/Length of output: 1968
| fn contract_multiproof(&self, addresses: Vec<ContractAddress>) -> ProviderResult<MultiProof> { | ||
| let _ = addresses; | ||
| Err(ProviderError::StateProofNotSupported) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, confirm contract multiproof removal is fully integrated!
This new error is consistent with the approach in storage_multiproof. Verify that no higher-level parts of the system still expect multiproof functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## katana/executor-refactor #2967 +/- ##
============================================================
+ Coverage 56.89% 57.28% +0.38%
============================================================
Files 423 423
Lines 56070 56051 -19
============================================================
+ Hits 31902 32108 +206
+ Misses 24168 23943 -225 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores
The changes primarily focus on streamlining state management interfaces and improving error handling in the system.