clean and merge local changes #1
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces user-initiated job cancellation to the durable queue system, adds a transaction registry for tracking transaction states, and refactors job handling to use a borrowed job wrapper with lease tokens. The API layer is enhanced with OpenAPI (utoipa) documentation, new signing endpoints, and transaction cancellation support. Several internal modules and structs are extended or refactored to support these features. Changes
Sequence Diagram(s)User-Initiated Job Cancellation FlowsequenceDiagram
participant Client
participant Server (Axum)
participant TransactionRegistry
participant Queue
participant Worker
Client->>Server (Axum): POST /transaction/{id}/cancel
Server (Axum)->>TransactionRegistry: get_transaction_queue(id)
TransactionRegistry-->>Server (Axum): queue_name or None
alt queue_name == "external_bundler_send"
Server (Axum)->>Queue: cancel_job(id)
Queue->>Redis: [atomic cancellation script]
Redis-->>Queue: CancelResult
alt CancelledImmediately
Queue->>Worker: process_cancelled_job(id)
Worker->>Queue: fail job with UserCancelled error
Queue-->>Server (Axum): CancelledImmediately
else CancellationPending
Queue-->>Server (Axum): CancellationPending
end
Server (Axum)->>TransactionRegistry: remove_transaction(id) (if cancelled)
else queue_name == "userop_confirm"
Server (Axum)-->>Client: CannotCancel (already sent)
else queue_name == None
Server (Axum)-->>Client: NotFound
else
Server (Axum)-->>Client: CannotCancel (unsupported queue)
end
Server (Axum)-->>Client: TransactionCancelResponse
BorrowedJob and Unified Completion FlowsequenceDiagram
participant Worker
participant Queue
participant Redis
loop Job Processing
Queue->>Redis: pop_batch_jobs()
Redis-->>Queue: [BorrowedJob(s) with lease tokens]
Worker->>Queue: complete_job(BorrowedJob, result)
Queue->>Redis: WATCH lease key
alt lease valid
Queue->>Redis: [pipeline: update job state, delete lease, etc.]
Redis-->>Queue: OK
Queue->>Worker: call hooks (on_success/on_nack/on_fail)
else lease expired/cancelled
Queue-->>Worker: retry or abort completion
end
end
Transaction Registration Before QueueingsequenceDiagram
participant ExecutionRouter
participant TransactionRegistry
participant Queue
ExecutionRouter->>TransactionRegistry: set_transaction_queue(id, "external_bundler_send")
TransactionRegistry-->>ExecutionRouter: OK or error
ExecutionRouter->>Queue: push(job)
Signing Endpoint Flow (EOA vs Smart Account)sequenceDiagram
participant Client
participant Server (Axum)
participant EoaSigner
participant SmartAccountSignerBuilder
participant SmartAccountSigner
Client->>Server (Axum): POST /sign/message or /sign/typed_data
Server (Axum)->>Server (Axum): for each message/typed_data
alt SigningOptions::Eoa
Server (Axum)->>EoaSigner: sign_message/sign_typed_data
EoaSigner-->>Server (Axum): signature
else SigningOptions::SmartAccount
Server (Axum)->>SmartAccountSignerBuilder: build()
SmartAccountSignerBuilder-->>SmartAccountSigner: signer
Server (Axum)->>SmartAccountSigner: sign_message/sign_typed_data
SmartAccountSigner-->>Server (Axum): signature
end
Server (Axum)-->>Client: aggregated results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✨ 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: 8
🧹 Nitpick comments (10)
aa-core/src/smart_account/mod.rs (1)
68-69: Approve derivingCloneandDebug.
DeterminedSmartAccountbenefits from cloning and debug printing. For completeness, consider also derivingPartialEqif comparisons appear in tests or logic.executors/src/lib.rs (1)
3-3: ```shell
#!/bin/bashCheck for module-level documentation in transaction_registry.rs
head -n 10 executors/src/transaction_registry.rs | grep -E '^//!|^///' -m 1 || echo "no module-level docs found"
</blockquote></details> <details> <summary>server/res/scalar.html (1)</summary><blockquote> `1-52`: **API documentation template looks good with minor security considerations.** The HTML template for Scalar API documentation is well-structured and minimal. However, consider the following for production deployment: 1. **External CDN dependency**: The Scalar library is loaded from `cdn.jsdelivr.net` which introduces an external dependency 2. **External favicon**: The favicon is loaded from `framerusercontent.com` 3. **Content Security Policy**: Consider implementing CSP headers to control resource loading The template structure and CSS customizations are appropriate for rendering API documentation. Consider hosting critical assets locally or implementing appropriate CSP headers: ```html <!-- Optional: Host Scalar library locally for better reliability --> <script src="/static/js/scalar-api-reference.js"></script> <!-- Or add integrity check for CDN --> <script src="https://cdn.jsdelivr.net/npm/@scalar/api-reference" integrity="sha384-..." crossorigin="anonymous"></script>aa-core/src/account_factory/chained.rs (1)
47-58: Good implementation with minor naming suggestionThe
implementation_addressmethod correctly follows the same pattern aspredict_address, including proper error handling with chain and factory context.Consider renaming the variable for clarity:
- let predicted_address = account_factory_contract + let implementation_address = account_factory_contract .accountImplementation() .call() .await .map_err(|e| e.to_engine_error(self.chain.chain_id(), Some(self.factory_address)))?; - Ok(predicted_address) + Ok(implementation_address)executors/src/transaction_registry.rs (1)
30-35: Consider adding prefix to registry key for better organization.The registry key construction could benefit from a standard prefix to avoid potential conflicts with other Redis keys.
fn registry_key(&self) -> String { match &self.namespace { - Some(ns) => format!("{}:tx_registry", ns), - None => "tx_registry".to_string(), + Some(ns) => format!("twmq:{}:tx_registry", ns), + None => "twmq:tx_registry".to_string(), } }server/src/execution_router/mod.rs (1)
76-98: Transaction registry integration looks good.The implementation correctly registers transactions before queuing and handles errors appropriately. The parameter type change from
&Vecto&[InnerTransaction]improves API flexibility.Consider extracting the queue name as a constant:
+const EXTERNAL_BUNDLER_SEND_QUEUE: &str = "external_bundler_send"; + impl ExecutionRouter { // ... async fn execute_external_bundler( // ... ) -> Result<(), TwmqError> { // ... self.transaction_registry .set_transaction_queue( &base_execution_options.idempotency_key, - "external_bundler_send", + EXTERNAL_BUNDLER_SEND_QUEUE, )executors/src/external_bundler/send.rs (1)
235-240: BorrowedJob usage and retry logic updated.The method signature and data access patterns are correctly updated. The retry logic now uses the job's attempt counter.
Consider making the retry limit configurable instead of hardcoding 100:
+const MAX_RETRY_ATTEMPTS: u32 = 100; + // if is_bundler_error_retryable(&e) { - if job.job.attempts < 100 { + if job.job.attempts < MAX_RETRY_ATTEMPTS {Also applies to: 253-253, 443-443
server/src/http/routes/sign_typed_data.rs (1)
90-179: Consider extracting common result typesThe result types (
SignResultItem,SignResultSuccessItem,SignResultFailureItem,SignResults) are duplicated between this file andsign_message.rs. This violates DRY principles.Consider creating a common module (e.g.,
server/src/http/routes/common/signing.rs) to house these shared types:// In common/signing.rs #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, utoipa::ToSchema)] #[serde(untagged)] pub enum SignResultItem { Success(SignResultSuccessItem), Failure(SignResultFailureItem), } // ... rest of the common typesThen both endpoints can import and reuse these types, reducing duplication and ensuring consistency.
aa-core/src/signer.rs (1)
254-254: Use hex crate for consistent encoding.Consider using the hex crate's
encode_prefixedfunction for consistency:-&format!("0x{}", hex::encode(signing_hash)), +&hex::encode_prefixed(signing_hash),twmq/src/lib.rs (1)
694-694: Consider using UUID for lease tokens to avoid collisions.The current lease token format concatenates timestamp, job_id, and attempts with underscores. If job_id contains underscores, this could theoretically lead to ambiguous tokens (though unlikely in practice).
Consider using a UUID or incorporating a hash for guaranteed uniqueness:
-local lease_token = now .. '_' .. job_id .. '_' .. attempts +-- Use a more robust token format +local lease_token = now .. ':' .. job_id .. ':' .. attempts .. ':' .. math.random(1000000)Alternatively, implement UUID generation in Rust and pass it to the script.
📜 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 (50)
aa-core/src/account_factory/chained.rs(2 hunks)aa-core/src/account_factory/default.rs(1 hunks)aa-core/src/account_factory/mod.rs(2 hunks)aa-core/src/lib.rs(1 hunks)aa-core/src/signer.rs(1 hunks)aa-core/src/smart_account/mod.rs(1 hunks)core/Cargo.toml(1 hunks)core/src/defs.rs(1 hunks)core/src/error.rs(5 hunks)core/src/execution_options/aa.rs(3 hunks)core/src/execution_options/auto.rs(1 hunks)core/src/execution_options/mod.rs(5 hunks)core/src/lib.rs(1 hunks)core/src/signer.rs(1 hunks)core/src/transaction.rs(1 hunks)executors/src/external_bundler/confirm.rs(12 hunks)executors/src/external_bundler/send.rs(9 hunks)executors/src/lib.rs(1 hunks)executors/src/transaction_registry.rs(1 hunks)executors/src/webhook/envelope.rs(8 hunks)executors/src/webhook/mod.rs(10 hunks)server/Cargo.toml(1 hunks)server/Dockerfile(1 hunks)server/bacon.toml(1 hunks)server/res/scalar.html(1 hunks)server/src/execution_router/mod.rs(4 hunks)server/src/http/dyn_contract.rs(1 hunks)server/src/http/error.rs(1 hunks)server/src/http/routes/contract_encode.rs(6 hunks)server/src/http/routes/contract_read.rs(9 hunks)server/src/http/routes/contract_write.rs(5 hunks)server/src/http/routes/mod.rs(1 hunks)server/src/http/routes/sign_message.rs(1 hunks)server/src/http/routes/sign_typed_data.rs(1 hunks)server/src/http/routes/transaction.rs(1 hunks)server/src/http/routes/transaction_write.rs(2 hunks)server/src/http/routes/working_read_contract.rs(0 hunks)server/src/http/server.rs(3 hunks)server/src/main.rs(3 hunks)server/src/queue/manager.rs(6 hunks)thirdweb-core/Cargo.toml(1 hunks)thirdweb-core/src/error.rs(3 hunks)twmq/benches/throughput.rs(5 hunks)twmq/src/job.rs(1 hunks)twmq/src/lib.rs(21 hunks)twmq/tests/basic_hook.rs(6 hunks)twmq/tests/delay.rs(2 hunks)twmq/tests/fixtures.rs(3 hunks)twmq/tests/lease_expiry.rs(5 hunks)twmq/tests/nack.rs(6 hunks)
💤 Files with no reviewable changes (1)
- server/src/http/routes/working_read_contract.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (84)
core/Cargo.toml (1)
20-20: Verifyutoipadependency configuration.
Ensure that the default features ofutoipa = "5.4.0"cover the derive macros needed for OpenAPI schema generation. If you encounter missing macros or errors, specify the required features explicitly (e.g.,features = ["macro"]).#!/bin/bash # Check default features for utoipa in Cargo metadata cargo metadata --format-version 1 --no-deps | jq '.packages[] | select(.name=="utoipa") | .features'thirdweb-core/Cargo.toml (1)
16-16: ```shell
#!/bin/bash
set -eecho "🔍 Searching for
utoipaderive macros in thirdweb-core source..."
rg '#[derive.*utoipa' -n thirdweb-core/src || echo "No derive macros found."echo
echo "🔍 Searching for any otherutoipa::usages in thirdweb-core..."
rg 'utoipa::' -n thirdweb-core/src || echo "Noutoipa::references found."echo
echo "🔍 Inspectingutoipaentry in thirdweb-core/Cargo.toml..."
grep -R 'utoipa' -n thirdweb-core/Cargo.toml</details> <details> <summary>core/src/lib.rs (1)</summary> `8-8`: **Ensure `signer` module presence in source.** Exposing `pub mod signer;` is correct, but verify that `core/src/signer.rs` exists and compiles cleanly. ```shell #!/bin/bash # Confirm presence of signer module file if [ ! -f core/src/signer.rs ]; then echo "Missing core/src/signer.rs" exit 1 fiaa-core/src/lib.rs (1)
2-2: LGTM: Clean module declarationThe addition of the
signermodule follows the existing pattern and properly exposes the signing functionality as part of the public API.server/src/http/error.rs (1)
10-10: LGTM: Consistent with OpenAPI migrationThe addition of
utoipa::ToSchemaderive aligns with the broader effort to migrate to utoipa for OpenAPI documentation generation. This maintains compatibility with existing derives while adding the new schema generation capability.server/Cargo.toml (1)
35-37: LGTM: Well-chosen dependencies for OpenAPI supportThe utoipa ecosystem dependencies are appropriately configured with relevant features. The versions are current and the feature selections align with the server's needs for OpenAPI documentation generation and Axum integration.
server/Dockerfile (2)
5-6: LGTM: Proper package installation with cleanupThe SSH and git package installation is correctly implemented with proper cleanup to minimize the Docker image size.
15-16: LGTM: Secure SSH setup for GitHubThe SSH configuration uses the standard
ssh-keyscanapproach to handle GitHub's host key verification, which is the recommended secure practice for CI environments.aa-core/src/account_factory/default.rs (1)
70-73: LGTM: Consistent async trait implementationThe
implementation_addressmethod correctly follows the same pattern as the existingpredict_addressmethod, usingstd::future::readyto wrap the synchronous implementation in a ready future. This maintains consistency with the trait interface while being efficient for the default factory case.core/src/transaction.rs (2)
8-8: LGTM! OpenAPI schema integration added correctly.The addition of
utoipa::ToSchemato the derive macro aligns with the broader OpenAPI documentation effort described in the AI summary.
11-11: Good consistency between schemars and utoipa schema attributes.The
#[schema(value_type = ...)]attributes correctly mirror the existing#[schemars(with = ...)]attributes, ensuring consistent schema generation between both crates. This maintains compatibility while adding OpenAPI support.Also applies to: 15-15, 19-19
core/src/defs.rs (1)
5-5: LGTM! Core type definitions enhanced for OpenAPI schema generation.The addition of
utoipa::ToSchemato the core type definitions (AddressDef,BytesDef,U256Def) is consistent and necessary for the OpenAPI integration. These types are referenced in other schema attributes throughout the codebase.Also applies to: 11-11, 17-17
server/src/http/routes/mod.rs (1)
5-7: LGTM! New route modules added for signing and transaction functionality.The addition of
sign_message,sign_typed_data, andtransactionmodules follows consistent naming conventions and aligns with the API enhancements described in the AI summary.thirdweb-core/src/error.rs (1)
5-5: LGTM! Error types enhanced for comprehensive OpenAPI documentation.The addition of
utoipa::ToSchemato the error enums (ThirdwebError,ThirdwebSerializationError,SerializableReqwestError) is consistent with the OpenAPI integration effort and ensures that error responses can be properly documented in the API schema.Also applies to: 20-20, 49-49
server/src/http/dyn_contract.rs (2)
21-21: LGTM: Proper utoipa integrationThe addition of
utoipa::ToSchemato the derive list enables automatic OpenAPI schema generation for theContractCallstruct, which aligns with the broader documentation effort.
26-26: LGTM: Correct schema type annotationsThe
#[schema(value_type = ...)]attributes properly specify the schema types for OpenAPI documentation:
AddressDeffor the contract address fieldOption<JsonValue>for the optional ABI fieldThese annotations ensure accurate API documentation generation.
Also applies to: 34-34
server/src/queue/manager.rs (5)
13-13: LGTM: Proper import additionThe import of
TransactionRegistryfromengine_executorsis correctly placed and necessary for the new functionality.
26-26: LGTM: Well-structured field additionThe
transaction_registryfield is properly typed asArc<TransactionRegistry>for shared ownership across handlers.
49-54: LGTM: Proper registry initializationThe transaction registry is correctly initialized with:
- Redis connection manager from the existing client
- Execution namespace from queue configuration
The error handling follows the existing pattern.
113-113: LGTM: Consistent handler integrationThe transaction registry is properly passed to both confirmation and send handlers using
clone()on theArc, ensuring shared access while maintaining thread safety.Also applies to: 133-133
149-149: LGTM: Complete struct initializationThe transaction registry is properly included in the
QueueManagerstruct initialization, maintaining consistency with the field definition.twmq/src/job.rs (2)
148-153: LGTM: Well-designed wrapper structThe
BorrowedJobstruct effectively wraps aJobwith a lease token, enabling lease-aware job processing. The public fields provide necessary access while maintaining clarity about the borrowed nature of the job.
155-172: LGTM: Clean implementation with helpful convenience methodsThe implementation provides:
- A straightforward constructor following standard patterns
- Convenient accessor methods (
id(),data(),attempts()) that delegate to the inner job- Proper method signatures that maintain the original types
This design reduces boilerplate when accessing job fields through the borrowed wrapper.
aa-core/src/account_factory/mod.rs (2)
46-46: LGTM: Consistent trait method additionThe
implementation_addressmethod follows the same pattern aspredict_address, usingimpl Futurefor async operations and returningResult<Address, EngineError>. The+ Sendbound is properly included for thread safety.
75-80: LGTM: Proper enum implementationThe
SmartAccountFactoryenum correctly implements the new method by delegating to the appropriate variant implementations, maintaining consistency with the existingpredict_addresspattern.aa-core/src/account_factory/chained.rs (1)
21-21: LGTM: Appropriate contract interface extensionThe
accountImplementation()function is correctly added as an external view function to retrieve the implementation address from the factory contract.core/src/execution_options/auto.rs (1)
12-19: LGTM! Well-documented automatic execution options.The struct is clearly documented with comprehensive explanation of the automatic strategy selection logic. The derives are appropriate for API integration.
Consider adding validation to ensure the
fromfield contains a valid identifier format, though this may be handled elsewhere in the execution flow.core/src/error.rs (1)
16-16: LGTM! Consistent OpenAPI schema generation additions.The
utoipa::ToSchemaderives are consistently added across all relevant error types, and the schema annotation on thecontract_addressfield properly complements the existing schemars attribute for OpenAPI documentation.Also applies to: 61-61, 102-102, 162-162, 201-201
server/bacon.toml (1)
1-116: LGTM! Comprehensive bacon tool configuration.The configuration file provides a well-organized set of development jobs including check, clippy, test, doc, and run commands with appropriate settings and helpful comments.
server/src/http/routes/transaction_write.rs (1)
18-43: LGTM! Clean migration to utoipa with comprehensive OpenAPI documentation.The migration from aide to utoipa is well-executed with detailed OpenAPI specifications including proper request body, response types, and authentication headers. The updated return type maintains compatibility while supporting the new documentation approach.
twmq/tests/fixtures.rs (2)
10-11: LGTM! Proper integration of UserCancellable trait.The addition of the
UserCancellabletrait implementation provides standardized handling for user-initiated cancellations, consistent with the broader system refactor.Also applies to: 39-45
60-60: LGTM! Consistent migration to BorrowedJob pattern.The updates properly migrate from
JobtoBorrowedJobusage, including correct method signatures and job data access patterns (job.job.datainstead ofjob.data). This aligns with the lease-aware job processing refactor.Also applies to: 63-63, 69-69, 75-75, 81-81
server/src/main.rs (4)
3-3: LGTM! Import addition is appropriate.The import of
EoaSigneralongsideUserOpSignercorrectly supports the dual signer architecture being introduced.
41-44: Consider consistent naming and verify vault client sharing.The variable naming change from
signerto specific signer types improves clarity. However, both signers share the samevault_clientinstance, which is appropriate for resource efficiency.Consider using more descriptive variable names:
- let signer = Arc::new(UserOpSigner { + let userop_signer = Arc::new(UserOpSigner { vault_client: vault_client.clone(), }); let eoa_signer = Arc::new(EoaSigner { vault_client });
69-74: LGTM! State structure properly updated.The
EngineServerStateis correctly updated to include both signer types and the queue manager. The field naming is consistent and the Arc wrapping is appropriate for shared state.
65-65: Verify transaction_registry integration.The addition of
transaction_registryfield toExecutionRouterlooks correct, but ensure that this registry is properly used throughout the execution flow.Run this script to verify the transaction_registry usage:
#!/bin/bash # Description: Verify transaction_registry is properly used in execution_router module # Search for transaction_registry usage in execution router ast-grep --pattern 'transaction_registry.$_' # Also check for any TODO/FIXME comments related to transaction registry rg -A 3 -B 3 "TODO|FIXME.*transaction.*registry"twmq/benches/throughput.rs (4)
13-13: LGTM! Import additions support new functionality.The import of
BorrowedJobandUserCancellablecorrectly supports the refactored job handling patterns.
48-54: LGTM! UserCancellable implementation is appropriate.The implementation provides a meaningful cancellation message that aligns with the benchmark context.
112-115: LGTM! Method signature updated correctly.The change from
Job<Self::JobData>to&BorrowedJob<Self::JobData>is consistent with the broader job borrowing refactor.
134-134: Verify consistent job data access patterns.The access pattern changes from
job.datatojob.job.dataandjob.id()look correct for theBorrowedJobwrapper.Run this script to ensure all job access patterns in benchmarks are consistent:
#!/bin/bash # Description: Verify consistent job access patterns in benchmark files # Search for any remaining old-style job access patterns rg "job\.data\." twmq/benches/ rg "job\.id(?!\(\))" twmq/benches/ # Verify new access patterns are used consistently rg "job\.job\." twmq/benches/ rg "job\.id\(\)" twmq/benches/Also applies to: 155-155
twmq/tests/delay.rs (4)
16-16: LGTM! Import updated correctly.The import of
BorrowedJobis appropriate for the updated job handling patterns.
63-63: LGTM! Method signatures updated consistently.Both
processandon_successmethods are correctly updated to use&BorrowedJob<Self::JobData>parameter type.Also applies to: 87-87
69-82: LGTM! Job data access patterns updated correctly.All job field accesses are consistently updated to use the
job.job.fieldpattern, which is appropriate for theBorrowedJobwrapper.
94-97: LGTM! Redis operations updated correctly.The access to job data and ID in the Redis operations is correctly updated to use the borrowed job pattern.
core/src/execution_options/aa.rs (4)
11-11: LGTM! OpenAPI schema derive added appropriately.Adding
utoipa::ToSchematoEntrypointVersionenum supports OpenAPI documentation generation.
65-84: LGTM! Comprehensive schema annotations added.The addition of
utoipa::ToSchemaderives and#[schema(value_type = ...)]attributes properly support OpenAPI schema generation while maintaining compatibility with existingschemarsannotations.
31-63: LGTM! Documentation improvements enhance clarity.The restructured documentation with explicit version numbers and addresses improves usability and understanding of the ERC-4337 configuration options.
97-132: Verify public visibility change impact.Making
EntrypointAndFactoryDetailsDeserHelperpublic enables its use in other modules, which appears intentional for the signer abstractions mentioned in the AI summary.Run this script to verify the usage of the now-public struct:
#!/bin/bash # Description: Verify usage of EntrypointAndFactoryDetailsDeserHelper in other modules # Search for imports or usage of EntrypointAndFactoryDetailsDeserHelper rg "EntrypointAndFactoryDetailsDeserHelper" --type rust # Check for any schema-related usage ast-grep --pattern 'use $_ EntrypointAndFactoryDetailsDeserHelper'server/src/http/routes/contract_encode.rs (4)
26-102: LGTM! Comprehensive schema annotations added.All request and response types are properly annotated with
utoipa::ToSchemaderives and appropriate schema attributes, enabling comprehensive OpenAPI documentation generation.
158-172: LGTM! Comprehensive OpenAPI path annotation.The
#[utoipa::path]annotation provides detailed API documentation including operation ID, tags, request/response schemas, and authentication headers.
4-8: LGTM! Import cleanup appropriate.The import adjustments support the new return type and maintain necessary functionality.
180-183: LGTM! Credential extraction simplified appropriately.The simplified credential extraction maintains the same functionality while being more concise and readable.
server/src/http/routes/contract_read.rs (3)
10-14: Import changes align with utoipa migration.The removal of aide-specific imports and adoption of axum's
IntoResponseis correct for the utoipa migration.
58-79: Schema annotations correctly handle special types.The
utoipa::ToSchemaderives and#[schema(value_type = ...)]annotations properly map Rust types to their OpenAPI representations, particularly forAddress→AddressDefandserde_bool→boolconversions.
189-211: Comprehensive endpoint documentation using utoipa.The
#[utoipa::path]macro provides complete OpenAPI documentation including operation ID, request/response schemas, and authentication headers. The migration from manual documentation to attribute-based documentation improves maintainability.server/src/http/server.rs (1)
20-28: New transaction management endpoints and state fields added.The addition of
cancel_transaction,sign_message, andsign_typed_dataendpoints, along with theeoa_signerandqueue_managerstate fields, properly implements the transaction cancellation and signing features described in the PR objectives.Also applies to: 49-64
twmq/tests/nack.rs (1)
66-77: Consistent migration to BorrowedJob wrapper.The updates to use
BorrowedJob<Self::JobData>and access job fields throughjob.jobare consistent with the queue system's enhancement for lease token management and cancellation support.Also applies to: 117-120, 129-131, 145-147
executors/src/transaction_registry.rs (1)
37-57: Well-structured transaction registry implementation.The registry provides a clean interface for managing transaction-to-queue mappings with proper error handling, optional results for queries, and support for atomic batch operations via Redis pipelines.
twmq/tests/lease_expiry.rs (1)
68-87: Consistent BorrowedJob adoption in lease expiry tests.The updates properly integrate the
BorrowedJobwrapper, maintaining consistency with the broader refactoring while preserving the lease expiry test functionality.server/src/execution_router/mod.rs (2)
19-19: Import looks good.The TransactionRegistry import is properly organized with other engine_executors imports.
29-29: Transaction registry field properly added.The field follows the same Arc pattern as other shared resources in the struct.
twmq/tests/basic_hook.rs (1)
19-19: BorrowedJob refactoring correctly implemented.All method signatures and data access patterns have been consistently updated to use the new
BorrowedJobwrapper type. The changes properly support the job cancellation and lease management features.Also applies to: 85-86, 92-92, 98-98, 107-107, 113-113, 128-130, 144-144, 150-150
server/src/http/routes/contract_write.rs (3)
2-6: Utoipa migration correctly implemented.The imports and schema derives are properly updated for the migration from aide to utoipa-based OpenAPI documentation.
Also applies to: 31-31, 42-42, 47-47
78-96: Comprehensive OpenAPI documentation added.The
#[utoipa::path]macro provides detailed API documentation including request/response schemas, headers, and example responses.
105-105: Return type and error handling properly updated.The changes align with standard axum patterns and simplify the error handling.
Also applies to: 185-185
core/src/execution_options/mod.rs (3)
2-4: Module and import additions look good.The imports support the custom deserializer implementation and the new auto execution options module.
Also applies to: 8-8
12-12: Schema annotations and Auto variant properly added.The utoipa::ToSchema derives enable OpenAPI documentation generation, and the Auto variant is correctly integrated with appropriate schema metadata.
Also applies to: 25-31, 59-59, 67-67, 75-75, 85-85, 109-109
124-124: Auto execution defaults to ERC4337.The implementation correctly defaults Auto execution options to the ERC4337 executor type.
executors/src/external_bundler/send.rs (4)
126-128: User cancellation support properly implemented.The
UserCancellederror variant andUserCancellabletrait implementation enable proper handling of user-initiated job cancellations.Also applies to: 138-142
187-187: Transaction registry field correctly added.The field follows the same Arc pattern as other shared resources in the handler.
472-476: Transaction registry lifecycle management implemented correctly.The registry operations properly track transaction state transitions:
- On success: Transaction moves from send queue to confirm queue
- On failure: Transaction is removed from the registry
Both operations use the Redis pipeline for atomicity.
Also applies to: 544-547
130-136: Error handling and lock management properly implemented.The error conversion provides better context, and deployment locks are correctly released in error scenarios using atomic pipeline operations.
Also applies to: 520-526, 549-555
server/src/http/routes/transaction.rs (2)
54-155: Well-implemented transaction cancellation handlerThe implementation properly handles different queue states, provides clear cancellation semantics, and includes comprehensive logging. The separation of concerns based on queue type is clean and maintainable.
79-79: Fix redundant error conversionThe error is being converted twice here. The
TwmqError::into(e)is unnecessary sincemap_erralready handles the conversion.- .map_err(|e| ApiEngineError(TwmqError::into(e)))? + .map_err(|e| ApiEngineError(e.into()))?Likely an incorrect or invalid review comment.
executors/src/webhook/mod.rs (1)
11-13: Consistent implementation of job cancellation supportThe addition of
UserCancellederror variant and implementation ofUserCancellabletrait aligns perfectly with the broader cancellation feature. The migration toBorrowedJobis handled correctly throughout.Also applies to: 93-108
server/src/http/routes/sign_message.rs (1)
172-190: Efficient concurrent message signing implementationExcellent use of
join_allfor parallel processing of multiple messages. The error handling and response construction are clean and maintainable.executors/src/webhook/envelope.rs (1)
9-9: Consistent BorrowedJob refactoringAll webhook methods have been properly updated to use
BorrowedJobwrapper, with consistent field access patterns throughout. The changes align perfectly with the broader refactoring effort.Also applies to: 97-97, 132-132, 173-173, 210-210
core/src/signer.rs (2)
183-185: Clarify the default account salt value.The function returns
"0x"which could be ambiguous. Is this intended to represent an empty bytes array, or should it be a specific default value like"0x0"or"0x00"?Please verify the expected behavior for the default account salt across the system to ensure compatibility.
96-166: Well-structured EOA signer implementation.The
EoaSignerimplementation properly delegates to the vault client with comprehensive error handling and logging. The async trait methods are correctly implemented.executors/src/external_bundler/confirm.rs (3)
85-89: Correct implementation of UserCancellable trait.The trait implementation properly provides the user cancellation error variant, enabling clean cancellation handling throughout the system.
219-224: Proper transaction cleanup in success hook.Good integration with the transaction registry to remove completed transactions. The atomic pipeline approach ensures consistency.
281-286: Consistent transaction cleanup in failure hook.The fail hook properly removes transactions from the registry for permanent failures, maintaining system consistency.
aa-core/src/signer.rs (1)
283-310: Well-implemented ERC-6492 signature encoding.The ERC-6492 signature creation correctly encodes all required components (factory address, init calldata, signature, and magic suffix) following the standard format.
twmq/src/lib.rs (1)
1099-1177: Excellent implementation of atomic job completion with lease awareness.The implementation properly handles:
- Atomic execution with WATCH/MULTI/EXEC
- Lease validation to detect cancellations
- Unlimited retry for handling race conditions
- Clean separation of pipeline building and execution
This ensures robust job completion even under concurrent access.
| // let v1_router = ApiRouter::new() | ||
| // // generate Scalar API References using the openapi spec route | ||
| // .api_route( | ||
| // "/read/contract", | ||
| // post_with(read_contract, read_contract_docs), | ||
| // ) | ||
| // .api_route( | ||
| // "/encode/contract", | ||
| // post_with(encode_contract, encode_contract_docs), | ||
| // ) | ||
| // .api_route( | ||
| // "/write/contract", | ||
| // post_with(write_contract, write_contract_docs), | ||
| // ) | ||
| // .api_route( | ||
| // "/write/transaction", | ||
| // post_with(write_transaction, write_transaction_docs), | ||
| // ) | ||
| // // We'll serve our generated document here. | ||
| // .route("/api.json", get(serve_api)) | ||
| // // .route("/smart-account/status", post(smart_account_status)) | ||
| // // .route("/userop/create", post(create_user_op)) | ||
| // // .route("/test", post(read_contract)) | ||
| // .layer(cors) | ||
| // .layer(TraceLayer::new_for_http()) | ||
| // // Generate the documentation. | ||
| // .route("/reference", Scalar::new("/v1/api.json").axum_route()) | ||
| // .with_state(state); | ||
|
|
||
| // let router = ApiRouter::new() | ||
| // .nest_api_service("/v1", v1_router) | ||
| // .finish_api(&mut api) | ||
| // // Expose the documentation to the handlers. | ||
| // .layer(Extension(Arc::new(api))); | ||
| // // Add more routes here |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out aide implementation.
The migration to utoipa is complete and functional. The commented-out aide code should be removed to maintain code cleanliness.
- // let v1_router = ApiRouter::new()
- // // generate Scalar API References using the openapi spec route
- // .api_route(
- // "/read/contract",
- // post_with(read_contract, read_contract_docs),
- // )
- // .api_route(
- // "/encode/contract",
- // post_with(encode_contract, encode_contract_docs),
- // )
- // .api_route(
- // "/write/contract",
- // post_with(write_contract, write_contract_docs),
- // )
- // .api_route(
- // "/write/transaction",
- // post_with(write_transaction, write_transaction_docs),
- // )
- // // We'll serve our generated document here.
- // .route("/api.json", get(serve_api))
- // // .route("/smart-account/status", post(smart_account_status))
- // // .route("/userop/create", post(create_user_op))
- // // .route("/test", post(read_contract))
- // .layer(cors)
- // .layer(TraceLayer::new_for_http())
- // // Generate the documentation.
- // .route("/reference", Scalar::new("/v1/api.json").axum_route())
- // .with_state(state);
-
- // let router = ApiRouter::new()
- // .nest_api_service("/v1", v1_router)
- // .finish_api(&mut api)
- // // Expose the documentation to the handlers.
- // .layer(Extension(Arc::new(api)));
- // // Add more routes here📝 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 v1_router = ApiRouter::new() | |
| // // generate Scalar API References using the openapi spec route | |
| // .api_route( | |
| // "/read/contract", | |
| // post_with(read_contract, read_contract_docs), | |
| // ) | |
| // .api_route( | |
| // "/encode/contract", | |
| // post_with(encode_contract, encode_contract_docs), | |
| // ) | |
| // .api_route( | |
| // "/write/contract", | |
| // post_with(write_contract, write_contract_docs), | |
| // ) | |
| // .api_route( | |
| // "/write/transaction", | |
| // post_with(write_transaction, write_transaction_docs), | |
| // ) | |
| // // We'll serve our generated document here. | |
| // .route("/api.json", get(serve_api)) | |
| // // .route("/smart-account/status", post(smart_account_status)) | |
| // // .route("/userop/create", post(create_user_op)) | |
| // // .route("/test", post(read_contract)) | |
| // .layer(cors) | |
| // .layer(TraceLayer::new_for_http()) | |
| // // Generate the documentation. | |
| // .route("/reference", Scalar::new("/v1/api.json").axum_route()) | |
| // .with_state(state); | |
| // let router = ApiRouter::new() | |
| // .nest_api_service("/v1", v1_router) | |
| // .finish_api(&mut api) | |
| // // Expose the documentation to the handlers. | |
| // .layer(Extension(Arc::new(api))); | |
| // // Add more routes here |
🤖 Prompt for AI Agents
In server/src/http/server.rs between lines 78 and 112, remove all the
commented-out code related to the old aide implementation since the migration to
utoipa is complete. This will clean up the code by deleting the unused commented
blocks defining the v1_router and router with aide routes and layers.
| SpecificExecutionOptions::Auto(auto_execution_options) => { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Critical: Unimplemented Auto execution option will panic at runtime.
The todo!() macro will cause a panic if the Auto execution option is used. This could crash the service in production.
Consider implementing the Auto variant or returning a proper error:
- SpecificExecutionOptions::Auto(auto_execution_options) => {
- todo!()
- }
+ SpecificExecutionOptions::Auto(auto_execution_options) => {
+ Err(EngineError::ValidationError {
+ message: "Auto execution options are not yet implemented".to_string(),
+ })
+ }📝 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.
| SpecificExecutionOptions::Auto(auto_execution_options) => { | |
| todo!() | |
| } | |
| SpecificExecutionOptions::Auto(auto_execution_options) => { | |
| Err(EngineError::ValidationError { | |
| message: "Auto execution options are not yet implemented".to_string(), | |
| }) | |
| } |
🤖 Prompt for AI Agents
In server/src/execution_router/mod.rs around lines 65 to 67, the Auto variant of
SpecificExecutionOptions is currently unimplemented and uses todo!(), which will
panic at runtime if triggered. Replace the todo!() macro by implementing the
necessary logic to handle the Auto execution option properly or return a
suitable error result to avoid panics and ensure graceful error handling in
production.
| #[schema(value_type = MessageFormatDef)] | ||
| pub format: MessageFormat, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, Copy, utoipa::ToSchema)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum MessageFormatDef { | ||
| Text, | ||
| Hex, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant MessageFormatDef enum
You're already importing MessageFormat from vault_types::enclave::encrypted::eoa but creating a duplicate enum for schema purposes. This duplication can lead to maintenance issues.
Consider using the imported MessageFormat directly and ensure it has the necessary schema derives:
- #[schema(value_type = MessageFormatDef)]
+ #[schema(value_type = MessageFormat)]
pub format: MessageFormat,
}
-
-#[derive(Serialize, Deserialize, Debug, Clone, Copy, utoipa::ToSchema)]
-#[serde(rename_all = "lowercase")]
-pub enum MessageFormatDef {
- Text,
- Hex,
-}If MessageFormat doesn't have ToSchema derive, it should be added to the original type in vault_types.
📝 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.
| #[schema(value_type = MessageFormatDef)] | |
| pub format: MessageFormat, | |
| } | |
| #[derive(Serialize, Deserialize, Debug, Clone, Copy, utoipa::ToSchema)] | |
| #[serde(rename_all = "lowercase")] | |
| pub enum MessageFormatDef { | |
| Text, | |
| Hex, | |
| } | |
| #[schema(value_type = MessageFormat)] | |
| pub format: MessageFormat, | |
| } |
🤖 Prompt for AI Agents
In server/src/http/routes/sign_message.rs around lines 33 to 43, remove the
redundant MessageFormatDef enum since MessageFormat is already imported from
vault_types. Instead, use the imported MessageFormat directly and ensure it has
the necessary serde and utoipa::ToSchema derives in its original definition
within vault_types. If MessageFormat lacks the ToSchema derive, add it there to
avoid duplication and maintenance issues.
| let salt_data = if self.options.account_salt.starts_with("0x") { | ||
| Bytes::from_hex(self.options.account_salt.clone()).map_err(|e| { | ||
| EngineError::ValidationError { | ||
| message: format!("Failed to parse hex salt: {}", e), | ||
| } | ||
| })? | ||
| } else { | ||
| let hex_string = hex::encode(self.options.account_salt.clone()); | ||
| Bytes::from_hex(hex_string).map_err(|e| EngineError::ValidationError { | ||
| message: format!("Failed to encode salt as hex: {}", e), | ||
| })? | ||
| }; |
There was a problem hiding this comment.
Ambiguous salt parsing logic.
The current implementation has inconsistent behavior:
- Hex-prefixed strings (e.g., "0x1234") are parsed as hex bytes
- Non-prefixed strings (e.g., "mysalt") are converted to hex encoding of their UTF-8 bytes
This could lead to confusion. Consider either:
- Always treating input as hex (with or without prefix)
- Clearly documenting the expected format
- Using a more explicit type like
Bytesfor the salt
Consider this approach for consistent hex parsing:
-let salt_data = if self.options.account_salt.starts_with("0x") {
- Bytes::from_hex(self.options.account_salt.clone()).map_err(|e| {
- EngineError::ValidationError {
- message: format!("Failed to parse hex salt: {}", e),
- }
- })?
-} else {
- let hex_string = hex::encode(self.options.account_salt.clone());
- Bytes::from_hex(hex_string).map_err(|e| EngineError::ValidationError {
- message: format!("Failed to encode salt as hex: {}", e),
- })?
-};
+// Always treat as hex, adding 0x prefix if missing
+let salt_hex = if self.options.account_salt.starts_with("0x") {
+ self.options.account_salt.clone()
+} else {
+ format!("0x{}", self.options.account_salt)
+};
+let salt_data = Bytes::from_hex(salt_hex).map_err(|e| {
+ EngineError::ValidationError {
+ message: format!("Failed to parse salt as hex: {}", e),
+ }
+})?;📝 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 salt_data = if self.options.account_salt.starts_with("0x") { | |
| Bytes::from_hex(self.options.account_salt.clone()).map_err(|e| { | |
| EngineError::ValidationError { | |
| message: format!("Failed to parse hex salt: {}", e), | |
| } | |
| })? | |
| } else { | |
| let hex_string = hex::encode(self.options.account_salt.clone()); | |
| Bytes::from_hex(hex_string).map_err(|e| EngineError::ValidationError { | |
| message: format!("Failed to encode salt as hex: {}", e), | |
| })? | |
| }; | |
| // Always treat as hex, adding 0x prefix if missing | |
| let salt_hex = if self.options.account_salt.starts_with("0x") { | |
| self.options.account_salt.clone() | |
| } else { | |
| format!("0x{}", self.options.account_salt) | |
| }; | |
| let salt_data = Bytes::from_hex(salt_hex).map_err(|e| { | |
| EngineError::ValidationError { | |
| message: format!("Failed to parse salt as hex: {}", e), | |
| } | |
| })?; |
🤖 Prompt for AI Agents
In aa-core/src/signer.rs around lines 73 to 84, the salt parsing logic
inconsistently handles input by parsing hex-prefixed strings as hex bytes and
encoding non-prefixed strings as hex of UTF-8 bytes, which can cause confusion.
To fix this, standardize the parsing by always treating the input as a hex
string regardless of prefix presence, optionally stripping the "0x" prefix
before parsing. Alternatively, update the code to clearly document the expected
input format or refactor to accept a more explicit type like `Bytes` for the
salt to avoid ambiguity.
| let twmq_error = TwmqError::Runtime("Job processing failed with user error".to_string()); | ||
| let queue_error_hook_data = QueueInternalErrorHookData { error: &twmq_error }; | ||
| self.handler.on_queue_error(job, queue_error_hook_data, &mut tx_context).await; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid creating dummy error when actual error is available.
The code creates a generic TwmqError instead of using the actual error information available in the error parameter. This loses valuable context.
Consider refactoring to preserve the actual error:
-let twmq_error = TwmqError::Runtime("Job processing failed with user error".to_string());
-let queue_error_hook_data = QueueInternalErrorHookData { error: &twmq_error };
+// Create a more specific error that preserves the original error information
+let twmq_error = TwmqError::Runtime(format!("Job deserialization failed: {:?}", error));
+let queue_error_hook_data = QueueInternalErrorHookData { error: &twmq_error };Or better yet, consider updating the QueueInternalErrorHookData to accept the actual error type.
📝 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 twmq_error = TwmqError::Runtime("Job processing failed with user error".to_string()); | |
| let queue_error_hook_data = QueueInternalErrorHookData { error: &twmq_error }; | |
| self.handler.on_queue_error(job, queue_error_hook_data, &mut tx_context).await; | |
| // Create a more specific error that preserves the original error information | |
| let twmq_error = TwmqError::Runtime(format!("Job deserialization failed: {:?}", error)); | |
| let queue_error_hook_data = QueueInternalErrorHookData { error: &twmq_error }; | |
| self.handler.on_queue_error(job, queue_error_hook_data, &mut tx_context).await; |
🤖 Prompt for AI Agents
In twmq/src/lib.rs around lines 1191 to 1193, the code creates a generic
TwmqError instead of using the actual error parameter, losing valuable error
context. Refactor the code to pass the actual error parameter directly to
QueueInternalErrorHookData instead of creating a new TwmqError instance. If
necessary, update QueueInternalErrorHookData to accept the actual error type to
preserve full error information.
Summary by CodeRabbit
New Features
Improvements
Chores
utoipaand related crates for OpenAPI support.utoipa.