-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TODO: add ref wrapper #717
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new method Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (8)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/starknet-devnet-types/src/rpc/estimate_message_fee.rs (1)
48-48
: Consider performance impact of cloning.While appropriate in this context, be aware that this clone operation happens on every method call. If this method is called frequently in performance-critical paths, it might be worth considering optimizations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/starknet-devnet-types/src/rpc/estimate_message_fee.rs
(2 hunks)crates/starknet-devnet/src/main.rs
(1 hunks)
🔇 Additional comments (3)
crates/starknet-devnet/src/main.rs (1)
59-59
: Minor formatting improvement.Adding a blank line here enhances documentation readability, which is a good practice.
crates/starknet-devnet-types/src/rpc/estimate_message_fee.rs (2)
41-50
: Good use of the closure pattern for safe object access.This is an excellent approach to providing safe access to dynamically created objects without returning references. The pattern allows consumers to work with the
EthAddressWrapper
by reference within a controlled scope, avoiding potential ownership and lifetime issues.The implementation correctly clones the inner value to create a temporary wrapper that lives for the duration of the closure execution.
93-93
:❓ Verification inconclusive
Confirm that the implementation macros work as expected.
The
impl_wrapper_deserialize!
macro is unchanged, but it's important to verify that it handles the newwith_from_address
method correctly, especially if the macro generates code that interacts with the struct's fields or methods.
🏁 Script executed:
#!/bin/bash # Check where the macro is defined and how it's implemented fd -t f "impl_wrapper_deserialize" --exec grep -A 10 "macro_rules! impl_wrapper_deserialize" {}Length of output: 97
Manual Verification Needed: Confirm Macro Behavior for
with_from_address
The search script did not locate the definition for
impl_wrapper_deserialize!
. Please manually verify that the macro’s implementation (whether defined locally or imported from an external module) correctly supports the newwith_from_address
method. In particular, check that the generated code properly handles field interactions and any logic related towith_from_address
.
- Verify the macro’s definition in the relevant module or crate.
- Confirm that its generated code correctly adapts to the new method in use at
crates/starknet-devnet-types/src/rpc/estimate_message_fee.rs
(line 93).
Please make it clear, which part should i adjust |
…ement tagged serde for Cairo0ContractClass - Added helper enum for serde with and fields. - Enables proper variant tagging during serialization/deserialization.
Usage related changes
Development related changes
Checklist:
./scripts/format.sh
./scripts/clippy_check.sh
./scripts/check_unused_deps.sh
./scripts/check_spelling.sh
./website/README.md
Summary by CodeRabbit