Support IAW auth tokens for EOA signing #2
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces support for In-App Wallet (IAW) signing across the codebase. It adds a new IAW client module, updates signing credential extraction to prioritize IAW tokens, and extends signers to handle IAW-based signing. User operation types are refactored into a new shared crate, and configuration files are updated to include the IAW service URL. Error handling and logging are enhanced to accommodate the new signing flow. Changes
Sequence Diagram(s)IAW Signing Flow (Message/TypedData/UserOp)sequenceDiagram
participant Client
participant Server
participant IAWClient
participant IAWService
Client->>Server: Send request with x-wallet-token (+auth headers)
Server->>Server: Extract IAW credentials
Server->>IAWClient: sign_message / sign_typed_data / sign_userop (with tokens, data)
IAWClient->>IAWService: POST /sign-message or /sign-typed-data or /sign-userop
IAWService-->>IAWClient: Signature response
IAWClient-->>Server: Return signature
Server-->>Client: Respond with signature
Credential Extraction FlowsequenceDiagram
participant HTTPRequest
participant Extractor
HTTPRequest->>Extractor: Provide headers
alt x-wallet-token present
Extractor->>Extractor: Parse IAW credentials (token + secret/client/service)
Extractor-->>HTTPRequest: Return SigningCredential::Iaw
else x-vault-access-token present
Extractor->>Extractor: Parse Vault credentials
Extractor-->>HTTPRequest: Return SigningCredential::Vault
else
Extractor-->>HTTPRequest: Return validation error
end
User Operation Hashing (v0.6/v0.7)sequenceDiagram
participant UserOpSigner
participant aa-types
UserOpSigner->>aa-types: compute_user_op_v06_hash or compute_user_op_v07_hash
aa-types-->>UserOpSigner: Return UserOp hash (B256)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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
🔭 Outside diff range comments (2)
server/src/config.rs (1)
48-55: Makeiaw_serviceoptional or provide a default to avoid breaking existing deployments
ThirdwebUrlspreviously deserialized successfully when justrpc,bundler,vault,paymaster, andabi_servicewere present.
Adding the non-optionaliaw_service: Stringmeans every existingserver_*.yaml(and any env overrides) must now define this key or the whole service panics during boot atconfig.try_deserialize.Unless you control every running environment, consider:
pub struct ThirdwebUrls { pub rpc: String, pub bundler: String, pub vault: String, pub paymaster: String, pub abi_service: String, - pub iaw_service: String, + #[serde(default)] + pub iaw_service: Option<String>, }and propagate
Nonehandling where the client is built (e.g. fall back toVaultsigning or return a clear config error).
This preserves backward compatibility while still allowing the new feature.server/src/http/error.rs (1)
28-33: Potential sensitive data leakage in serializeddetailsThe entire
EngineErroris dumped into the response JSON under"details".
IfIAWError(wrapped byEngineError::IawError) contains the bearer auth token or any PII, it will leak to clients & logs.Audit the
Display/Debugimpls forIAWErrorand redact secrets, or omitdetailsfor that variant.
🧹 Nitpick comments (3)
thirdweb-core/Cargo.toml (1)
6-8: Adding heavyalloyfeature flags increases compile size & times
consensus,dyn-abi,eips,eip712pull in a large transitive set.
If onlyeip712is required for IAW signing, consider enabling the minimal subset to keep build times reasonable for downstream crates.Not a blocker, but worth revisiting.
server/src/main.rs (1)
41-42: Consider graceful degradation for IAW service availability.The IAW client initialization follows the established pattern. However, if the IAW service is unavailable at startup, the entire server will fail to start due to the
?operator.Consider whether graceful degradation would be more appropriate, where the server could start without IAW support and log a warning instead.
thirdweb-core/src/iaw/mod.rs (1)
175-191: TODO methods noted for future implementation.The
sign_typed_dataandsign_authorizationmethods are currently unimplemented. Since this is a WIP PR, this is expected.Would you like me to help implement these methods or create issues to track their implementation?
Also applies to: 252-269
📜 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 (13)
core/src/credentials.rs(1 hunks)core/src/error.rs(3 hunks)core/src/signer.rs(4 hunks)core/src/userop.rs(1 hunks)server/configuration/server_base.yaml(1 hunks)server/configuration/server_production.yaml(1 hunks)server/src/config.rs(1 hunks)server/src/http/error.rs(1 hunks)server/src/http/extractors.rs(1 hunks)server/src/main.rs(2 hunks)thirdweb-core/Cargo.toml(1 hunks)thirdweb-core/src/iaw/mod.rs(1 hunks)thirdweb-core/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-25T02:13:08.257Z
Learning: Wallet architecture should unify Wallet and Account interfaces, support in-app wallets (social/email login), smart wallets with account abstraction, and EIP-1193, EIP-5792, EIP-7702 standards.
server/configuration/server_base.yaml (1)
Learnt from: MananTank
PR: thirdweb-dev/js#7177
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/public-pages/erc20/_hooks/useTokenTransfers.ts:41-44
Timestamp: 2025-05-27T19:56:16.920Z
Learning: When reviewing hooks that use environment variables like NEXT_PUBLIC_DASHBOARD_THIRDWEB_CLIENT_ID for API calls, MananTank prefers not to add explicit validation checks for these variables, trusting they will be set in the deployment environment.
server/configuration/server_production.yaml (1)
Learnt from: MananTank
PR: thirdweb-dev/js#7177
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/public-pages/erc20/_hooks/useTokenTransfers.ts:41-44
Timestamp: 2025-05-27T19:56:16.920Z
Learning: When reviewing hooks that use environment variables like NEXT_PUBLIC_DASHBOARD_THIRDWEB_CLIENT_ID for API calls, MananTank prefers not to add explicit validation checks for these variables, trusting they will be set in the deployment environment.
🧬 Code Graph Analysis (1)
core/src/error.rs (1)
thirdweb-core/src/error.rs (2)
from(127-167)from(171-173)
🔇 Additional comments (16)
server/configuration/server_base.yaml (1)
12-13: Consistency / local-env parity
server_base.yamlnow containsiaw_service, butserver_local.yaml(not in diff) likely doesn’t. If local developers spin up the server without adding the key, deserialization will fail (see previous comment).Either:
- Add the key to all environment configs.
- Or switch the struct field to
Option<String>with a sensible default.This keeps onboarding friction low.
thirdweb-core/src/lib.rs (1)
4-5: Publicly exporting the newiawmodule looks goodExposing the module at the crate root keeps the API coherent with
abiandauth.
No issues spotted here.server/configuration/server_production.yaml (1)
12-12: Verify the IAW service URL is accessible and secure.The configuration follows the established pattern. Ensure the service URL is accessible and uses HTTPS for secure communication.
#!/bin/bash # Description: Verify the IAW service URL is accessible # Expected: HTTP 200 or similar success response curl -I https://embedded-wallet.thirdweb.comcore/src/userop.rs (1)
106-111: LGTM: Clear rejection of unsupported functionality.The explicit rejection of IAW credentials for UserOperation signing is well-implemented with a clear error message. This approach is better than silent failures or unexpected behavior.
server/src/main.rs (2)
4-4: LGTM: Appropriate import addition.
47-47: LGTM: Constructor update aligns with new architecture.The change from struct literal to constructor call properly integrates the IAW client alongside the existing vault client.
core/src/credentials.rs (2)
2-3: LGTM: Appropriate imports for IAW integration.
9-12: LGTM: Well-structured credential variant addition.The new
Iawvariant appropriately extends the signing credential system with the necessary authentication components. The named fields provide clarity about the required IAW authentication data.core/src/error.rs (2)
210-213: LGTM: Consistent error variant addition.The
IawErrorvariant follows the established pattern used forVaultErrorand includes appropriate schema annotations for API documentation.
466-472: LGTM: Error conversion follows established pattern.The conversion from
thirdweb_core::iaw::IAWErrortoEngineError::IawErroris consistent with other error conversions in this file, using.to_string()to extract the error message.server/src/http/extractors.rs (2)
95-140: LGTM! Prioritized IAW credential extraction is well implemented.The implementation correctly:
- Prioritizes IAW credentials over Vault credentials
- Validates required ThirdwebAuth headers when using IAW
- Provides clear error messages for missing headers
149-149: Good error message improvement.The updated error message clearly indicates both authentication options available to the user.
core/src/signer.rs (3)
8-8: LGTM! Clean integration of IAWClient into EoaSigner.The IAWClient is properly integrated as a required dependency alongside the existing VaultClient.
Also applies to: 168-175
207-231: LGTM! IAW message signing implementation is correct.The implementation properly:
- Converts MessageFormat between Vault and IAW formats
- Handles errors with appropriate logging
- Returns the signature in the expected format
254-270: LGTM! Consistent IAW typed data signing implementation.The implementation follows the same pattern as message signing with proper error handling and logging.
thirdweb-core/src/iaw/mod.rs (1)
117-117: Verify the authentication token format with IAW service.The format
Bearer embedded-wallet-token:{auth_token}appears to be a custom authentication scheme. Please confirm this is the expected format for the IAW service.Also applies to: 206-206
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
thirdweb-core/src/iaw/mod.rs (2)
88-89: Remove underscore prefix from actively used fields.The underscore prefix typically indicates unused fields, but these are actively used throughout the implementation.
pub struct IAWClient { - _base_url: String, - _http_client: reqwest::Client, + base_url: String, + http_client: reqwest::Client, }
149-149: Remove or sanitize header logging to prevent exposing sensitive data.Logging all headers including the Authorization header could expose sensitive authentication tokens.
- tracing::info!("Headers: {:?}", headers); + // Remove sensitive header logging
🧹 Nitpick comments (2)
thirdweb-core/src/iaw/userop.rs (1)
65-75: Consider extracting gas limit encoding to a helper function.The gas limit encoding logic is repeated and could be extracted for better maintainability.
+fn encode_gas_limits(verification_gas_limit: u64, call_gas_limit: u64) -> Result<B256, IAWError> { + let vgl_u128: u128 = verification_gas_limit.try_into() + .map_err(|_| IAWError::UnexpectedError("verification_gas_limit too large".to_string()))?; + let cgl_u128: u128 = call_gas_limit.try_into() + .map_err(|_| IAWError::UnexpectedError("call_gas_limit too large".to_string()))?; + + let mut bytes = [0u8; 32]; + bytes[0..16].copy_from_slice(&vgl_u128.to_be_bytes()); + bytes[16..32].copy_from_slice(&cgl_u128.to_be_bytes()); + Ok(B256::from(bytes)) +} - // Construct accountGasLimits - let vgl_u128: u128 = op.verification_gas_limit.try_into().map_err(|_| { - IAWError::UnexpectedError("verification_gas_limit too large".to_string()) - })?; - let cgl_u128: u128 = op.call_gas_limit.try_into().map_err(|_| { - IAWError::UnexpectedError("call_gas_limit too large".to_string()) - })?; - - let mut account_gas_limits_bytes = [0u8; 32]; - account_gas_limits_bytes[0..16].copy_from_slice(&vgl_u128.to_be_bytes()); - account_gas_limits_bytes[16..32].copy_from_slice(&cgl_u128.to_be_bytes()); - let account_gas_limits = B256::from(account_gas_limits_bytes); + let account_gas_limits = encode_gas_limits(op.verification_gas_limit, op.call_gas_limit)?;thirdweb-core/src/iaw/mod.rs (1)
444-447: Remove debug logging of signed response.This appears to be debug code that should not be in production.
- tracing::warn!( - signed_response = serde_json::to_string(&signed_response).unwrap(), - "Signed response" - );
📜 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 (15)
Cargo.toml(1 hunks)aa-core/Cargo.toml(1 hunks)aa-core/src/userop/builder.rs(1 hunks)core/Cargo.toml(1 hunks)core/src/rpc_clients/bundler.rs(1 hunks)core/src/userop.rs(5 hunks)executors/Cargo.toml(1 hunks)executors/src/external_bundler/confirm.rs(1 hunks)executors/src/external_bundler/send.rs(4 hunks)server/src/main.rs(2 hunks)thirdweb-core/Cargo.toml(2 hunks)thirdweb-core/src/iaw/mod.rs(1 hunks)thirdweb-core/src/iaw/userop.rs(1 hunks)types-core/Cargo.toml(1 hunks)types-core/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- aa-core/Cargo.toml
- Cargo.toml
- core/Cargo.toml
- core/src/rpc_clients/bundler.rs
- aa-core/src/userop/builder.rs
- executors/Cargo.toml
- executors/src/external_bundler/confirm.rs
- types-core/Cargo.toml
- types-core/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- thirdweb-core/Cargo.toml
- server/src/main.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-25T02:13:08.257Z
Learning: Wallet architecture should unify Wallet and Account interfaces, support in-app wallets (social/email login), smart wallets with account abstraction, and EIP-1193, EIP-5792, EIP-7702 standards.
🔇 Additional comments (2)
core/src/userop.rs (1)
29-72: LGTM!The refactored
userop_to_vault_inputfunction correctly handles both UserOp versions and safely manages optional fields.thirdweb-core/src/iaw/userop.rs (1)
10-45: LGTM!The UserOperation v0.6 hash computation correctly follows the EIP-4337 specification by excluding the signature from the hash calculation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
thirdweb-core/src/iaw/mod.rs (8)
107-108: Remove underscore prefix from actively used fields.The underscore prefix typically indicates unused fields, but
_base_urland_http_clientare actively used throughout the implementation.
185-186: Update field references after renaming.After removing the underscore prefix from fields, update these references accordingly.
243-244: Update field references in sign_typed_data method.Update
_base_urland_http_clientreferences to match the renamed fields.
302-303: Update field references in sign_transaction method.Update
_base_urland_http_clientreferences to match the renamed fields.
364-365: Update field references in sign_authorization method.Update
_base_urland_http_clientreferences to match the renamed fields.
444-445: Update field references in sign_userop method.Update
_base_urland_http_clientreferences to match the renamed fields.
439-439: Handle serialization error instead of using unwrap().The
unwrap()could cause a panic if serialization fails.
168-168: Remove header logging to prevent exposing sensitive authentication tokens.The current logging outputs all headers including the Authorization header which contains sensitive authentication tokens.
Remove this line:
- tracing::info!("Headers: {:?}", headers);
📜 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 (14)
Cargo.toml(1 hunks)aa-core/Cargo.toml(1 hunks)aa-core/src/userop/builder.rs(9 hunks)aa-types/Cargo.toml(1 hunks)aa-types/src/lib.rs(1 hunks)aa-types/src/userop.rs(1 hunks)core/Cargo.toml(1 hunks)core/src/error.rs(3 hunks)core/src/rpc_clients/bundler.rs(3 hunks)core/src/userop.rs(4 hunks)executors/Cargo.toml(1 hunks)executors/src/external_bundler/send.rs(6 hunks)thirdweb-core/Cargo.toml(1 hunks)thirdweb-core/src/iaw/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- aa-types/src/lib.rs
- aa-types/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- aa-core/Cargo.toml
- core/Cargo.toml
- Cargo.toml
- executors/Cargo.toml
- thirdweb-core/Cargo.toml
- core/src/error.rs
- core/src/rpc_clients/bundler.rs
- core/src/userop.rs
- executors/src/external_bundler/send.rs
- aa-core/src/userop/builder.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-25T02:13:08.257Z
Learning: Wallet architecture should unify Wallet and Account interfaces, support in-app wallets (social/email login), smart wallets with account abstraction, and EIP-1193, EIP-5792, EIP-7702 standards.
🔇 Additional comments (2)
aa-types/src/userop.rs (2)
8-14: LGTM!The
VersionedUserOpenum is well-structured with appropriate untagged serialization for automatic variant selection.
24-60: Correct implementation of UserOperation v0.6 hash computation.The function correctly implements the EIP-4337 specification for computing UserOperation v0.6 hashes with proper field ordering and double hashing.
joaquim-verges
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
thirdweb-core/src/iaw/mod.rs (1)
175-175: Remove or sanitize header logging to prevent exposing sensitive data.This logging statement still exposes sensitive authentication tokens in logs, as previously identified.
🧹 Nitpick comments (1)
thirdweb-core/src/iaw/mod.rs (1)
156-510: Consider refactoring to reduce code duplication.The signing methods share significant common patterns (header setup, error handling, response parsing). Consider extracting helper methods to improve maintainability and reduce duplication.
For example, create a helper method for common header setup:
+ fn build_headers(&self, auth_token: &AuthToken, thirdweb_auth: &ThirdwebAuth) -> Result<reqwest::header::HeaderMap, IAWError> { + let mut headers = thirdweb_auth.to_header_map()?; + headers.insert( + "Authorization", + reqwest::header::HeaderValue::from_str(&format!( + "Bearer embedded-wallet-token:{}", + auth_token + )) + .map_err(|_| IAWError::AuthError("Invalid auth token format".to_string()))?, + ); + headers.insert( + "Content-Type", + reqwest::header::HeaderValue::from_static("application/json"), + ); + Ok(headers) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/http/error.rs(1 hunks)thirdweb-core/src/iaw/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/http/error.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-25T02:13:08.257Z
Learning: Wallet architecture should unify Wallet and Account interfaces, support in-app wallets (social/email login), smart wallets with account abstraction, and EIP-1193, EIP-5792, EIP-7702 standards.
thirdweb-core/src/iaw/mod.rs (1)
Learnt from: iuwqyir
PR: thirdweb-dev/insight-broker-service#37
File: internal/consumer/consumer.go:34-40
Timestamp: 2025-05-22T20:45:25.169Z
Learning: In the insight-broker-service, error handling should propagate errors upward instead of panicking, allowing higher-level functions (like main) to implement appropriate retry logic or graceful shutdown. This is particularly important for Kafka client creation and connection errors.
🔇 Additional comments (5)
thirdweb-core/src/iaw/mod.rs (5)
1-19: LGTM! Clean imports and type definitions.The imports are well-organized and the AuthToken type alias provides clear semantic meaning for authentication tokens.
21-66: LGTM! Comprehensive error handling.The error enum covers all necessary error cases with proper categorization and the From trait implementations provide smooth error conversion.
68-116: LGTM! Well-structured data types.The data structures are clean and the field naming in IAWClient correctly uses non-underscore prefixes as addressed from previous feedback.
118-144: LGTM! Well-configured HTTP client setup.The constructors provide good flexibility with sensible production-ready HTTP client configurations including appropriate timeouts and keep-alive settings.
430-510: LGTM! Proper error handling and UserOp version support.The method correctly addresses the previous unwrap() issue on line 473 with proper error handling, and implements good pattern matching for different UserOp versions.
Summary by CodeRabbit
New Features
Improvements
Refactor