Implement delegation contract handling in EIP-7702 executor and bundler client #41
Conversation
…er client - Added `TwGetDelegationContractResponse` struct to encapsulate the delegation contract address. - Introduced `tw_get_delegation_contract` method in `BundlerClient` to fetch the delegation contract. - Updated transaction signing methods in `DelegatedAccount` and `MinimalAccountTransaction` to accept delegation contract as a parameter. - Refactored integration tests to utilize the new delegation contract handling. - Created a `DelegationContractCache` for efficient retrieval of delegation contracts in the executor. These changes enhance the EIP-7702 framework by integrating delegation contract management, improving transaction processing and observability.
|
Caution Review failedThe pull request is closed. WalkthroughAdds an RPC to fetch the delegation contract, removes a hardcoded minimal-account address, threads Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as EIP-7702 Send Handler
participant Cache as DelegationContractCache
participant Bundler as Bundler RPC
participant Tx as MinimalAccountTransaction
Caller->>Cache: get_delegation_contract(chain)
alt cache hit
Cache-->>Caller: delegation_contract (Address)
else cache miss/expired
Cache->>Bundler: tw_getDelegationContract()
Bundler-->>Cache: { delegationContract }
Cache-->>Caller: delegation_contract (Address)
end
Caller->>Tx: add_authorization_if_needed(signer, cred, delegation_contract)
Tx->>Tx: sign_authorization(..., delegation_contract)
Tx-->>Caller: authorized tx or error
sequenceDiagram
autonumber
participant Worker as EOA Worker
participant Metrics as EoaMetrics
Worker->>Worker: compute time_since_movement
Worker->>Worker: is_out_of_funds = balance <= threshold
alt stalled & has waiting & not is_out_of_funds
Worker->>Worker: attempt gas bump
opt failure
Worker->>Worker: warn (gas bump failed)
end
else stalled & is_out_of_funds
Worker->>Worker: warn (skip bump, out_of_funds)
end
Worker->>Metrics: record_stuck_eoa(eoa, chain_id, seconds, is_out_of_funds)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
executors/src/metrics.rs (1)
106-119: Critical: duplicate metric registration in get_metrics() fallback.When no custom metrics are set,
get_metrics()constructs a new ExecutorMetrics each call, re-registering the same metric names in the same registry and likely panicking (“already registered”). Reuse a single default Arc instead.Apply:
lazy_static! { /// Default metrics registry for executors (fallback if no external registry provided) static ref DEFAULT_EXECUTOR_METRICS_REGISTRY: Registry = Registry::new(); /// Default executor metrics instance (used when no external metrics are provided) - static ref DEFAULT_EXECUTOR_METRICS: ExecutorMetrics = - ExecutorMetrics::new(&DEFAULT_EXECUTOR_METRICS_REGISTRY) - .expect("Failed to create default executor metrics"); + static ref DEFAULT_EXECUTOR_METRICS: std::sync::Arc<ExecutorMetrics> = + std::sync::Arc::new( + ExecutorMetrics::new(&DEFAULT_EXECUTOR_METRICS_REGISTRY) + .expect("Failed to create default executor metrics") + ); /// Global metrics instance - can be set by the binary crate or uses default static ref EXECUTOR_METRICS_INSTANCE: std::sync::RwLock<Option<Arc<ExecutorMetrics>>> = std::sync::RwLock::new(None); } /// Get the current metrics instance (either custom or default) fn get_metrics() -> Arc<ExecutorMetrics> { let instance = EXECUTOR_METRICS_INSTANCE.read().unwrap(); match instance.as_ref() { Some(metrics) => metrics.clone(), None => { // Use default metrics if no custom metrics were set // This ensures backward compatibility drop(instance); // Release read lock - Arc::new(ExecutorMetrics::new(&DEFAULT_EXECUTOR_METRICS_REGISTRY) - .expect("Failed to create default metrics")) + DEFAULT_EXECUTOR_METRICS.clone() } } }eip7702-core/tests/integration_tests.rs (1)
135-138: Test will panic: bundler_client() is unimplemented but invoked in fetch_and_set_bytecode()TestChain::bundler_client() panics, yet fetch_and_set_bytecode() calls it. The test will always abort before setting delegation_contract.
Apply a minimal fix by instantiating a BundlerClient directly against Base Sepolia inside fetch_and_set_bytecode(), avoiding TestChain::bundler_client():
- let delegation_contract_response = self - .chain - .bundler_client() - .tw_get_delegation_contract() - .await?; + // Use a real BundlerClient for Base Sepolia in tests + let bundler = + engine_core::rpc_clients::BundlerClient::new(base_sepolia_url, Default::default()); + let delegation_contract_response = bundler.tw_get_delegation_contract().await?;Alternatively, implement a real bundler_client() on TestChain (store a BundlerClient field) and point it to a valid bundler endpoint. Let me know which direction you prefer; I can provide a full patch for either option.
Also applies to: 380-390
🧹 Nitpick comments (15)
eip7702-core/src/constants.rs (2)
3-6: Fix RPC name in the note (snake_case).The RPC is
tw_get_delegation_contractthroughout the codebase; here it’s written astw_getDelegationContract. Align to avoid confusion.Apply:
-// NOTE!: do not hardcode. If needed later, use tw_getDelegationContract +// NOTE!: do not hardcode. If needed later, use tw_get_delegation_contract
1-1: Remove unused imports.
Addressandaddress!aren’t used anymore after removing the hardcoded constant. This will trigger dead_code/unused_imports warnings.-use alloy::primitives::{Address, address}; +// No imports needed hereexecutors/src/metrics.rs (1)
283-291: Strengthen test: assert presence of the new label (both values).Validate export includes out_of_funds label and both True/False samples when recorded.
let metrics_output = export_default_metrics().expect("Should be able to export default metrics"); assert!(metrics_output.contains("tw_engine_executor_transaction_queued_to_sent_duration_seconds")); assert!(metrics_output.contains("tw_engine_executor_transaction_queued_to_confirmed_duration_seconds")); assert!(metrics_output.contains("tw_engine_eoa_executor_job_processing_duration_seconds")); assert!(metrics_output.contains("tw_engine_executor_eoa_degraded_send_duration_seconds")); assert!(metrics_output.contains("tw_engine_executor_eoa_degraded_confirmation_duration_seconds")); assert!(metrics_output.contains("tw_engine_executor_eoa_stuck_duration_seconds")); + // Verify the new label is present and both boolean values can appear + assert!(metrics_output.contains("out_of_funds")); + // Record a 'true' sample and re-export to check both values + eoa_metrics.record_stuck_eoa(test_address, 1, 900.0, true); + let metrics_output = export_default_metrics().expect("Should be able to export default metrics"); + assert!(metrics_output.contains("out_of_funds=\"false\"") || metrics_output.contains("out_of_funds=\"true\""));eip7702-core/src/delegated_account.rs (2)
30-33: Update doc to match behavior (prefix/length check only).The function no longer validates the target equals a specific minimal account. Reflect this to avoid misleading callers.
- /// Check if the EOA has EIP-7702 delegation to the minimal account implementation + /// Check if the EOA has EIP-7702 delegation (validates prefix/length only; does not validate the target contract)
63-66: Optional: log detected delegation target at trace level for debugging.Lightweight visibility without re-introducing hardcoded checks.
- // NOTE!: skip the actual delegated target address check for now - // extremely unlikely that an EOA being used with engine is delegated to a non-minimal account - // Potential source for fringe edge cases, please verify delegated target address if debugging 7702 execution issues + // NOTE: We intentionally skip asserting the target equals a specific implementation. + // For debug visibility, log the delegation target at trace level. + let target_address = Address::from_slice(&code[3..23]); + tracing::trace!( + eoa_address = ?self.eoa_address, + target_address = ?target_address, + "EIP-7702 delegation detected" + );executors/src/eoa/worker/mod.rs (1)
27-28: Decouple time util from metrics module.Prefer using the existing
eoa::store::now()to avoid depending on metrics for time.-use crate::metrics::{ - EoaMetrics, calculate_duration_seconds, current_timestamp_ms, record_eoa_job_processing_time, -}; +use crate::metrics::{EoaMetrics, calculate_duration_seconds, record_eoa_job_processing_time}; +use crate::eoa::store::now as current_timestamp_ms;server/src/queue/manager.rs (1)
191-199: Make cache tuning configurable and consider warmup to avoid first-hit latency
- Consider plumbing max_capacity/TTL/TTI from config (QueueConfig or a new cache section) to tune in prod without redeploys.
- Optional: warm the cache per configured chain at startup to avoid the first tx paying the RPC latency.
eip7702-core/src/transaction.rs (1)
192-205: API change verified: all call sites updated; add documentationI’ve confirmed that both usages of
add_authorization_if_needednow include thedelegation_contractargument:
- eip7702-core/tests/integration_tests.rs (around line 697)
- executors/src/eip7702_executor/send.rs (around line 233)
Please add a doc comment on the method to explain the new parameter. For example:
- pub async fn add_authorization_if_needed<S: AccountSigner>( + /// If the target is not a minimal account, signs and attaches an EIP-7702 + /// authorization payload using the given `delegation_contract` address. + /// + /// Minimal accounts short-circuit and return immediately without using it. + pub async fn add_authorization_if_needed<S: AccountSigner>( mut self, signer: &S, credentials: &SigningCredential, delegation_contract: Address, ) -> Result<Self, EngineError> {executors/src/eip7702_executor/delegation_cache.rs (2)
19-21: Hide internal cache to maintain invariantsExpose a getter if needed; avoid letting external code mutate/replace the cache internals.
Apply this diff:
-#[derive(Clone)] -pub struct DelegationContractCache { - pub inner: moka::future::Cache<DelegationContractCacheKey, Address>, -} +#[derive(Clone)] +pub struct DelegationContractCache { + inner: moka::future::Cache<DelegationContractCacheKey, Address>, +} + +impl DelegationContractCache { + pub fn inner(&self) -> &moka::future::Cache<DelegationContractCacheKey, Address> { + &self.inner + } +}
38-66: Minor: avoid repeated chain_id() calls in the loaderStore once for logging clarity and tiny perf/readability win.
Apply this diff:
- let result = self + let chain_id = chain.chain_id(); + let result = self .inner .try_get_with(cache_key, async { - tracing::debug!( - chain_id = chain.chain_id(), + tracing::debug!( + chain_id = chain_id, "Fetching delegation contract from bundler" ); let TwGetDelegationContractResponse { delegation_contract, } = chain .bundler_client() .tw_get_delegation_contract() .await .map_err(|e| e.to_engine_bundler_error(chain))?; - tracing::debug!( - chain_id = chain.chain_id(), + tracing::debug!( + chain_id = chain_id, delegation_contract = ?delegation_contract, "Successfully fetched and cached delegation contract" );eip7702-core/tests/integration_tests.rs (4)
121-129: Bundler URL equals Anvil RPC URL; tw_get_delegation_contract won’t exist thereReturning rpc_url from bundler_url is misleading for this test because Anvil won’t implement the bundler RPC. If you wire TestChain::bundler_client(), ensure it targets a real bundler endpoint (e.g., via a separate bundler_url or env override).
391-406: Log/message terminology: say “delegation contract,” not “minimal account implementation”The printed message still references “minimal account implementation,” which can confuse when debugging delegation contract issues.
- println!( - "Set bytecode for minimal account implementation at {:?}", - delegation_contract_response.delegation_contract - ); + println!( + "Set bytecode for delegation contract at {:?}", + delegation_contract_response.delegation_contract + );
661-663: Network dependency in tests may flake CIFetching bytecode from a public RPC at runtime makes tests non-hermetic. Consider vendoring the delegation contract bytecode (like MockERC20) or stubbing tw_get_delegation_contract during tests.
697-702: Early assertion for delegation_contract improves failure localityInstead of expecting here, assert that delegation_contract is set immediately after fetch_and_set_bytecode() to fail fast if the bundler call or fetch fails.
- setup.fetch_and_set_bytecode().await?; + setup.fetch_and_set_bytecode().await?; + assert!(setup.delegation_contract.is_some(), "delegation_contract must be set before building transactions");executors/src/eip7702_executor/send.rs (1)
145-146: Prefer sharing DelegationContractCache via ArcIf the cache holds shared state/HTTP clients, storing by value risks unnecessary cloning. Passing Arc avoids duplication and keeps cache warm across handlers.
- pub delegation_contract_cache: DelegationContractCache, + pub delegation_contract_cache: std::sync::Arc<DelegationContractCache>,Note: update construction sites accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
core/src/rpc_clients/bundler.rs(2 hunks)eip7702-core/src/constants.rs(1 hunks)eip7702-core/src/delegated_account.rs(4 hunks)eip7702-core/src/transaction.rs(1 hunks)eip7702-core/tests/integration_tests.rs(6 hunks)executors/src/eip7702_executor/delegation_cache.rs(1 hunks)executors/src/eip7702_executor/mod.rs(1 hunks)executors/src/eip7702_executor/send.rs(4 hunks)executors/src/eoa/worker/confirm.rs(2 hunks)executors/src/eoa/worker/mod.rs(1 hunks)executors/src/metrics.rs(3 hunks)server/src/queue/manager.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
executors/src/eoa/worker/mod.rs (2)
executors/src/eoa/store/mod.rs (1)
now(780-782)executors/src/metrics.rs (1)
current_timestamp_ms(236-238)
executors/src/eip7702_executor/send.rs (1)
executors/src/metrics.rs (3)
calculate_duration_seconds_from_twmq(230-233)current_timestamp_ms(236-238)record_transaction_queued_to_sent(132-137)
executors/src/eip7702_executor/delegation_cache.rs (2)
eip7702-core/tests/integration_tests.rs (2)
new(298-363)chain_id(113-115)server/src/queue/manager.rs (1)
new(46-256)
🔇 Additional comments (11)
executors/src/metrics.rs (2)
70-71: LGTM: added out_of_funds label.The extra label improves diagnosability. Using boolean-to-string yields "true"/"false", which is Prometheus-friendly.
205-211: Allrecord_stuck_eoacall sites include the newout_of_fundsparameterI’ve verified across the codebase that there are no remaining invocations of
record_stuck_eoawith only three arguments:
ast-grepfound zero matches for a 3-argument pattern- In
executors/src/eoa/worker/confirm.rs, the call at lines 127–130 correctly passesis_out_of_fundsas the fourth argumentNo further updates are needed.
eip7702-core/src/delegated_account.rs (1)
104-126: Review Complete: Delegation Contract Argument PropagationAll call sites of
sign_authorizationandadd_authorization_if_neededcorrectly include thedelegation_contractargument:
eip7702-core/src/transaction.rs: bothsign_authorization(...)(line 202) andadd_authorization_if_needed(…)(line 192) accept and propagatedelegation_contract.- Integration tests in
eip7702-core/tests/integration_tests.rsandexecutors/src/eip7702_executor/send.rspasssetup.delegation_contract.expect(...)anddelegation_contract, respectively.- No missing call sites were found.
No further changes are needed.
executors/src/eip7702_executor/mod.rs (1)
1-3: LGTM: export delegation_cache module.Surface looks correct; no concerns.
executors/src/eoa/worker/mod.rs (1)
502-511: LGTM: consistent initialization timestamps.Initializing all health timestamps to the same
nowreduces ordering ambiguities on first write.server/src/queue/manager.rs (1)
200-208: LGTM: Cache is correctly threaded into the EIP-7702 send handlerPassing DelegationContractCache into Eip7702SendHandler looks correct and keeps concerns separated.
core/src/rpc_clients/bundler.rs (1)
91-96: LGTM: New response type is well-shaped and follows existing serde patternsCamelCase rename aligns with bundler payloads; Address type is appropriate.
executors/src/eoa/worker/confirm.rs (2)
79-113: Good guard: skip gas bumps when out of fundsConditioning the bump on balance prevents futile retries and noisy logs.
121-133: Allrecord_stuck_eoainvocations now include the newout_of_fundsparameter
- In
executors/src/metrics.rs(line 205), the signature was updated to acceptout_of_funds: bool.- In the metrics test (line 281), the call now passes four arguments:
eoa_metrics.record_stuck_eoa(test_address, 1, 900.0, false);- In
executors/src/eoa/worker/confirm.rs(lines 127–133), the call was updated to:self.store.eoa_metrics.record_stuck_eoa( self.eoa, self.chain_id, time_since_movement_seconds, is_out_of_funds, );- A repository-wide search (
rg '\brecord_stuck_eoa\s*\(') shows no other call sites requiring updates.All callers have been confirmed. Feel free to resolve this comment.
eip7702-core/tests/integration_tests.rs (1)
576-582: Verify EIP-712 domain values for session signingConfirm that name = "MinimalAccount" and verifying_contract = user_address are correct for the deployed delegation/minimal-account setup you’re exercising. If the domain has changed with the new delegation contract, signatures will fail subtly.
executors/src/eip7702_executor/send.rs (1)
301-306: Metrics LGTMQueued→sent duration is computed with consistent timestamp units; labels match existing metrics.
| pub async fn tw_get_delegation_contract(&self) -> TransportResult<TwGetDelegationContractResponse> { | ||
| let response: TwGetDelegationContractResponse = self.inner.request("tw_getDelegationContract", ()).await?; | ||
| Ok(response) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer sending [] over () for zero-arg JSON-RPC calls
Some servers reject params: null. Using an empty array is safer and matches other methods here.
Apply this diff:
- pub async fn tw_get_delegation_contract(&self) -> TransportResult<TwGetDelegationContractResponse> {
- let response: TwGetDelegationContractResponse = self.inner.request("tw_getDelegationContract", ()).await?;
- Ok(response)
- }
+ pub async fn tw_get_delegation_contract(
+ &self,
+ ) -> TransportResult<TwGetDelegationContractResponse> {
+ let params = serde_json::json!([]);
+ let response: TwGetDelegationContractResponse =
+ self.inner.request("tw_getDelegationContract", params).await?;
+ Ok(response)
+ }To verify consistency repo-wide (and catch any other zero-arg calls using ()), run:
🏁 Script executed:
#!/bin/bash
# Find JSON-RPC calls passing unit params
rg -nP --type=rust 'request\(\s*"[^"]+",\s*\(\)\s*\)'Length of output: 305
Use an empty JSON array for zero-arg RPC calls
Some JSON-RPC servers reject params: null. To ensure compatibility and match the rest of our clients, always send [] instead of () when there are no parameters.
Affected methods:
core/src/rpc_clients/bundler.rs:tw_get_delegation_contractcore/src/rpc_clients/paymaster.rs:thirdweb_getUserOperationGasPrice
Proposed diffs:
--- a/core/src/rpc_clients/bundler.rs
+++ b/core/src/rpc_clients/bundler.rs
@@ -180,7 +180,12 @@ impl BundlerClient {
- pub async fn tw_get_delegation_contract(&self) -> TransportResult<TwGetDelegationContractResponse> {
- let response: TwGetDelegationContractResponse = self.inner.request("tw_getDelegationContract", ()).await?;
- Ok(response)
- }
+ pub async fn tw_get_delegation_contract(
+ &self,
+ ) -> TransportResult<TwGetDelegationContractResponse> {
+ let params = serde_json::json!([]);
+ let response: TwGetDelegationContractResponse =
+ self.inner.request("tw_getDelegationContract", params).await?;
+ Ok(response)
+ }--- a/core/src/rpc_clients/paymaster.rs
+++ b/core/src/rpc_clients/paymaster.rs
@@ -45,7 +45,11 @@ impl PaymasterClient {
- let response: ThirdwebGetUserOperationGasPriceResponse = self
- .inner
- .request("thirdweb_getUserOperationGasPrice", ())
- .await?;
+ let params = serde_json::json!([]);
+ let response: ThirdwebGetUserOperationGasPriceResponse = self
+ .inner
+ .request("thirdweb_getUserOperationGasPrice", params)
+ .await?;
Ok(response)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn tw_get_delegation_contract(&self) -> TransportResult<TwGetDelegationContractResponse> { | |
| let response: TwGetDelegationContractResponse = self.inner.request("tw_getDelegationContract", ()).await?; | |
| Ok(response) | |
| } | |
| // core/src/rpc_clients/bundler.rs | |
| pub async fn tw_get_delegation_contract( | |
| &self, | |
| ) -> TransportResult<TwGetDelegationContractResponse> { | |
| let params = serde_json::json!([]); | |
| let response: TwGetDelegationContractResponse = | |
| self.inner.request("tw_getDelegationContract", params).await?; | |
| Ok(response) | |
| } |
| pub async fn tw_get_delegation_contract(&self) -> TransportResult<TwGetDelegationContractResponse> { | |
| let response: TwGetDelegationContractResponse = self.inner.request("tw_getDelegationContract", ()).await?; | |
| Ok(response) | |
| } | |
| // core/src/rpc_clients/paymaster.rs | |
| let params = serde_json::json!([]); | |
| let response: ThirdwebGetUserOperationGasPriceResponse = self | |
| .inner | |
| .request("thirdweb_getUserOperationGasPrice", params) | |
| .await?; | |
| Ok(response) |
🤖 Prompt for AI Agents
In core/src/rpc_clients/bundler.rs around lines 180 to 183, the RPC call uses ()
as the params which serializes to null; change the params to an empty JSON array
so the request sends [] instead of null — replace the request invocation to use
an empty-array params value to ensure compatibility with JSON-RPC servers that
reject null params (and apply the same pattern to other zero-arg RPC methods
such as the paymaster thirdweb_getUserOperationGasPrice).
| // Get delegation contract from cache | ||
| let delegation_contract = self | ||
| .delegation_contract_cache | ||
| .get_delegation_contract(transactions.account().chain()) | ||
| .await | ||
| .map_err(|e| Eip7702SendError::DelegationCheckFailed { inner_error: e }) | ||
| .map_err_fail()?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make delegation-contract lookup retryable on transient errors
Currently, a failure in get_delegation_contract() always fails the job. Align with the retry policy used later (add_authorization_if_needed) so network blips don’t cause permanent failures.
- let delegation_contract = self
- .delegation_contract_cache
- .get_delegation_contract(transactions.account().chain())
- .await
- .map_err(|e| Eip7702SendError::DelegationCheckFailed { inner_error: e })
- .map_err_fail()?;
+ let delegation_contract = self
+ .delegation_contract_cache
+ .get_delegation_contract(transactions.account().chain())
+ .await
+ .map_err(|e| {
+ let mapped = Eip7702SendError::DelegationCheckFailed { inner_error: e.clone() };
+ if is_build_error_retryable(&e) {
+ mapped.nack_with_max_retries(
+ Some(Duration::from_secs(2)),
+ twmq::job::RequeuePosition::Last,
+ 3,
+ job.attempts(),
+ )
+ } else {
+ mapped.fail()
+ }
+ })?;Additionally, add a debug log to aid triage:
+ tracing::debug!(delegation_contract = ?delegation_contract, "resolved delegation contract");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get delegation contract from cache | |
| let delegation_contract = self | |
| .delegation_contract_cache | |
| .get_delegation_contract(transactions.account().chain()) | |
| .await | |
| .map_err(|e| Eip7702SendError::DelegationCheckFailed { inner_error: e }) | |
| .map_err_fail()?; | |
| // Get delegation contract from cache | |
| let delegation_contract = self | |
| .delegation_contract_cache | |
| .get_delegation_contract(transactions.account().chain()) | |
| .await | |
| .map_err(|e| { | |
| let mapped = Eip7702SendError::DelegationCheckFailed { inner_error: e.clone() }; | |
| if is_build_error_retryable(&e) { | |
| mapped.nack_with_max_retries( | |
| Some(Duration::from_secs(2)), | |
| twmq::job::RequeuePosition::Last, | |
| 3, | |
| job.attempts(), | |
| ) | |
| } else { | |
| mapped.fail() | |
| } | |
| })?; | |
| tracing::debug!(delegation_contract = ?delegation_contract, "resolved delegation contract"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
executors/src/eip7702_executor/confirm.rs (1)
200-206: Disambiguate log fields: use bundler_transaction_id and include engine transaction_idCurrent log uses transaction_id = job_data.bundler_transaction_id, which collides with the engine’s transaction_id used elsewhere and hurts log querying.
Apply:
- tracing::error!( - transaction_id = job_data.bundler_transaction_id, - sender_details = ?job_data.sender_details, - error = ?e, - "Failed to get transaction hash from bundler" - ); + tracing::error!( + transaction_id = job_data.transaction_id, + bundler_transaction_id = job_data.bundler_transaction_id, + sender_details = ?job_data.sender_details, + error = ?e, + "Failed to get transaction hash from bundler" + );Note: Ensure Eip7702Sender does not include secrets; if it might, redact or log minimal identifiers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
executors/src/eip7702_executor/confirm.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
executors/src/eip7702_executor/confirm.rs (1)
executors/src/metrics.rs (3)
calculate_duration_seconds_from_twmq(230-233)current_timestamp_ms(236-238)record_transaction_queued_to_confirmed(139-144)
🔇 Additional comments (3)
executors/src/eip7702_executor/confirm.rs (3)
211-211: Good nack/backoff on bundler fetch failures10s nack with RequeuePosition::Last is reasonable and consistent with transient RPC issues. No changes needed.
21-24: Imports OK; will remain valid after unit-fixNo change needed if you keep calculate_duration_seconds_from_twmq and divide ms→s as suggested above.
280-290: The next step is to inspect the definition ofcalculate_duration_seconds_from_twmqto confirm whether it expects timestamps in seconds (which would validate the need to divide the millisecond value by 1000). Once we have that, we can finalize whether the proposed ms→s conversion is required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
twmq/src/lib.rs (1)
587-590: ARGV reindexing looks correct; document mapping to avoid regressionsThe shift to ARGV[2..4] for pop_id/batch_size/lease_seconds matches the Rust call order. Add an inline note at the call site to keep this mapping obvious during future edits.
.key(self.success_list_name()) - .arg(now) + // ARGV: [1]=now, [2]=pop_id, [3]=batch_size, [4]=lease_seconds + .arg(now) .arg(pop_id) .arg(batch_size) .arg(self.options.lease_duration.as_secs())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
executors/src/eoa/worker/mod.rs(3 hunks)twmq/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- executors/src/eoa/worker/mod.rs
⏰ 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 (2)
twmq/src/lib.rs (2)
713-713: Lease token scoped by batch pop_id — LGTMIncluding pop_id in lease_token cleanly isolates per-batch leases without affecting existing parsing (none observed).
754-757: Lua invocation updated to pass pop_id — consistent with scriptArgument order matches the script’s ARGV usage. No issues.
| let pop_id = nanoid::nanoid!(4); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Batch pop identifier added; consider length and dependency check
Good call adding a per-batch pop_id. Two small follow-ups:
- Use a slightly longer id (e.g., 8) to reduce accidental collisions under very high QPS, even if collisions are low impact here.
- Ensure the nanoid crate is declared in Cargo.toml.
Apply:
- let pop_id = nanoid::nanoid!(4);
+ let pop_id = nanoid::nanoid!(8);Verify dependency:
🏁 Script executed:
#!/bin/bash
rg -n '^\s*nanoid\s*=' Cargo.toml || { echo "Missing nanoid crate in Cargo.toml"; exit 1; }Length of output: 123
Add missing nanoid dependency & increase pop_id length
The project currently uses the nanoid::nanoid! macro but does not declare the nanoid crate in Cargo.toml, causing a compilation error. Additionally, bumping the ID length from 4 to 8 will further reduce collision risk under high QPS.
Please apply the following changes:
• In Cargo.toml, add the nanoid dependency (choose the latest compatible version, for example 0.4):
[dependencies]
+ nanoid = "0.4"• In twmq/src/lib.rs, update the batch pop ID to length 8:
- let pop_id = nanoid::nanoid!(4);
+ let pop_id = nanoid::nanoid!(8);After applying these, verify that the crate compiles and the identifiers are generated at the new length.
🤖 Prompt for AI Agents
In twmq/src/lib.rs around lines 577-578 and Cargo.toml: the code uses the
nanoid::nanoid! macro but the crate is not declared and the generated ID length
is only 4; add the nanoid dependency to Cargo.toml (e.g., nanoid = "0.4") and
update the macro call in twmq/src/lib.rs to generate an 8-character ID (replace
nanoid!(4) with nanoid!(8)); after changes, run cargo build to verify the crate
compiles and that identifiers are produced at the new length.
7702 Changes:
TwGetDelegationContractResponsestruct to encapsulate the delegation contract address.tw_get_delegation_contractmethod inBundlerClientto fetch the delegation contract.DelegatedAccountandMinimalAccountTransactionto accept delegation contract as a parameter.DelegationContractCachefor efficient retrieval of delegation contracts in the executor.EOA Changes:
Monitoring Changes:
Summary by CodeRabbit
New Features
Performance
Reliability
Metrics