Conversation
aurel-fr
requested review from
Dzejkop,
andy-t-wang,
lukejmann,
murph,
paolodamico,
sideround and
tomislavhoman
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces bundler-sponsored user operations, a flow where the bundler fully sponsors transaction gas costs without requiring a paymaster. This enables direct-to-RPC transaction broadcasting, allowing transactions to bypass the app backend entirely. The implementation is particularly well-suited for World App's primary transaction flow where the bundler covers gas upfront and the API key owner is billed monthly.
Changes:
- Added
UserOperation::as_bundler_sponsored()to convert user operations to bundler-sponsored format (zeroed fees/paymaster) - Extracted
SafeSmartAccount::sign_user_operation()as a reusable signing helper - Introduced
SafeSmartAccount::send_bundler_sponsored_user_operation()as the main public API for bundler-sponsored flows - Extended
AuthenticatedHttpClienttrait withfetch_from_url()for direct external RPC calls - Added
send_user_operation_to_url()function to submit user operations to arbitrary bundler endpoints
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bedrock/src/primitives/contracts.rs | Added as_bundler_sponsored() method to convert UserOperation to bundler-sponsored format |
| bedrock/src/smart_account/transaction_4337.rs | Extracted signing logic into reusable sign_user_operation() helper; refactored sign_and_execute() to use it; added comprehensive unit tests |
| bedrock/src/transactions/mod.rs | Added public send_bundler_sponsored_user_operation() API; removed unused error import |
| bedrock/src/transactions/rpc.rs | Added standalone send_user_operation_to_url() function for direct RPC submission to external bundlers |
| bedrock/src/primitives/http_client.rs | Extended AuthenticatedHttpClient trait with fetch_from_url() for arbitrary URL requests |
| bedrock/src/test_utils.rs | Implemented fetch_from_url() in test client by delegating to existing backend simulation |
| bedrock/tests/test_smart_account_bundler_sponsored.rs | Added comprehensive integration test for bundler-sponsored flow |
| .vscode/settings.json | Added rust-analyzer feature flag for test_utils to improve IDE support |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708b1b6cb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// | ||
| /// # Errors | ||
| /// Same error variants as [`fetch_from_app_backend`]. | ||
| async fn fetch_from_url( |
There was a problem hiding this comment.
Should this be a foreign trait instead of just using a built-in HTTP request handler? There are trade-offs here for sure, but leaning towards Rust-native requests being a better solution here.
aurel-fr
dismissed stale reviews from karankurbur and paolodamico
via
fbb5fe7
|
@codex review |
| async fn test_send_bundler_sponsored_user_operation() -> anyhow::Result<()> { | ||
| // 1) Spin up anvil fork | ||
| let anvil = setup_anvil(); | ||
|
|
||
| // 2) Owner signer and provider | ||
| let owner_signer = PrivateKeySigner::random(); | ||
| let owner_key_hex = hex::encode(owner_signer.to_bytes()); | ||
| let owner = owner_signer.address(); | ||
|
|
||
| let provider = ProviderBuilder::new() | ||
| .wallet(owner_signer.clone()) | ||
| .connect_http(anvil.endpoint_url()); | ||
|
|
||
| provider | ||
| .anvil_set_balance(owner, U256::from(1e18 as u64)) | ||
| .await?; | ||
|
|
||
| // 3) Deploy Safe with 4337 module enabled | ||
| let safe_address = deploy_safe(&provider, owner, U256::ZERO).await?; | ||
|
|
||
| // 4) Fund EntryPoint deposit for Safe (needs enough to cover gas at zero fee) | ||
| let entry_point = IEntryPoint::new(*ENTRYPOINT_4337, &provider); | ||
| entry_point | ||
| .depositTo(safe_address) | ||
| .value(U256::from(1e18 as u64)) | ||
| .send() | ||
| .await? | ||
| .get_receipt() | ||
| .await?; | ||
|
|
||
| // 5) Give Safe some ERC-20 balance | ||
| let wld_token_address = address!("0x2cFc85d8E48F8EAB294be644d9E25C3030863003"); | ||
| let wld = IERC20::new(wld_token_address, &provider); | ||
|
|
||
| let starting_balance = U256::from(10u128.pow(18) * 10); // 10 WLD | ||
| set_erc20_balance_for_safe( | ||
| &provider, | ||
| wld_token_address, | ||
| safe_address, | ||
| starting_balance, | ||
| ) | ||
| .await?; | ||
|
|
||
| // 6) Prepare recipient and assert initial balances | ||
| let recipient = PrivateKeySigner::random().address(); | ||
| let before_recipient = wld.balanceOf(recipient).call().await?; | ||
| let before_safe = wld.balanceOf(safe_address).call().await?; | ||
|
|
||
| // 7) Install HTTP client for backend RPC calls + start mock bundler server | ||
| let client = AnvilBackedHttpClient::new(provider.clone()); | ||
| set_http_client(Arc::new(client.clone())); | ||
| let bundler_url = start_mock_bundler_server(client).await; | ||
|
|
||
| // 8) Craft the user operation manually | ||
| let transfer_amount = U256::from(10u128.pow(18)); // 1 WLD | ||
| let erc20_transfer_call_data = IERC20::transferCall { | ||
| to: recipient, | ||
| amount: transfer_amount, | ||
| }; | ||
| let execute_call_data = ISafe4337Module::executeUserOpCall { | ||
| to: wld_token_address, | ||
| value: U256::ZERO, | ||
| data: alloy::sol_types::SolCall::abi_encode(&erc20_transfer_call_data).into(), | ||
| operation: SafeOperation::Call as u8, | ||
| }; | ||
|
|
||
| let nonce_key = NonceKeyV1::new( | ||
| TransactionTypeId::Transfer, | ||
| InstructionFlag::Default, | ||
| [0u8; 10], | ||
| ); | ||
| let nonce = nonce_key.encode_with_sequence(0); | ||
|
|
||
| let unparsed_user_op = UnparsedUserOperation { | ||
| sender: safe_address.to_string(), | ||
| nonce: format!("{nonce:#x}"), | ||
| call_data: format!( | ||
| "0x{}", | ||
| hex::encode(alloy::sol_types::SolCall::abi_encode(&execute_call_data)) | ||
| ), | ||
| call_gas_limit: "0x200000".to_string(), | ||
| verification_gas_limit: "0x200000".to_string(), | ||
| pre_verification_gas: "0x200000".to_string(), | ||
| max_fee_per_gas: "0x12A05F200".to_string(), | ||
| max_priority_fee_per_gas: "0x12A05F200".to_string(), | ||
| paymaster: None, | ||
| paymaster_verification_gas_limit: None, | ||
| paymaster_post_op_gas_limit: None, | ||
| paymaster_data: None, | ||
| signature: format!("0x{}", hex::encode(vec![0xff; 77])), | ||
| factory: None, | ||
| factory_data: None, | ||
| }; | ||
|
|
||
| // 9) Execute via send_bundler_sponsored_user_operation | ||
| let safe_account = SafeSmartAccount::new(owner_key_hex, &safe_address.to_string())?; | ||
| let _user_op_hash = safe_account | ||
| .send_bundler_sponsored_user_operation(unparsed_user_op, bundler_url.clone()) | ||
| .await | ||
| .expect("send_bundler_sponsored_user_operation failed"); | ||
|
|
||
| // 10) Verify balances updated | ||
| let after_recipient = wld.balanceOf(recipient).call().await?; | ||
| let after_safe = wld.balanceOf(safe_address).call().await?; | ||
|
|
||
| assert_eq!( | ||
| after_recipient, | ||
| before_recipient + U256::from(10u128.pow(18)) | ||
| ); | ||
| assert_eq!(after_safe, before_safe - U256::from(10u128.pow(18))); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
There is no test coverage for error conditions in the bundler-sponsored flow. The integration test only covers the happy path (successful ERC-20 transfer), leaving critical failure modes untested.
Key untested scenarios include: bundler timeout, bundler returning HTTP 5xx, bundler returning RPC error response, invalid user operation hash format, network connectivity failures, and malformed JSON responses. These are all realistic production failure modes that should have test coverage.
Add negative test cases for at least: bundler RPC error response, HTTP error status codes, and timeout scenarios. This is essential per the custom guidelines which require tests for 5xx/timeout/degraded-mode behavior.
There was a problem hiding this comment.
will ship with the PR adding better error support
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080565ccf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async fn post_json_rpc_to_url(url: &str, body: Vec<u8>) -> Result<Vec<u8>, RpcError> { | ||
| let client = REQWEST_CLIENT.get_or_init(reqwest::Client::new); | ||
| let response = client | ||
| .post(url) | ||
| .header(reqwest::header::CONTENT_TYPE, "application/json") | ||
| .body(body) | ||
| .send() | ||
| .await | ||
| .map_err(|e| RpcError::HttpError(e.to_string()))?; | ||
|
|
||
| let status = response.status(); | ||
| let bytes = response | ||
| .bytes() | ||
| .await | ||
| .map_err(|e| RpcError::HttpError(e.to_string()))?; | ||
|
|
||
| if !status.is_success() { | ||
| return Err(RpcError::HttpError(format!( | ||
| "HTTP {status}: {}", | ||
| String::from_utf8_lossy(&bytes) | ||
| ))); | ||
| } | ||
|
|
||
| Ok(bytes.to_vec()) | ||
| } |
There was a problem hiding this comment.
Missing observability for direct bundler RPC calls. When bundler submissions fail in production, there's no logging or metrics to track:
- Which bundler URL was called
- The HTTP status code or error type
- Request latency
- User operation details (sender, nonce) for correlation
This creates a blind spot for incident response. If users report stuck transactions, operators can't determine whether the issue is bundler latency, HTTP errors, or malformed requests without additional instrumentation.
Add structured error logging before returning errors, including the bundler URL, HTTP status, and a correlation identifier (e.g., user_operation.sender). Consider emitting metrics for bundler call latency and error rates if the codebase has a metrics system.
There was a problem hiding this comment.
valid, will be in a follow-up PR
| if !status.is_success() { | ||
| return Err(RpcError::HttpError(format!( | ||
| "HTTP {status}: {}", | ||
| String::from_utf8_lossy(&bytes) | ||
| ))); |
There was a problem hiding this comment.
The function converts non-2xx HTTP status codes to generic errors without distinguishing between different error classes. A bundler might return:
- 400/422 for invalid user operations (client error - should not retry)
- 429 for rate limiting (should retry with backoff)
- 500/502/503 for server errors (may be transient - could retry)
- 504 for timeout (network issue - could retry)
Bundling all these into RpcError::HttpError makes it impossible for callers to implement intelligent retry logic or provide specific error messages to users. Production incidents will be harder to categorize (e.g., "is this our bug or the bundler's?").
Consider mapping HTTP status codes to more specific error variants (e.g., RpcError::RateLimited, RpcError::InvalidRequest, RpcError::BundlerUnavailable) or at minimum preserve the status code in HttpError. This aligns with the operational reliability requirement to enable fast recovery and clear incident categorization.
There was a problem hiding this comment.
also valid, want to address in another PR to keep this one easy to review
| let valid_until_seconds = (Utc::now() | ||
| + Duration::minutes(USER_OPERATION_VALIDITY_DURATION_MINUTES)) | ||
| .timestamp(); | ||
| let valid_until_seconds: u64 = valid_until_seconds.try_into().unwrap_or(0); |
There was a problem hiding this comment.
The timestamp conversion silently defaults to 0 on overflow/underflow via unwrap_or(0). This would cause the signature to have validUntil=0, which likely means "valid forever" or "immediately expired" depending on the entrypoint's interpretation.
In 2038 or if system time is corrupted, this could cause user operations to be rejected or accepted indefinitely, both of which are security/UX issues. Users would see unexplained transaction failures with no clear error message.
Either handle the error explicitly with a clear error message, or add a runtime assertion that the timestamp is reasonable (e.g., > 2020 and < 2100). A proper error message like "System clock is invalid" helps users diagnose the issue.
| let valid_until_seconds = (Utc::now() | |
| + Duration::minutes(USER_OPERATION_VALIDITY_DURATION_MINUTES)) | |
| .timestamp(); | |
| let valid_until_seconds: u64 = valid_until_seconds.try_into().unwrap_or(0); | |
| let valid_until_seconds_i64 = (Utc::now() | |
| + Duration::minutes(USER_OPERATION_VALIDITY_DURATION_MINUTES)) | |
| .timestamp(); | |
| // Sanity-check the system clock to avoid silently generating an invalid timestamp. | |
| // Require a Unix timestamp between 2020-01-01 and 2100-01-01. | |
| const MIN_VALID_UNIX_TIMESTAMP: i64 = 1_577_836_800; // 2020-01-01T00:00:00Z | |
| const MAX_VALID_UNIX_TIMESTAMP: i64 = 4_102_444_800; // 2100-01-01T00:00:00Z | |
| assert!( | |
| (MIN_VALID_UNIX_TIMESTAMP..=MAX_VALID_UNIX_TIMESTAMP).contains(&valid_until_seconds_i64), | |
| "System clock is invalid: computed Unix timestamp {} is outside the supported range", | |
| valid_until_seconds_i64 | |
| ); | |
| let valid_until_seconds: u64 = valid_until_seconds_i64 as u64; |
There was a problem hiding this comment.
This is an improvement but orthogonal to this PR
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| provider | ||
| .anvil_set_balance(owner, U256::from(1e18 as u64)) | ||
| .await?; |
There was a problem hiding this comment.
Maybe this is not needed because deploy_safe called below also calls anvil_set_balance
bedrock/bedrock/tests/common.rs
Lines 115 to 119 in c1bef7c
4 participants
This PR introduces a method to send any user operation to a transaction bundler using the sponsored flow.
As a reminder, this is the primary flow used by World App: the bundler fully sponsors the transaction by covering the gas costs upfront, and the api key owner is billed at the end of the month via credit card.
This approach does not rely on a paymaster and therefore avoids:
As a result, this flow is particularly well suited for direct-to-RPC transaction broadcasting, enabling transactions to bypass the app backend entirely.