multicall fallback + improvements #11
Conversation
…contract read logic - Updated `Cargo.toml` to include `serde_with` version 3.14.0. - Enhanced `contract_read.rs` to support multicall configuration with detailed execution strategies. - Introduced `MulticallConfig` enum for flexible multicall handling, including enabling/disabling and custom addresses. - Refactored contract reading logic to utilize multicall by default, with fallback to direct calls on failure. - Improved error handling and response mapping for multicall results.
WalkthroughThe changes introduce improved configuration and error handling for contract read operations, including a new multicall configuration enum and execution strategy logic. Minor schema documentation annotations are added, and a dependency is updated. Other adjustments include code refactoring for readability and a method visibility change, with no functional modifications outside contract read enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant MulticallContract
participant Blockchain
Client->>Server: Request contract read (possibly multicall)
Server->>Server: Prepare contract calls
alt All preparations failed
Server-->>Client: Return errors per call
else Only one call or multicall disabled
Server->>Blockchain: Execute direct call(s)
Blockchain-->>Server: Return result(s)
Server-->>Client: Return result(s)
else Multicall enabled
Server->>MulticallContract: Execute multicall aggregate3
MulticallContract-->>Server: Return results or error
alt Multicall fails and fallback allowed
Server->>Blockchain: Execute direct call(s)
Blockchain-->>Server: Return result(s)
Server-->>Client: Return result(s)
else Multicall succeeds or fallback not allowed
Server-->>Client: Return multicall results or error
end
end
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to get Caused by: Caused by: Caused by: Caused by: ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
🔭 Outside diff range comments (2)
server/src/http/routes/contract_read.rs (2)
530-563: Fix hardcoded chain ID in error messages.The chain ID is hardcoded as
0in error messages at lines 548 and 572, which loses important context for debugging. The actual chain ID should be passed through to these functions.Consider passing the chain_id through the function parameters:
fn map_multicall_results_to_original_order( preparation_results: &[ContractOperationResult<PreparedContractCall>], multicall_results: &[Result3], call_indices: &[usize], + chain_id: ChainId, ) -> Vec<BatchResultItem<ReadResultSuccessItem>> {And update the error messages:
- BatchResultItem::failure(EngineError::contract_multicall_error( - 0, // Chain ID not available here - "Multicall execution failed".to_string(), - )) + BatchResultItem::failure(EngineError::contract_multicall_error( + chain_id, + "Multicall execution failed".to_string(), + ))
566-596: Pass chain ID to process_multicall_result for accurate error reporting.Similar to the previous issue, chain ID is hardcoded as
0in error messages.Update the function signature and error handling:
fn process_multicall_result( multicall_result: &Result3, prepared_call: &PreparedContractCall, + chain_id: ChainId, ) -> BatchResultItem<ReadResultSuccessItem> { if !multicall_result.success { return BatchResultItem::failure(EngineError::contract_multicall_error( - 0, // Chain ID not available here + chain_id, "Multicall execution failed".to_string(), )); } // ... rest of the function ... Err(e) => { BatchResultItem::failure(EngineError::contract_decoding_error( Some(prepared_call.target), - 0, // Chain ID not available here + chain_id, format!("Failed to decode result: {}", e), )) }
🧹 Nitpick comments (1)
server/src/http/routes/contract_read.rs (1)
320-332: Consider more robust error detection for multicall fallback.The current implementation relies on string matching for error detection, which is fragile and could break if error messages change. Consider using error types or codes if available from the provider.
If the provider returns structured errors, you could match on error types instead:
fn is_multicall_fallback_error(error: &EngineError) -> bool { match error { EngineError::ContractInteractionError { message, .. } => { - // Check for ABI decoding errors that suggest multicall contract doesn't exist or is incompatible - message.contains("ABI decoding failed") - || message.contains("buffer overrun") - || message.contains("Failed to decode multicall result") - || message.contains("execution reverted") - || message.contains("contract not deployed") + // Check for specific error patterns that indicate multicall issues + // Consider adding an error_code field to ContractInteractionError for more robust matching + message.contains("ABI decoding failed") + || message.contains("buffer overrun") + || message.contains("Failed to decode multicall result") + || message.contains("execution reverted") + || message.contains("contract not deployed") } _ => false, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
aa-core/src/signer.rs(5 hunks)core/src/defs.rs(1 hunks)server/Cargo.toml(1 hunks)server/src/http/dyn_contract.rs(1 hunks)server/src/http/routes/contract_read.rs(8 hunks)twmq/tests/basic_hook.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (13)
server/Cargo.toml (1)
43-43: Dependency serde_with@3.14.0 is up-to-date and has no known vulnerabilitiesThe version you’ve pinned (3.14.0) matches the latest release on crates.io and there are no security advisories affecting it. No further action needed.
twmq/tests/basic_hook.rs (3)
84-87: LGTM! Improved function signature formatting.The async function signature formatting enhances readability without affecting functionality.
130-133: LGTM! Consistent formatting improvement.The function signature formatting is consistent with the previous improvement.
180-183: LGTM! Cleaner struct initialization syntax.The struct initialization with
..Default::default()is more idiomatic and readable.core/src/defs.rs (3)
5-5: LGTM! Enhanced schema documentation.The schema title "EVM Address" provides clear documentation for the API schema generation.
11-11: LGTM! Clear schema title for bytes type.The schema title "Bytes" enhances API documentation clarity.
17-17: LGTM! Descriptive schema title for U256.The schema title "U256" provides clear type identification in the API documentation.
server/src/http/dyn_contract.rs (1)
201-209: LGTM! More idiomatic function name extraction.The refactoring to use
strip_prefix("function ")is more idiomatic than manual substring slicing and improves code readability while preserving the same functionality.aa-core/src/signer.rs (2)
251-273: LGTM! Improved error handling and visibility.Making
verify_erc1271public allows external verification workflows. The improved error handling that returns properEngineErrorinstead ofOk(false)is more appropriate.
333-342: Confirm intentional disabling of ERC-1271 signature verificationThe verification code in
aa-core/src/signer.rs(around lines 333–342) has been fully commented out with no feature flag or configuration guard, so deployed accounts’ signatures are never validated locally.
Please verify that this change is intentional. If it isn’t, re-enable the ERC-1271 verification logic or protect it behind a feature flag/config.
- File:
aa-core/src/signer.rs- Approximate lines: 333–342
server/src/http/routes/contract_read.rs (3)
61-136: Well-designed multicall configuration system!The
MulticallConfigenum with its custom deserializer provides excellent flexibility, allowing users to specify multicall configuration as a boolean, null, or custom address. The error messages in the deserializer are clear and helpful.
147-172: Excellent improvements to ReadOptions!The changes enhance both type safety and user experience:
chain_idasu64with flexible deserialization maintains backward compatibility- The multicall configuration is well-documented with clear behavior for each option
223-291: Clean refactoring with intelligent execution strategy!The handler now uses a clear strategy pattern that:
- Efficiently prepares calls in parallel
- Intelligently chooses between direct calls and multicall
- Handles fallback scenarios gracefully
This is a significant improvement in code organization and maintainability.
| // let typed_data_hash = | ||
| // typed_data | ||
| // .eip712_signing_hash() | ||
| // .map_err(|_e| EngineError::ValidationError { | ||
| // message: "Failed to compute typed data hash".to_string(), | ||
| // })?; | ||
| // let is_valid = self.verify_erc1271(typed_data_hash, &signature).await?; | ||
|
|
||
| // if is_valid { | ||
| Ok(signature) | ||
| // } else { | ||
| // Err(EngineError::ValidationError { | ||
| // message: "ERC-1271 signature validation failed".to_string(), | ||
| // }) | ||
| // } |
There was a problem hiding this comment.
Critical: ERC-1271 signature verification is disabled for typed data.
Similar to the message signing, the ERC-1271 verification for typed data signatures has been commented out, creating the same security risk.
This change should be reverted or properly justified with documentation explaining why verification is being bypassed.
🤖 Prompt for AI Agents
In aa-core/src/signer.rs between lines 359 and 373, the ERC-1271 signature
verification for typed data is commented out, disabling an important security
check. Restore the commented code that computes the typed data hash, performs
the ERC-1271 verification asynchronously, and conditionally returns the
signature or an error if validation fails. Ensure the verification logic is
active to maintain security unless a clear justification is documented.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Style
Chores