handle policy restrictions from bundler service #4
Conversation
|
""" WalkthroughThis change adds explicit handling for paymaster policy restriction errors in the external bundler send logic. It introduces a new error struct and enum variant for policy errors, updates error mapping and retry logic to recognize and process these errors, and adjusts the build step to use the new retryability function. Changes
Sequence Diagram(s)sequenceDiagram
participant UserOpBuilder
participant Bundler
participant ErrorHandler
UserOpBuilder->>Bundler: Send user operation
Bundler-->>UserOpBuilder: Respond with error (may include paymaster policy error)
UserOpBuilder->>ErrorHandler: Map error using map_build_error
ErrorHandler->>ErrorHandler: try_parse_policy_error (if PaymasterError)
alt PolicyRestriction detected
ErrorHandler-->>UserOpBuilder: Return PolicyRestriction error
UserOpBuilder->>ErrorHandler: is_external_bundler_error_retryable
ErrorHandler-->>UserOpBuilder: Returns false (non-retryable)
UserOpBuilder-->>UserOpBuilder: Fail job
else Other error
ErrorHandler-->>UserOpBuilder: Return other error variant
UserOpBuilder->>ErrorHandler: is_external_bundler_error_retryable
ErrorHandler-->>UserOpBuilder: Returns true/false based on error type
UserOpBuilder-->>UserOpBuilder: Retry or fail job based on result
end
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
🧹 Nitpick comments (1)
executors/src/external_bundler/send.rs (1)
788-824: Consider adding documentationThe function implementation is correct and comprehensive. The retry logic properly handles all error variants with sensible decisions.
Consider adding a doc comment to explain the retry strategy:
+/// Determines if an ExternalBundlerSendError should be retried. +/// +/// Returns `false` for: +/// - Policy restrictions (permanent failures) +/// - User cancellations +/// - Validation errors (invalid salt, credentials, account determination) +/// +/// Returns `true` for: +/// - Temporary failures (deployment locks, chain service errors, internal errors) +/// +/// For build and bundler failures, delegates to the existing retry logic based on the inner error. fn is_external_bundler_error_retryable(e: &ExternalBundlerSendError) -> bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executors/src/external_bundler/send.rs(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
executors/src/external_bundler/send.rs (2)
executors/src/external_bundler/deployment.rs (1)
serde_json(179-179)twmq/src/lib.rs (1)
serde_json(750-750)
🔇 Additional comments (5)
executors/src/external_bundler/send.rs (5)
20-20: LGTM!The
serde_jsonimport is correctly added and necessary for JSON parsing operations.
78-85: LGTM!The
PaymasterPolicyErrorstruct is well-structured with appropriate derives and serde attributes for JSON deserialization.
131-136: LGTM!The
PolicyRestrictionerror variant is well-defined with clear error messaging that includes both the reason and policy ID.
421-421: LGTM!The function call change correctly uses the new
is_external_bundler_error_retryablefunction to handle the expanded error types including policy restrictions.
603-625: LGTM!The policy error detection logic is well-implemented with proper early return pattern and comprehensive checking of both HTTP body and deserialization error text.
| // --- Policy Error Structure --- | ||
| #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct PaymasterPolicyError { | ||
| pub policy_id: String, | ||
| pub reason: String, | ||
| } | ||
|
|
||
| // --- Error Types --- |
There was a problem hiding this comment.
PaymasterPolicyError at the top level feels off because it's not generic, it's a thirdweb specific abstraction. I'd probably have this be expressed into existing auth error, making auth error more detailed if necessary
There was a problem hiding this comment.
dont think it really fits in auth. its more like a valid response that in this case is treated as an error.
rename the var to PaymasterPolicyResponse so its clear
There was a problem hiding this comment.
technically the proper flow would be to make the reqwest return type a union, but this feels good enough for now
Summary by CodeRabbit
New Features
Bug Fixes