fix(katana): estimate message insufficient fee error #2549
Conversation
WalkthroughOhayo, sensei! This pull request introduces modifications to the 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: 0
🧹 Outside diff range and nitpick comments (2)
crates/katana/rpc/rpc-types/src/message.rs (1)
16-20: Ohayo, sensei! LGTM with a small suggestion.The addition of
paid_fee_on_l1and the accompanying comments are well-explained and address the specific requirement in theblockifiercrate. Great job on providing context!Consider adding a link to the relevant documentation or discussion about fee estimation to provide even more context for future developers.
crates/katana/rpc/rpc/tests/messaging.rs (1)
306-351: Ohayo, sensei! The new function looks promising, but let's make it even better!The
estimate_message_feefunction is well-structured and covers the necessary steps to test the fee estimation. However, I have a few suggestions to enhance its effectiveness:
- Consider using the deployed contract address directly instead of computing it again.
- The assertion on line 348 only checks if the result is OK. It would be more valuable to assert something about the actual fee value, such as ensuring it's greater than zero or within an expected range.
- Think about adding some cleanup or tear-down of the test environment to ensure a clean state for subsequent tests.
Here's a suggested improvement for the assertion:
- assert!(result.is_ok()); + let fee = result.expect("Fee estimation failed"); + assert!(fee.gas_price > 0, "Estimated gas price should be greater than zero"); + assert!(fee.gas_consumed > 0, "Estimated gas consumed should be greater than zero");What do you think about these suggestions, sensei? Would you like me to propose more detailed changes?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- crates/katana/rpc/rpc-types/src/message.rs (2 hunks)
- crates/katana/rpc/rpc/tests/messaging.rs (3 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (0 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/rpc/rpc/tests/starknet.rs
🧰 Additional context used
🔇 Additional comments (3)
crates/katana/rpc/rpc-types/src/message.rs (2)
34-36: Ohayo again, sensei! This change looks good.The repositioning of the
calldatafield in the struct initialization is a nice touch. It doesn't affect functionality but might improve readability by grouping related fields together.
Line range hint
1-41: Ohayo one last time, sensei! Overall, these changes look solid.The modifications to the
MsgFromL1struct and itsinto_tx_with_chain_idmethod effectively address the requirements mentioned in the PR objectives. The addition ofpaid_fee_on_l1and the repositioning of thecalldatafield are well-implemented and documented.Great work on improving this part of the codebase!
crates/katana/rpc/rpc/tests/messaging.rs (1)
8-8: Ohayo, sensei! The import changes look good!The new imports are necessary for the added functionality and there are no unused imports. Great job keeping the imports clean and relevant!
Also applies to: 15-15, 24-25
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2549 +/- ##
==========================================
+ Coverage 69.44% 69.54% +0.10%
==========================================
Files 388 388
Lines 49953 49959 +6
==========================================
+ Hits 34688 34745 +57
+ Misses 15265 15214 -51 ☔ View full report in Codecov by Sentry. |
resolves #2546
Summary by CodeRabbit
New Features
Bug Fixes
Tests