KMS #22
KMS #22
Conversation
- Introduced the `engine-eip7702-core` package with its dependencies in `Cargo.lock` and `Cargo.toml`. - Updated the `executors` module to include `engine-eip7702-core` as a dependency. - Refactored execution options in `eip7702.rs` to support new execution models for EIP-7702. - Enhanced transaction handling in the `eip7702_executor` to accommodate new sender details and improve compatibility with existing job structures. - Updated webhook and job data structures to reflect changes in sender details and execution options. - Improved error handling and retry logic in job processing to enhance robustness.
- Updated `Cargo.lock` to reflect new versions for several dependencies, including `alloy-consensus`, `alloy-eips`, and AWS-related packages. - Introduced `AwsKmsCredential` for AWS KMS signing, allowing integration of AWS KMS into the signing process. - Enhanced `SigningCredential` enum to support AWS KMS credentials. - Implemented error handling for AWS KMS signing errors in the `EngineError` enum. - Updated user operation signing logic to utilize AWS KMS for signing user operations. - Refactored HTTP extractors to support AWS KMS credentials extraction from headers. These changes improve the integration of AWS KMS for signing operations, enhancing the overall functionality and security of the application.
- Updated AWS KMS credential extraction to use a more consistent header naming convention. - Improved error handling for retryable RPC errors in the EoaExecutorWorkerError, allowing for better job management. - Refactored AWS KMS credential validation to ensure proper extraction of key IDs and regions from ARNs. These changes aim to streamline AWS KMS operations and enhance the robustness of error handling in the executor worker.
WalkthroughThis set of changes introduces AWS KMS-based signing support throughout the codebase, refactors EIP-7702 execution and transaction handling, and enhances error handling and retry logic. It adds a new Changes
Sequence Diagram(s)AWS KMS Signing FlowsequenceDiagram
participant Client
participant Server
participant Signer
participant AWS_KMS
Client->>Server: Sends request with AWS KMS headers
Server->>Server: Extract AWS KMS credentials
Server->>Signer: Request signature (message/tx/userop)
Signer->>AWS_KMS: Sign data using KMS key
AWS_KMS-->>Signer: Return signature
Signer-->>Server: Return signature
Server-->>Client: Respond with signature/result
EIP-7702 Execution Flow (Owner/Session Key)sequenceDiagram
participant User
participant Server
participant DelegatedAccount
participant Bundler
User->>Server: Submit EIP-7702 transaction (owner/session key)
Server->>DelegatedAccount: Build transaction (owner/session key)
DelegatedAccount->>Server: Return transaction data
Server->>Bundler: Submit transaction
Bundler-->>Server: Return result/receipt
Server-->>User: Respond with execution result
Estimated code review effortScore: 5 (~2–3 hours)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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: 5
🧹 Nitpick comments (12)
eip7702-core/Cargo.toml (1)
8-8: Consider using workspace dependency for tokio.Using a direct version specification for tokio might lead to version conflicts with other workspace crates. Consider using
tokio = { workspace = true }if the workspace defines a tokio version.-tokio = "1.44.2" +tokio = { workspace = true }core/src/userop.rs (1)
135-143: Consider simplifying error handling.The nested error structure with
SerialisableAwsSignerError::SignwrappingSerialisableAwsSdkError::Otherseems overly complex for this use case. Consider a more direct error mapping.- let signature = signer.sign_hash(&userophash).await.map_err(|e| { - EngineError::AwsKmsSignerError { - error: SerialisableAwsSignerError::Sign { - aws_sdk_error: SerialisableAwsSdkError::Other { - message: e.to_string(), - }, - }, - } - })?; + let signature = signer.sign_hash(&userophash).await.map_err(|e| { + EngineError::ValidationError { + message: format!("AWS KMS signing failed: {}", e), + } + })?;executors/src/eoa/worker/error.rs (1)
233-237: Consider expanding the non-retryable error patterns.While checking for "invalid chain" is a good start, consider adding more deterministic error patterns that should not be retried, such as "method not found", "invalid params", or "unauthorized".
RpcErrorKind::ErrorResp(resp) => { let message = resp.message.to_lowercase(); - // if the error message contains "invalid chain", it's not retryable - !message.contains("invalid chain") + // Check for deterministic errors that should not be retried + !(message.contains("invalid chain") + || message.contains("method not found") + || message.contains("invalid params") + || message.contains("unauthorized")) }core/src/credentials.rs (1)
36-44: Make the provider name configurable.The hardcoded provider name "engine-core" should be configurable or derived from the application context.
fn provide_credentials<'a>(&'a self) -> ProvideCredentialsFuture<'a> where Self: 'a, { let credentials = Credentials::new( self.access_key_id.clone(), self.secret_access_key.clone(), None, None, - "engine-core", + "engine-core-kms-signer", ); ProvideCredentialsFuture::ready(Ok(credentials)) }core/src/execution_options/eip7702.rs (1)
29-35: Consider adding validation for session key execution.The session key execution should validate that the session key address differs from the account address to prevent configuration errors.
Add a validation method:
impl Eip7702SessionKeyExecution { pub fn validate(&self) -> Result<(), ValidationError> { if self.session_key_address == self.account_address { return Err(ValidationError::SameAddresses); } Ok(()) } }eip7702-core/src/delegated_account.rs (2)
51-52: Define magic numbers as constants.The delegation code length check uses a hardcoded value that should reference the constant.
-if code.len() < EIP_7702_DELEGATION_CODE_LENGTH +// EIP-7702 delegation format: 3 bytes prefix + 20 bytes address = 23 bytes +if code.len() < EIP_7702_DELEGATION_CODE_LENGTH
65-66: Add bounds checking for bytecode parsing.While the length check ensures sufficient bytes exist, consider adding explicit bounds validation.
+// Ensure we have exactly the expected format +if code.len() != EIP_7702_DELEGATION_CODE_LENGTH { + return Ok(false); +} let target_bytes = &code[3..23]; let target_address = Address::from_slice(target_bytes);executors/src/eip7702_executor/confirm.rs (1)
34-39: Track the migration progress for deprecated fields.The dual-field approach with
eoa_address(deprecated) andsender_detailsis a good migration strategy. However, ensure there's a plan to:
- Track when all existing jobs have been processed
- Remove the deprecated field and make
sender_detailsnon-optionalConsider adding metrics or logging to track usage of the deprecated field.
core/src/error.rs (1)
330-353: Consider improving error message formatting for production.The conversion from
SdkError<T>usesDebugformatting (format!("{:?}", err)), which may produce verbose and less user-friendly error messages. Consider usingDisplayformatting or extracting specific error details for better production error messages.impl<T: Debug> From<SdkError<T>> for SerialisableAwsSdkError { fn from(err: SdkError<T>) -> Self { match err { SdkError::ConstructionFailure(err) => SerialisableAwsSdkError::ConstructionFailure { - message: format!("{:?}", err), + message: err.to_string(), }, SdkError::TimeoutError(err) => SerialisableAwsSdkError::TimeoutError { - message: format!("{:?}", err), + message: err.to_string(), }, SdkError::DispatchFailure(err) => SerialisableAwsSdkError::DispatchFailure { - message: format!("{:?}", err), + message: err.to_string(), }, SdkError::ResponseError(err) => SerialisableAwsSdkError::ResponseError { - message: format!("{:?}", err), + message: err.to_string(), }, SdkError::ServiceError(err) => SerialisableAwsSdkError::ServiceError { - message: format!("{:?}", err), + message: err.service_error().to_string(), }, _ => SerialisableAwsSdkError::Other { - message: format!("{:?}", err), + message: err.to_string(), }, } } }core/src/signer.rs (1)
289-301: Address the TODO for proper error serialization.The current implementation loses error details by wrapping all signing errors in a generic "Other" variant. This makes debugging difficult. Consider creating a proper serializable error type for alloy-signer errors or extracting more specific error information.
Would you like me to help create a proper serializable error type for alloy-signer errors that preserves error details?
executors/src/eip7702_executor/send.rs (2)
37-59: Good backward compatibility approach, but track the migration timeline.The dual-field approach with clear documentation is excellent for maintaining backward compatibility. However, consider adding a specific timeline or version target for when
eoa_addresswill be removed.Consider adding a more specific TODO comment:
- // !IMPORTANT TODO - // To preserve backwards compatibility with pre-existing queued jobs, we continue keeping the eoa_address field until the next release + // TODO: Remove eoa_address field after version X.Y.Z (target date: MM/YYYY) + // To preserve backwards compatibility with pre-existing queued jobs, we continue keeping the eoa_address field until the next release
425-494: Comprehensive retry logic implementation.The retry logic correctly distinguishes between transient errors (network issues) and permanent errors (client errors, contract reverts). The implementation matches the pattern used in
external_bundler/send.rs.Consider extracting these retry helper functions to a shared module to avoid duplication with
external_bundler/send.rs. This would ensure consistent retry behavior across executors.
📜 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 (29)
Cargo.toml(1 hunks)aa-types/src/userop.rs(3 hunks)core/Cargo.toml(1 hunks)core/src/constants.rs(0 hunks)core/src/credentials.rs(1 hunks)core/src/error.rs(2 hunks)core/src/execution_options/aa.rs(1 hunks)core/src/execution_options/eip7702.rs(1 hunks)core/src/execution_options/mod.rs(1 hunks)core/src/signer.rs(7 hunks)core/src/userop.rs(3 hunks)eip7702-core/Cargo.toml(1 hunks)eip7702-core/src/constants.rs(1 hunks)eip7702-core/src/delegated_account.rs(1 hunks)eip7702-core/src/lib.rs(1 hunks)eip7702-core/src/transaction.rs(1 hunks)executors/Cargo.toml(1 hunks)executors/src/eip7702_executor/confirm.rs(6 hunks)executors/src/eip7702_executor/send.rs(6 hunks)executors/src/eoa/error_classifier.rs(1 hunks)executors/src/eoa/store/atomic.rs(1 hunks)executors/src/eoa/worker/confirm.rs(2 hunks)executors/src/eoa/worker/error.rs(2 hunks)server/Cargo.toml(1 hunks)server/src/execution_router/mod.rs(1 hunks)server/src/http/error.rs(2 hunks)server/src/http/extractors.rs(6 hunks)thirdweb-core/src/iaw/mod.rs(1 hunks)twmq/src/job.rs(3 hunks)
🧠 Learnings (2)
executors/src/eoa/worker/confirm.rs (2)
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker/error.rs (2)
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
🧬 Code Graph Analysis (4)
executors/src/eoa/worker/error.rs (1)
executors/src/eoa/error_classifier.rs (1)
message(224-236)
twmq/src/job.rs (1)
twmq/src/multilane.rs (1)
delay(1342-1348)
eip7702-core/src/transaction.rs (2)
eip7702-core/src/delegated_account.rs (3)
chain(97-99)address(83-85)generate_random_uid(126-131)core/src/signer.rs (2)
sign_typed_data(178-183)sign_typed_data(305-360)
executors/src/eip7702_executor/send.rs (7)
eip7702-core/src/delegated_account.rs (2)
chain(97-99)new(29-31)executors/src/external_bundler/send.rs (5)
is_build_error_retryable(695-711)is_retryable_rpc_error(723-737)is_client_error(714-720)contains_revert_data(740-753)is_non_retryable_rpc_code(756-763)eip7702-core/src/transaction.rs (2)
account(184-186)authorization(189-191)executors/src/eip7702_executor/confirm.rs (1)
transaction_id(52-54)executors/src/eoa/store/borrowed.rs (1)
transaction_id(32-34)executors/src/eoa/store/submitted.rs (1)
transaction_id(73-78)executors/src/eoa/worker/error.rs (1)
is_retryable_rpc_error(229-240)
💤 Files with no reviewable changes (1)
- core/src/constants.rs
🧰 Additional context used
🧠 Learnings (2)
executors/src/eoa/worker/confirm.rs (2)
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker/error.rs (2)
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: #5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
🧬 Code Graph Analysis (4)
executors/src/eoa/worker/error.rs (1)
executors/src/eoa/error_classifier.rs (1)
message(224-236)
twmq/src/job.rs (1)
twmq/src/multilane.rs (1)
delay(1342-1348)
eip7702-core/src/transaction.rs (2)
eip7702-core/src/delegated_account.rs (3)
chain(97-99)address(83-85)generate_random_uid(126-131)core/src/signer.rs (2)
sign_typed_data(178-183)sign_typed_data(305-360)
executors/src/eip7702_executor/send.rs (7)
eip7702-core/src/delegated_account.rs (2)
chain(97-99)new(29-31)executors/src/external_bundler/send.rs (5)
is_build_error_retryable(695-711)is_retryable_rpc_error(723-737)is_client_error(714-720)contains_revert_data(740-753)is_non_retryable_rpc_code(756-763)eip7702-core/src/transaction.rs (2)
account(184-186)authorization(189-191)executors/src/eip7702_executor/confirm.rs (1)
transaction_id(52-54)executors/src/eoa/store/borrowed.rs (1)
transaction_id(32-34)executors/src/eoa/store/submitted.rs (1)
transaction_id(73-78)executors/src/eoa/worker/error.rs (1)
is_retryable_rpc_error(229-240)
🔇 Additional comments (35)
executors/src/eoa/store/atomic.rs (1)
22-22: LGTM! Clean removal of unused import.The removal of
SubmittedTransactionfrom the imports is appropriate as the type is not used anywhere in this file. The code uses other transaction types likeSubmittedTransactionDehydratedandSubmittedNoopTransactioninstead.executors/src/eoa/worker/confirm.rs (2)
7-8: LGTM! Clean removal of unused import.The removal of
SubmittedTransactionfrom the imports is appropriate as this type is not used in the confirm flow logic.
114-114: Good improvement for code clarity.Explicitly naming the ignored tuple element as
_replaced_txsmakes the code more readable and clearly communicates that replaced transactions are intentionally not used in the downstream confirmation logic.twmq/src/job.rs (4)
58-64: LGTM! Well-designed retry logic enhancement.The new
map_err_with_max_retriesmethod signature follows the existing pattern and adds essential retry limit functionality. The parameters are appropriately typed and named.
83-101: Excellent implementation of conditional retry logic.The implementation correctly handles the retry limit logic:
- When
current_attempts >= max_retries, it fails permanently withJobError::Fail- Otherwise, it creates a retryable
JobError::Nackwith the specified delay and positionThis provides graceful degradation from retryable to permanent failure, which is essential for robust job processing.
108-114: Consistent method signature design.The
nack_with_max_retriesmethod signature mirrors themap_err_with_max_retriesmethod, ensuring consistency across the trait implementations.
130-146: Implementation correctly mirrors the Result-based method.The logic is identical to the
map_err_with_max_retriesimplementation, ensuring consistent behavior between theToJobResultandToJobErrortraits. This maintains the expected contract for users of either trait.executors/src/eoa/error_classifier.rs (1)
245-245: Good cleanup of unused import.Removing the unused
ToJobResultimport is appropriate since theto_send_job_resultmethod only uses theToJobErrortrait. This reduces unnecessary imports and aligns with the code's actual usage.server/Cargo.toml (1)
44-44: Approve aws-arn dependency additionVersion 0.3.1 is confirmed as the latest stable release of the
aws-arncrate on crates.io, so no further version changes are needed.
- Location:
server/Cargo.toml, line 44Cargo.toml (1)
6-6: Appropriate addition of EIP-7702 core crate to workspace.Adding
eip7702-coreas a workspace member supports the modular architecture and EIP-7702 functionality mentioned in the PR objectives. The placement maintains logical ordering within the workspace.executors/Cargo.toml (1)
19-19: Correctly adds EIP-7702 core dependency.The
engine-eip7702-coredependency addition is properly configured with the correct version, path, and naming convention. This enables the executors module to utilize EIP-7702 functionality as intended.server/src/http/error.rs (1)
69-69: LGTM! Improved HTTP status code semantics for external service errors.The change from 500 to 502 for
VaultErrorand the new 502 mapping forAwsKmsSignerErrorcorrectly reflect that these are external service failures rather than internal application errors. Status code 502 (Bad Gateway) is semantically appropriate for upstream service failures.Also applies to: 86-86
core/Cargo.toml (1)
22-25: AWS dependencies verified as up-to-date and secure.All four crates in core/Cargo.toml match the latest versions on crates.io (1.0.23, 1.8.2, 1.79.0, 1.2.4) and no security advisories were found. No further action required.
eip7702-core/src/lib.rs (1)
1-3: LGTM! Clean module structure for the new EIP-7702 crate.The module organization is logical and follows Rust conventions. The module names clearly indicate their responsibilities within the EIP-7702 functionality.
core/src/execution_options/mod.rs (1)
30-30: JsonSchema derive removal from SpecificExecutionOptions is safe; import remains requiredThe
JsonSchemaderive was successfully removed fromSpecificExecutionOptionsand no other references toJsonSchemaexist for that type, so schema generation is unaffected. Theschemars::JsonSchemaimport incore/src/execution_options/mod.rsis still used by other derive macros in this file and should be retained.server/src/execution_router/mod.rs (1)
353-354: VerifyEip7702SendJobDataDefinition and Downstream HandlingI wasn’t able to locate the
Eip7702SendJobDatastruct definition in the server crate to confirm it declares:
eoa_address: Option<…>execution_options: Option<…>Please ensure that:
- The server‐side
Eip7702SendJobDatastruct is updated to makeeoa_addressoptional and to include the newexecution_optionsfield.- Any serializers/deserializers or RPC layers in the server accept
Noneforeoa_addressand properly handleSome(eip7702_execution_options)forexecution_options.- The executor (in
executors/src/eip7702_executor/send.rs) continues to resolve the correct owner/session‐key address wheneoa_addressisNoneandexecution_optionsis provided.thirdweb-core/src/iaw/mod.rs (1)
71-76: Clean modernization of Default implementation.The refactoring from manual
Defaultimplementation to the derived#[default]attribute on theTextvariant is idiomatic and makes the default choice more explicit in the code.core/src/execution_options/aa.rs (1)
1-10: Clean import reorganization.The consolidation of crate imports and separation of entrypoint constants to the
engine_aa_typescrate improves code organization and follows the logical separation of concerns.core/src/userop.rs (1)
4-4: Good addition of Signer trait import.The import of the
Signertrait is necessary for the new AWS KMS signing functionality.eip7702-core/src/constants.rs (1)
1-11: Constants Verified Against EIP-7702 Specification
- EIP_7702_DELEGATION_PREFIX
[0xef, 0x01, 0x00]matches the 3-byte0xef0100delegation designator.- EIP_7702_DELEGATION_CODE_LENGTH
23bytes is correct (3-byte prefix + 20-byte address).- MINIMAL_ACCOUNT_IMPLEMENTATION_ADDRESS (
0xD6999651Fc0964B9c6B444307a0ab20534a66560) is project-specific—please confirm this is the deployed address of your minimal EIP-7702 account implementation contract.No code changes required.
executors/src/eoa/worker/error.rs (1)
97-107: Good enhancement to RPC error retry logic.The refined error handling provides more granular control over retry behavior, distinguishing between retryable and non-retryable RPC errors. This prevents unnecessary retries for deterministic failures.
aa-types/src/userop.rs (2)
16-17: LGTM! Standard entrypoint addresses correctly defined.The entrypoint addresses match the official deployment addresses for UserOperation v0.6 and v0.7.
189-218: Well-structured versioned hash implementation.The implementation provides a clean API for hashing user operations with appropriate version-specific dispatch. The separation between default and custom entrypoint methods offers good flexibility.
core/src/execution_options/eip7702.rs (1)
6-15: Excellent refactoring to explicit execution modes.The enum-based approach clearly distinguishes between owner and session key execution, improving type safety and API clarity. The untagged serialization maintains backward compatibility.
eip7702-core/src/delegated_account.rs (1)
33-80: Well-implemented delegation check with good logging.The bytecode parsing correctly identifies EIP-7702 delegations and includes comprehensive debug logging for troubleshooting.
executors/src/eip7702_executor/confirm.rs (1)
265-278: LGTM! Well-implemented migration logic.The fallback logic correctly handles the transition period by:
- Preferring the new
sender_detailsfield- Falling back to the deprecated
eoa_addresswrapped asEip7702Sender::Owner- Failing with a clear error if neither is present
This ensures backward compatibility while migrating to the new structure.
core/src/signer.rs (1)
387-387: Good fix for the IAW transaction signing.Correctly changed from passing
&transactiontotransactionto match the expected method signature.eip7702-core/src/transaction.rs (1)
1-232: Well-designed minimal account transaction system.The implementation provides a clean abstraction for EIP-7702 transactions with:
- Clear separation between session key and owner execution modes
- Proper EIP-712 typed data signing
- Good error handling and bundler integration
- Well-documented execution flows
The design elegantly handles the complexity of delegated execution.
executors/src/eip7702_executor/send.rs (7)
1-22: LGTM! Import changes align with the refactoring.The new imports for
DelegatedAccount,Eip7702ExecutionOptions, error handling types, and retry logic utilities are appropriate for the refactored implementation.
74-96: Well-designed sender enum structure.The
Eip7702Senderenum with untagged serialization and flattened fields provides a clean API response structure that distinguishes between owner and session key execution modes.
99-126: Improved error structure with better consistency.The simplified error variants with
EngineErroras the inner error type provide better error context and consistency. The descriptive error messages clearly indicate the failure points.
211-229: Correct backward compatibility handling for address resolution.The logic properly handles both legacy
eoa_addressand newexecution_optionsformats, with appropriate error handling when neither is present.
230-279: Well-structured transaction building with appropriate retry logic.The implementation correctly handles both owner and session key transactions, with sophisticated retry logic that distinguishes between retryable and non-retryable errors.
280-335: Proper bundler integration with consistent error handling.The bundler call correctly passes all required parameters including the optional authorization, and the result construction properly populates the sender details based on the execution mode.
354-360: Consistent with the migration strategy.Setting
eoa_addresstoNoneand usingsender_detailsin the confirmation job aligns with the migration away from the legacy field.
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct AwsKmsCredential { | ||
| pub access_key_id: String, | ||
| pub secret_access_key: String, | ||
| pub key_id: String, | ||
| pub region: String, | ||
| } |
There was a problem hiding this comment.
Security concern: Storing AWS credentials in memory.
Storing AWS access keys directly in the struct poses security risks. Consider using AWS STS temporary credentials or IAM roles instead of long-lived access keys.
Consider implementing support for:
- AWS STS AssumeRole for temporary credentials
- IAM instance roles for EC2/ECS/Lambda environments
- Credential rotation mechanisms
🤖 Prompt for AI Agents
In core/src/credentials.rs around lines 23 to 29, the AwsKmsCredential struct
currently stores long-lived AWS access keys directly, which is a security risk.
Refactor the code to support AWS STS AssumeRole for obtaining temporary
credentials, integrate IAM roles for EC2/ECS/Lambda environments to avoid
hardcoding credentials, and implement credential rotation mechanisms to refresh
credentials periodically. This will enhance security by avoiding persistent
sensitive data in memory.
| pub async fn get_signer(&self, chain_id: Option<ChainId>) -> Result<AwsSigner, EngineError> { | ||
| let config = aws_config::defaults(BehaviorVersion::latest()) | ||
| .credentials_provider(self.clone()) | ||
| .region(Region::new(self.region.clone())) | ||
| .load() | ||
| .await; | ||
| let client = aws_sdk_kms::Client::new(&config); | ||
|
|
||
| let signer = AwsSigner::new(client, self.key_id.clone(), chain_id).await?; | ||
| Ok(signer) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout configuration for AWS operations.
AWS SDK operations should have timeout configurations to prevent hanging requests.
pub async fn get_signer(&self, chain_id: Option<ChainId>) -> Result<AwsSigner, EngineError> {
let config = aws_config::defaults(BehaviorVersion::latest())
.credentials_provider(self.clone())
.region(Region::new(self.region.clone()))
+ .timeout_config(
+ aws_config::timeout::TimeoutConfig::builder()
+ .operation_timeout(std::time::Duration::from_secs(30))
+ .build()
+ )
.load()
.await;
let client = aws_sdk_kms::Client::new(&config);
let signer = AwsSigner::new(client, self.key_id.clone(), chain_id).await?;
Ok(signer)
}🤖 Prompt for AI Agents
In core/src/credentials.rs around lines 48 to 58, the AWS SDK client is created
without any timeout configuration, which can cause requests to hang
indefinitely. Modify the AWS SDK client or config builder to include a timeout
setting for AWS operations, ensuring that requests will fail gracefully if they
exceed the specified duration. Use the appropriate timeout configuration method
provided by the AWS SDK or underlying HTTP client to set a reasonable timeout
value.
| SigningCredential::AwsKms(creds) => { | ||
| let signer = creds.get_signer(Some(params.chain_id)).await?; | ||
| let userophash = params.userop.hash(params.chain_id).map_err(|e| { | ||
| EngineError::ValidationError { | ||
| message: format!("Failed to hash userop: {}", e), | ||
| } | ||
| })?; | ||
|
|
||
| let signature = signer.sign_hash(&userophash).await.map_err(|e| { | ||
| EngineError::AwsKmsSignerError { | ||
| error: SerialisableAwsSignerError::Sign { | ||
| aws_sdk_error: SerialisableAwsSdkError::Other { | ||
| message: e.to_string(), | ||
| }, | ||
| }, | ||
| } | ||
| })?; | ||
|
|
||
| Ok(Bytes::copy_from_slice(&signature.as_bytes())) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the userop.hash() method implementation.
The AWS KMS signing logic looks correct, but please verify that the hash() method on VersionedUserOp is properly implemented and tested, as this is critical for generating correct signatures.
🏁 Script executed:
#!/bin/bash
# Search for the hash method implementation on VersionedUserOp
ast-grep --pattern 'impl $_ for VersionedUserOp {
$$$
fn hash($$$) {
$$$
}
$$$
}'
# Also search for any trait implementations that might provide the hash method
rg -A 10 "impl.*VersionedUserOp" --type rust
rg -A 5 "fn hash.*VersionedUserOp\|VersionedUserOp.*hash" --type rustLength of output: 874
🏁 Script executed:
#!/bin/bash
# Extract compute_user_op_v06_hash and compute_user_op_v07_hash implementations
rg -n -A10 "fn compute_user_op_v06_hash" aa-types/src/userop.rs
rg -n -A10 "fn compute_user_op_v07_hash" aa-types/src/userop.rs
# Search for any tests referencing compute_user_op_v06_hash or VersionedUserOp::hash
rg -n "compute_user_op_v06_hash" .
rg -n "VersionedUserOp.*hash" .
rg -n "#\[test\]" aa-types/srcLength of output: 2267
Verify VersionedUserOp.hash implementation and add missing tests
I confirmed that VersionedUserOp::hash is implemented in aa-types/src/userop.rs by delegating to:
compute_user_op_v06_hash(op, ENTRYPOINT_ADDRESS_V0_6, chain_id)compute_user_op_v07_hash(op, ENTRYPOINT_ADDRESS_V0_7, chain_id)
However, I didn’t find any unit tests covering these hashing functions. Please:
- Review the implementations in
aa-types/src/userop.rs(lines 36–46 and 74–84) to ensure they match your protocol spec. - Add or update unit tests that assert the correct hash values for both V0_6 and V0_7 user-ops across representative inputs.
This will prevent signature mismatches in production.
🤖 Prompt for AI Agents
In core/src/userop.rs around lines 127 to 146, the code uses
VersionedUserOp::hash but lacks unit tests verifying its correctness. Review the
hash implementations in aa-types/src/userop.rs at lines 36–46 and 74–84 to
confirm they align with the protocol specification. Then, add or update unit
tests in aa-types/src/userop.rs that cover compute_user_op_v06_hash and
compute_user_op_v07_hash, asserting expected hash outputs for representative
VersionedUserOp inputs to prevent signature mismatches.
| /// Generate a random UID for wrapped calls | ||
| pub fn generate_random_uid() -> FixedBytes<32> { | ||
| let mut rng = rand::rng(); | ||
| let mut bytes = [0u8; 32]; | ||
| rng.fill(&mut bytes); | ||
| FixedBytes::from(bytes) | ||
| } |
There was a problem hiding this comment.
Use cryptographically secure random for UID generation.
The current implementation uses rand::rng() which may not be cryptographically secure. For security-sensitive operations, use a CSPRNG.
pub fn generate_random_uid() -> FixedBytes<32> {
- let mut rng = rand::rng();
+ use rand::{RngCore, rngs::OsRng};
+ let mut rng = OsRng;
let mut bytes = [0u8; 32];
- rng.fill(&mut bytes);
+ rng.fill_bytes(&mut bytes);
FixedBytes::from(bytes)
}📝 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.
| /// Generate a random UID for wrapped calls | |
| pub fn generate_random_uid() -> FixedBytes<32> { | |
| let mut rng = rand::rng(); | |
| let mut bytes = [0u8; 32]; | |
| rng.fill(&mut bytes); | |
| FixedBytes::from(bytes) | |
| } | |
| /// Generate a random UID for wrapped calls | |
| pub fn generate_random_uid() -> FixedBytes<32> { | |
| use rand::{RngCore, rngs::OsRng}; | |
| let mut rng = OsRng; | |
| let mut bytes = [0u8; 32]; | |
| rng.fill_bytes(&mut bytes); | |
| FixedBytes::from(bytes) | |
| } |
🤖 Prompt for AI Agents
In eip7702-core/src/delegated_account.rs around lines 125 to 131, the
generate_random_uid function uses rand::rng(), which is not cryptographically
secure. Replace this with a cryptographically secure random number generator,
such as using the rand::rngs::OsRng or another CSPRNG from the rand crate, to
fill the bytes array securely before creating the FixedBytes instance.
- Updated `Cargo.lock` and `Cargo.toml` to reflect the new version `1.0.23` for the `alloy` package and added `serde_repr` as a dependency. - Introduced `PrivateKeySigner` for local signing in the `SigningCredential` enum, allowing for random private key generation for testing. - Enhanced the `AccountSigner` trait to support signing operations with the new `PrivateKey` variant. - Added 7702 tests for session keys as well as owner execution These changes improve the signing capabilities and facilitate testing with local private keys.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
core/src/credentials.rs (2)
35-41: Security concern: Storing AWS credentials in memory.This is the same security concern identified in the previous review regarding storing AWS access keys directly in the struct.
60-70: Add timeout configuration for AWS operations.This is the same timeout configuration issue identified in the previous review regarding missing timeout settings for AWS SDK operations.
🧹 Nitpick comments (13)
core/src/credentials.rs (1)
14-19: Consider making random generation conditional on test features.The
random_local()method creates production credentials that could accidentally be used outside of testing contexts.+ #[cfg(any(test, feature = "testing"))] pub fn random_local() -> Self { SigningCredential::PrivateKey(PrivateKeySigner::random()) }eip7702-core/tests/bytecode/erc20.sol (2)
22-32: Consider adding overflow protection for production-like testing.While this is a test contract, adding basic overflow checks would make the tests more realistic and prevent potential issues during testing.
function mint(uint256 amount) public { + require(totalSupply + amount >= totalSupply, "Overflow"); balanceOf[msg.sender] += amount; totalSupply += amount; emit Transfer(address(0), msg.sender, amount); } function mintTo(address to, uint256 amount) public { + require(totalSupply + amount >= totalSupply, "Overflow"); balanceOf[to] += amount; totalSupply += amount; emit Transfer(address(0), to, amount); }
34-40: Transfer to zero address should be handled.Consider adding a check to prevent transfers to the zero address, which is a common ERC-20 requirement.
function transfer(address to, uint256 amount) public returns (bool) { + require(to != address(0), "Transfer to zero address"); require(balanceOf[msg.sender] >= amount, "Insufficient balance"); balanceOf[msg.sender] -= amount; balanceOf[to] += amount; emit Transfer(msg.sender, to, amount); return true; }core/src/signer.rs (1)
285-286: TODO comment should be addressed.The TODO comment indicates missing serializable error handling for Alloy signer errors. This appears throughout the AWS KMS implementations.
Would you like me to help create a proper serializable error type for
alloy_signer::error::Errorto replace the current generic error handling?eip7702-core/src/transaction.rs (1)
256-280: Consider caching typed data for repeated signing.If the same transaction might be signed multiple times (e.g., for retries), consider caching the typed data to avoid recreation.
+ /// Cached typed data for signing + typed_data_cache: Option<TypedData>, + pub async fn sign_wrapped_calls<S: AccountSigner>( &self, signer: &S, credentials: &SigningCredential, ) -> Result<String, EngineError> { - let typed_data = self.create_wrapped_calls_typed_data(); + let typed_data = self.typed_data_cache.clone() + .unwrap_or_else(|| self.create_wrapped_calls_typed_data()); self.sign_typed_data(signer, credentials, &typed_data).await }executors/src/eip7702_executor/send.rs (1)
279-298: Bundler error handling could be more specific.The retry logic only checks for
BundlerErrorkind but doesn't handle other error types that might be retryable.let wrapped_error = Eip7702SendError::BundlerCallFailed { inner_error: engine_error.clone(), }; - if let EngineError::BundlerError { kind, .. } = &engine_error { - if is_retryable_rpc_error(kind) { + if is_build_error_retryable(&engine_error) { wrapped_error.nack_with_max_retries( Some(Duration::from_secs(2)), twmq::job::RequeuePosition::Last, 3, job.attempts(), ) - } else { - wrapped_error.fail() - } } else { wrapped_error.fail() }eip7702-core/tests/integration_tests.rs (7)
140-162: Consider returning errors instead of panicking in unimplemented methods.The use of
panic!in test code can make debugging harder and may cause unexpected test failures. Consider returning a proper error type instead.- fn bundler_client(&self) -> &engine_core::rpc_clients::BundlerClient { - // For tests, we don't need real bundler client - panic!("bundler_client not implemented for test chain") - } + fn bundler_client(&self) -> &engine_core::rpc_clients::BundlerClient { + // This could return a mock client or the trait could be refactored to return Result/Option + unimplemented!("bundler_client not needed for integration tests") + }
299-299: Consider making the Anvil port configurable to avoid conflicts.The hardcoded port 8545 could cause issues if multiple tests run in parallel or if the port is already in use.
-const ANVIL_PORT: u16 = 8545; +const ANVIL_PORT: u16 = 0; // Let Anvil choose an available portAlternatively, use an environment variable or test framework port allocation.
334-346: Consider extracting address retrieval into a helper method.The repeated pattern of extracting addresses from credentials could be simplified.
+ fn get_address(credential: &SigningCredential) -> Address { + match credential { + SigningCredential::PrivateKey(signer) => signer.address(), + _ => panic!("Expected PrivateKey credential"), + } + } + // Extract addresses from the credentials - let executor_address = match &executor_credentials { - SigningCredential::PrivateKey(signer) => signer.address(), - _ => panic!("Expected PrivateKey credential"), - }; - let developer_address = match &developer_credentials { - SigningCredential::PrivateKey(signer) => signer.address(), - _ => panic!("Expected PrivateKey credential"), - }; - let user_address = match &user_credentials { - SigningCredential::PrivateKey(signer) => signer.address(), - _ => panic!("Expected PrivateKey credential"), - }; + let executor_address = get_address(&executor_credentials); + let developer_address = get_address(&developer_credentials); + let user_address = get_address(&user_credentials);
385-385: Consider making the Base Sepolia RPC URL configurable.The hardcoded RPC URL could cause test failures if the service is unavailable or rate-limited.
- let base_sepolia_url = "https://84532.rpc.thirdweb.com".parse()?; + let base_sepolia_url = std::env::var("BASE_SEPOLIA_RPC_URL") + .unwrap_or_else(|_| "https://84532.rpc.thirdweb.com".to_string()) + .parse()?;
559-559: Consider using a random UID for session uniqueness.Using a fixed all-zero UID could cause issues if multiple sessions are created in tests.
- uid: [0u8; 32].into(), + uid: { + let mut uid = [0u8; 32]; + getrandom::getrandom(&mut uid).unwrap(); + uid.into() + },
801-801: Remove or document the reason for sleep.The sleep call may not be necessary if transactions are properly awaited.
- sleep(Duration::from_secs(1)).await; + // Remove sleep - transaction receipts are already awaited above
650-821: Consider splitting this large integration test into smaller focused tests.The test covers multiple scenarios which could be tested independently for better maintainability and debugging.
Consider splitting into:
test_minimal_account_detection()test_owner_transaction_with_delegation()test_session_key_establishment()test_session_key_transaction_execution()This would make failures easier to diagnose and tests more maintainable.
📜 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 (11)
Cargo.toml(2 hunks)core/src/credentials.rs(1 hunks)core/src/signer.rs(9 hunks)core/src/userop.rs(3 hunks)eip7702-core/Cargo.toml(1 hunks)eip7702-core/src/delegated_account.rs(1 hunks)eip7702-core/src/transaction.rs(1 hunks)eip7702-core/tests/bytecode/erc20.hex(1 hunks)eip7702-core/tests/bytecode/erc20.sol(1 hunks)eip7702-core/tests/integration_tests.rs(1 hunks)executors/src/eip7702_executor/send.rs(6 hunks)
🧬 Code Graph Analysis (2)
core/src/signer.rs (4)
eip7702-core/tests/integration_tests.rs (3)
sign_transaction(221-241)sign_authorization(243-270)signature(478-478)thirdweb-core/src/iaw/mod.rs (2)
sign_transaction(363-430)sign_authorization(433-504)executors/src/eoa/worker/transaction.rs (2)
sign_transaction(351-377)signature(370-370)eip7702-core/src/delegated_account.rs (1)
sign_authorization(102-123)
eip7702-core/src/transaction.rs (2)
eip7702-core/src/delegated_account.rs (3)
chain(97-99)address(83-85)generate_random_uid(126-131)core/src/signer.rs (2)
sign_typed_data(176-181)sign_typed_data(319-386)
✅ Files skipped from review due to trivial changes (2)
- eip7702-core/tests/bytecode/erc20.hex
- eip7702-core/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- core/src/userop.rs
- eip7702-core/src/delegated_account.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/src/signer.rs (4)
eip7702-core/tests/integration_tests.rs (3)
sign_transaction(221-241)sign_authorization(243-270)signature(478-478)thirdweb-core/src/iaw/mod.rs (2)
sign_transaction(363-430)sign_authorization(433-504)executors/src/eoa/worker/transaction.rs (2)
sign_transaction(351-377)signature(370-370)eip7702-core/src/delegated_account.rs (1)
sign_authorization(102-123)
eip7702-core/src/transaction.rs (2)
eip7702-core/src/delegated_account.rs (3)
chain(97-99)address(83-85)generate_random_uid(126-131)core/src/signer.rs (2)
sign_typed_data(176-181)sign_typed_data(319-386)
⏰ 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: coverage
- GitHub Check: test
🔇 Additional comments (24)
eip7702-core/tests/bytecode/erc20.sol (1)
1-74: Well-implemented mock ERC-20 for testing purposes.The contract provides a clean implementation of ERC-20 functionality suitable for integration tests. The additional
mint,mintTo, andburnfunctions are useful for test scenarios.core/src/signer.rs (5)
167-199: Good refactoring to simplify trait interface.Removing the associated type
SigningOptionsand using concreteEoaSigningOptionsmakes the trait more straightforward to implement and use.
274-297: AWS KMS message signing implementation is correct.The implementation properly handles different message formats (text vs hex) and provides appropriate error mapping for AWS KMS errors.
298-315: PrivateKey signing implementation is well-structured.The implementation correctly handles message format conversion and provides appropriate error mapping.
413-413: Good fix for IAW transaction signing.Passing the transaction by reference instead of by value aligns with the expected IAW client interface.
521-521: VerifyPrivateKeySignersync/async implementationI couldn’t locate a local
PrivateKeySignerdefinition in the repo (it may be imported from another crate), nor did I find anasync fn sign_hashon that type. Please confirm:
- That
PrivateKeySigneractually provides bothsign_hash_syncand the corresponding async signing method (e.g.sign_hash).- That calling the synchronous variant here is intentional for private‐key authorization.
If the async method isn’t supported or sync is required, no changes are needed; otherwise update to the async API.
eip7702-core/src/transaction.rs (5)
19-96: Comprehensive Solidity ABI definitions for EIP-7702.The sol! macro definitions provide complete coverage of the minimal account interface including session management and constraint handling.
115-158: Well-structured session key transaction flow.The implementation correctly handles the nested transaction structure where the bundler calls
executeWithSigon the session key address, which then callsexecuteon the target account.
192-204: Good conditional authorization pattern.The
add_authorization_if_neededmethod efficiently checks delegation status before adding authorization, avoiding unnecessary signing operations.
284-293: EIP-712 domain configuration is correct.The typed data creation properly sets up the EIP-712 domain with the correct name, version, chain ID, and verifying contract address.
150-150: Confirm cryptographic security of UID generation
generate_random_uidpopulates a 32-byte (256-bit) array viarand::rng().fill(&mut bytes), making the collision probability effectively negligible.- Please verify that
rand::rng()is backed by a cryptographically secure source (e.g.OsRngorthread_rng()).- If it isn’t, switch to a known CSPRNG (for example,
rand::rngs::OsRng) to guarantee collision resistance under high throughput.executors/src/eip7702_executor/send.rs (6)
41-53: Good backward compatibility strategy.The approach to maintain both
eoa_addressandexecution_optionsfields during the transition period prevents breaking changes for existing queued jobs.
86-96: Well-designed sender representation.The
Eip7702Senderenum clearly distinguishes between owner and session key transactions, providing better type safety and clearer data modeling.
240-260: Enhanced error handling with retry logic is well-implemented.The error classification and conditional retry logic based on error types provides robust handling of transient failures.
414-482: Comprehensive error classification logic.The error classification functions provide detailed analysis of error types, HTTP status codes, and revert data to determine retryability. The implementation is thorough and well-reasoned.
459-472: Revert data detection is thorough.The function correctly identifies common revert patterns and keywords that indicate contract execution failures that shouldn't be retried.
346-347: Confirmation handler already supportssender_detailsfallbackThe
Eip7702ConfirmationJobDatastruct inexecutors/src/eip7702_executor/confirm.rsdeclares both:
eoa_address: Option<Address>sender_details: Option<Eip7702Sender>And the confirmation logic does:
let sender_details = job_data .sender_details.clone() .or_else(|| job_data.eoa_address.map(|eoa| Eip7702Sender::Owner { eoa_address: eoa })) .ok_or_else(|| /* error */)?;This ensures that:
- New jobs with
sender_detailsare used directly.- Legacy jobs without
sender_detailsbut witheoa_addressare correctly handled.No further action needed here.
eip7702-core/tests/integration_tests.rs (7)
1-40: LGTM!The imports are well-organized and appropriate for the EIP-7702 integration test.
41-107: Test-only contract implementation looks good.The MockERC20 contract is well-structured for testing purposes. The public mint functions are appropriate for test scenarios.
172-271: LGTM!The MockEoaSigner implementation is well-structured for testing with private key credentials. Error handling is consistent and messages are clear.
368-381: LGTM!The account funding implementation is appropriate for test setup.
463-531: LGTM!The wrapped calls execution implementation correctly handles EIP-7702 delegated transactions with optional authorization.
563-592: LGTM!The EIP-712 session signing implementation is correct and follows the standard properly.
594-647: LGTM!The session establishment implementation correctly handles the executor calling createSessionWithSig on the granter's account.
| #[serde(skip)] | ||
| PrivateKey(PrivateKeySigner), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document security implications of PrivateKey variant.
The #[serde(skip)] attribute prevents serialization, but consider adding runtime validation to ensure this variant is only used in development/test environments.
+ /// Private key signer for testing and development
+ /// Note: This should only be used in test environments
+ #[serde(skip)]
+ #[cfg_attr(not(any(test, feature = "testing")), deprecated = "PrivateKey credentials should only be used in test environments")]
PrivateKey(PrivateKeySigner),🤖 Prompt for AI Agents
In core/src/credentials.rs at lines 31 to 32, the PrivateKey variant is marked
with #[serde(skip)] to avoid serialization, but lacks runtime checks. Add
validation logic to ensure this variant is only instantiated or used in
development or test environments by checking the environment at runtime and
returning an error or panic if used in production.
eip7702-core/src/transaction.rs
Outdated
| let inner_calls = transactions | ||
| .iter() | ||
| .map(|tx| Call { | ||
| target: tx.to.unwrap_or_default(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential None values in transaction parsing.
The code uses unwrap_or_default() for the to field, which could result in transactions to the zero address. Consider explicit validation.
- target: tx.to.unwrap_or_default(),
+ target: tx.to.ok_or_else(|| EngineError::ValidationError {
+ message: "Transaction must have a target address".to_string(),
+ })?,Note: This would require propagating the Result type through the calling methods.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In eip7702-core/src/transaction.rs at line 125, the use of unwrap_or_default()
on the tx.to field can silently convert None values to the zero address, which
may be incorrect. Replace unwrap_or_default() with explicit validation that
checks if tx.to is None and returns an error instead. Update the function
signature to return a Result type and propagate this change through all calling
methods to handle the error properly.
| let tx_request = tx_request.with_nonce(nonce); | ||
|
|
||
| let gas_fees = self.chain.provider().estimate_eip1559_fees().await?; | ||
|
|
||
| let tx_request = tx_request | ||
| .with_nonce(nonce) | ||
| .with_max_fee_per_gas(gas_fees.max_fee_per_gas) | ||
| .with_max_priority_fee_per_gas(gas_fees.max_priority_fee_per_gas); |
There was a problem hiding this comment.
Remove duplicate nonce and fee configuration.
The nonce is set twice and fees are configured after being set in the initial request.
- let tx_request = tx_request.with_nonce(nonce);
-
let gas_fees = self.chain.provider().estimate_eip1559_fees().await?;
let tx_request = tx_request
.with_nonce(nonce)
.with_max_fee_per_gas(gas_fees.max_fee_per_gas)
.with_max_priority_fee_per_gas(gas_fees.max_priority_fee_per_gas);📝 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.
| let tx_request = tx_request.with_nonce(nonce); | |
| let gas_fees = self.chain.provider().estimate_eip1559_fees().await?; | |
| let tx_request = tx_request | |
| .with_nonce(nonce) | |
| .with_max_fee_per_gas(gas_fees.max_fee_per_gas) | |
| .with_max_priority_fee_per_gas(gas_fees.max_priority_fee_per_gas); | |
| let gas_fees = self.chain.provider().estimate_eip1559_fees().await?; | |
| let tx_request = tx_request | |
| .with_nonce(nonce) | |
| .with_max_fee_per_gas(gas_fees.max_fee_per_gas) | |
| .with_max_priority_fee_per_gas(gas_fees.max_priority_fee_per_gas); |
🤖 Prompt for AI Agents
In eip7702-core/tests/integration_tests.rs around lines 427 to 434, the nonce is
set twice on tx_request and the fee configuration is applied after the initial
nonce setting, causing duplication. Remove the first
tx_request.with_nonce(nonce) call and keep only the second block where nonce and
fees are set together to avoid redundant assignments.
| let owner_address = job_data | ||
| .eoa_address | ||
| .or(job_data.execution_options.as_ref().map(|e| match e { | ||
| Eip7702ExecutionOptions::Owner(o) => o.from, | ||
| Eip7702ExecutionOptions::SessionKey(s) => s.session_key_address, | ||
| })) | ||
| .ok_or(Eip7702SendError::InternalError { | ||
| message: "No owner address found".to_string(), | ||
| }) | ||
| .map_err_fail()?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address resolution logic needs improvement.
The fallback chain for address resolution could be simplified and made more robust.
- let owner_address = job_data
- .eoa_address
- .or(job_data.execution_options.as_ref().map(|e| match e {
- Eip7702ExecutionOptions::Owner(o) => o.from,
- Eip7702ExecutionOptions::SessionKey(s) => s.session_key_address,
- }))
- .ok_or(Eip7702SendError::InternalError {
- message: "No owner address found".to_string(),
- })
- .map_err_fail()?;
+ let owner_address = match &job_data.execution_options {
+ Some(Eip7702ExecutionOptions::Owner(o)) => o.from,
+ Some(Eip7702ExecutionOptions::SessionKey(s)) => s.session_key_address,
+ None => job_data.eoa_address.ok_or(Eip7702SendError::InternalError {
+ message: "No owner address found in either execution_options or eoa_address".to_string(),
+ }).map_err_fail()?,
+ };📝 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.
| let owner_address = job_data | |
| .eoa_address | |
| .or(job_data.execution_options.as_ref().map(|e| match e { | |
| Eip7702ExecutionOptions::Owner(o) => o.from, | |
| Eip7702ExecutionOptions::SessionKey(s) => s.session_key_address, | |
| })) | |
| .ok_or(Eip7702SendError::InternalError { | |
| message: "No owner address found".to_string(), | |
| }) | |
| .map_err_fail()?; | |
| let owner_address = match &job_data.execution_options { | |
| Some(Eip7702ExecutionOptions::Owner(o)) => o.from, | |
| Some(Eip7702ExecutionOptions::SessionKey(s)) => s.session_key_address, | |
| None => job_data.eoa_address | |
| .ok_or(Eip7702SendError::InternalError { | |
| message: "No owner address found in either execution_options or eoa_address" | |
| .to_string(), | |
| }) | |
| .map_err_fail()?, | |
| }; |
🤖 Prompt for AI Agents
In executors/src/eip7702_executor/send.rs around lines 211 to 220, the current
address resolution logic uses nested option mapping that can be simplified.
Refactor the code to flatten the fallback chain by first checking if eoa_address
is present, otherwise directly match on execution_options to extract the owner
address or session key address. This will make the logic clearer and more robust
by avoiding nested closures and improving readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/http/extractors.rs (1)
109-139: Well-structured prioritized credential extraction.The implementation follows a clear priority order and provides comprehensive error handling. The fallback mechanism is logical and well-organized.
Consider shortening the error message to avoid potential information disclosure about the authentication methods supported:
- message: "Missing valid authentication credentials. Provide either AWS KMS headers (x-aws-kms-arn, x-aws-kms-access-key-id, x-aws-secret-access-key), IAW credentials (x-wallet-access-token + x-thirdweb-client-id + x-thirdweb-service-key), or Vault credentials (x-vault-access-token)".to_string(), + message: "Missing valid authentication credentials. Please provide the required headers for your authentication method.".to_string(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/http/extractors.rs(6 hunks)
🔇 Additional comments (3)
server/src/http/extractors.rs (3)
18-27: LGTM! Good practice using header constants.The introduction of header name constants improves maintainability and reduces the risk of typos in header name references throughout the code.
42-42: LGTM! Consistent use of header constants.The replacement of hardcoded header strings with constants is consistent with the constants introduction and improves maintainability.
Also applies to: 54-54, 64-64
141-238: Excellent validation and security practices in helper methods.The helper methods demonstrate thorough validation:
- AWS KMS ARN parsing validates format, service type, key ID, and region
- Proper error handling with descriptive messages
- Clean separation of concerns between different credential types
The comprehensive ARN validation using the
aws_arncrate is particularly well-implemented and follows security best practices.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores