feat(katana-pool): invalidate declare tx if the class already exists #2564
Conversation
WalkthroughOhayo, sensei! This pull request introduces several changes to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🔇 Additional comments (2)
🪧 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: 0
🧹 Outside diff range and nitpick comments (4)
crates/katana/pool/src/validation/mod.rs (1)
32-36: Ohayo, sensei! Great clarification on error handling.The updated documentation for the
validatemethod provides clear guidance on how to handle different types of errors. This aligns well with the PR objectives and improves the overall validation process.Consider adding an example to illustrate the difference between an unexpected error and an invalid transaction. This could further enhance the clarity of the documentation.
crates/katana/pool/src/validation/error.rs (1)
66-70: Ohayo, sensei! The new error variant looks sharp!The
ClassAlreadyExistsvariant is a great addition to handle the case of declaring an already existing class. The error message is clear, and the use of the#[error]attribute is consistent with other variants.Consider adding a brief explanation of when this error might occur in the documentation comment, similar to other variants. For example:
/// Error when a Declare transaction attempts to declare a class that has already been /// declared. This ensures that each class is declared only once in the system. #[error("Class with hash {class_hash:#x} already exists.")] ClassAlreadyExists { class_hash: ClassHash },crates/katana/rpc/rpc/tests/starknet.rs (1)
153-187: Ohayo again, sensei! This test function is looking sharp!The
declaring_already_existing_classtest function effectively covers the scenario of attempting to declare an already existing class. It's well-structured and uses our newassert_starknet_err!macro nicely.One small suggestion to make it even better:
Consider adding a comment explaining why we're setting the max fee manually on line 183. It might help future readers understand the reasoning behind this step.
+ // We set max fee manually to avoid performing fee estimation as we just want to test that the + // pool validation will reject the tx. let result = account.declare_v2(contract.into(), compiled_hash).max_fee(Felt::ONE).send().await;Overall, great job implementing this test, sensei! It aligns perfectly with our PR objectives.
crates/katana/pool/src/validation/stateful.rs (1)
141-142: Ohayo, sensei! Consider combining comment lines for clarityThe comment is split across two lines. For improved readability, consider combining them into a single line.
Apply this diff:
- // prepare a stateful validator and run the account validation logic (ie __validate__ - // entrypoint) + // prepare a stateful validator and run the account validation logic (ie __validate__ entrypoint)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- crates/katana/pool/src/validation/error.rs (1 hunks)
- crates/katana/pool/src/validation/mod.rs (1 hunks)
- crates/katana/pool/src/validation/stateful.rs (2 hunks)
- crates/katana/rpc/rpc-types/src/error/starknet.rs (1 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
crates/katana/pool/src/validation/mod.rs (1)
19-23: Ohayo, sensei! LGTM: Newnewmethod forErrorstruct.The addition of the
newmethod to theErrorstruct provides a clean and convenient way to create new instances. It follows good Rust practices for struct initialization.crates/katana/rpc/rpc-types/src/error/starknet.rs (3)
Line range hint
25-25: Ohayo, sensei! New error variant looks sharp!The addition of
ClassAlreadyDeclaredto theStarknetApiErrorenum is spot-on. It aligns perfectly with our mission to invalidate declare transactions for existing classes. The error message is clear and concise, just like a well-sharpened katana!
185-185: Ohayo again, sensei! TheFromimplementation is as smooth as silk!The new match arm for
InvalidTransactionError::ClassAlreadyExistsis a perfect addition to our error-handling dojo. It seamlessly integrates with the existing implementation, maintaining our code's harmony. Great job on keeping our error conversions consistent and clear!
Line range hint
25-25: Ohayo once more, sensei! Your code changes are as harmonious as a zen garden!The additions to
StarknetApiErrorand itsFromimplementation are perfectly aligned with our mission to invalidate declare transactions for existing classes. These changes enhance our error handling capabilities while maintaining the existing structure and style. Excellent work on improving our katana's edge!Is there anything else you'd like me to review or explain further, sensei?
Also applies to: 185-185
crates/katana/rpc/rpc/tests/starknet.rs (2)
37-42: Ohayo, sensei! LGTM for this macro implementation.The
assert_starknet_err!macro looks well-crafted and will simplify error assertions in our tests. It's a nice addition to our testing toolkit.
Line range hint
37-187: Ohayo one last time, sensei! Let's wrap this up with a bow.The changes in this file are spot-on for our PR objectives. Here's a quick summary:
- The new
assert_starknet_err!macro (lines 37-42) simplifies our error assertions, making our tests cleaner and more readable.- The
declaring_already_existing_classtest function (lines 153-187) effectively covers the scenario of attempting to declare an already existing class, which is exactly what we aimed for in this PR.These additions improve our test coverage and make our error handling more robust. Great work, sensei! This implementation aligns perfectly with the StarkNet API documentation and enhances the integrity of our transaction process.
crates/katana/pool/src/validation/stateful.rs (1)
105-116: Ohayo, sensei! The static class existence check looks good!The implementation correctly checks for the existence of the class and returns the appropriate error if the class already exists.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2564 +/- ##
==========================================
- Coverage 69.77% 69.76% -0.01%
==========================================
Files 400 400
Lines 51171 51183 +12
==========================================
+ Hits 35704 35709 +5
- Misses 15467 15474 +7 ☔ View full report in Codecov by Sentry. |
add a static check in the pool validator to check if the class that a Declare tx is trying to declare has already existed (ie already been declared). this is align with the spec
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests