Kms implementation #284
Kms implementation #284
Conversation
- Added package to support AWS KMS signing. - Updated and to include new dependencies. - Implemented for KMS-backed signing operations. - Enhanced to support unified signing through . - Introduced new features for KMS integration in the crate. - Updated delivery and builder logic to utilize the new signer abstraction.
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughAdds a feature-gated AWS KMS account backend and unified AccountSigner; converts account factories to async futures; replaces raw private-key flows with signer objects across delivery/core layers and tests; bumps Rust toolchain and workspace deps; updates docs and config merging to support account overrides. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SolverBuilder
participant AccountFactory
participant AccountService
participant AccountInterface
participant AccountSigner
participant KmsWallet
participant AwsSigner
participant AWS_KMS
Client->>SolverBuilder: build(config)
SolverBuilder->>AccountFactory: invoke factory(config)
AccountFactory-->>SolverBuilder: await -> returns AccountInterface
SolverBuilder->>AccountService: create with AccountInterface
AccountService->>AccountInterface: signer()
AccountInterface-->>AccountService: AccountSigner (Local or Kms)
alt KMS selected
AccountService->>KmsWallet: KmsWallet::new(key_id, region, endpoint)
KmsWallet->>AwsSigner: construct AwsSigner (aws-config)
AwsSigner->>AWS_KMS: sign request
AWS_KMS-->>AwsSigner: signature
AwsSigner-->>KmsWallet: signature
KmsWallet-->>AccountService: AccountSigner::Kms
else Local selected
AccountService-->>AccountService: AccountSigner::Local (wrap PrivateKeySigner)
end
Client->>AccountSigner: sign_hash(hash)
alt KMS
AccountSigner->>AwsSigner: sign_hash -> AWS_KMS flow
else Local
AccountSigner->>PrivateKeySigner: sign_hash locally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 79-81: Update the AWS SDK crate versions in Cargo.toml to patched,
compatible releases: bump aws-config to at least 1.8.12 (or use aws-config = {
version = "1", features = ["behavior-version-latest"] }) and aws-sdk-kms to
1.98.0+ so the KMS client is not vulnerable; ensure both crates come from the
same AWS SDK Rust release set (verify pairing on the AWS SDK releases page) and
update the lockfile by running cargo update after changing the dependency
versions.
In `@crates/solver-account/src/implementations/kms.rs`:
- Around line 150-167: The code uses tokio::task::block_in_place when
tokio::runtime::Handle::try_current() returns Ok, which will panic if that
handle is a current-thread runtime; change the strategy to avoid block_in_place:
detect the runtime flavor returned by try_current() and if it is (or might be) a
current-thread runtime, use handle.spawn_blocking (or spawn a blocking task that
builds a temporary current-thread runtime and rt.block_on(KmsWallet::new(...)))
to run the async KmsWallet::new call safely; keep the existing fallback that
builds a new current-thread runtime for the Err case. Update the logic around
try_current(), tokio::task::block_in_place, and KmsWallet::new (or alternatively
make create_account async) so no code path calls block_in_place on a
current-thread runtime.
In `@README.md`:
- Around line 423-428: Update the "Performance Considerations" section: replace
the incorrect "~5,500 cryptographic requests/second" claim with accurate
per-Region, per-account quotas from AWS KMS — state that Sign (RSA/ECC) is
limited to 1,000 requests/second (shared across Sign, Verify, and related
operations) and GetPublicKey is 2,000 requests/second, and explicitly note these
limits are per AWS account per Region and vary by operation type; leave the
latency estimate unchanged.
🧹 Nitpick comments (4)
README.md (1)
403-422: Consider documenting tool dependencies for the AWS CLI address derivation.The wallet address derivation section is well-structured with both automated and manual approaches. The AWS CLI command pipeline is technically sound for deriving Ethereum addresses from KMS public keys.
However, the command relies on tools that may not be readily available (
openssl,keccak-256sum). Consider adding a brief note about these dependencies, though the "simpler approach" recommendation (running the solver once) already provides a practical alternative.📝 Optional: Add tool dependency note
You can also query the address directly using the AWS CLI: ```bash +# Note: Requires openssl and keccak-256sum tools # Get public key from KMS and derive Ethereum address aws kms get-public-key --key-id "your-key-id" --output text --query 'PublicKey' | \crates/solver-account/Cargo.toml (1)
19-19: Consider removing the duplicatetokiodependency.
tokioappears in both[dependencies](line 19) and[dev-dependencies](line 31). Iftokiois only needed for tests, it should remain solely in[dev-dependencies]. If it's required at runtime (e.g., for the KMS async operations), then the[dev-dependencies]entry is redundant and can be removed.Also applies to: 30-31
crates/solver-account/src/implementations/kms.rs (2)
13-16: Unused importSecretString.
SecretStringis imported but never used in this module.🧹 Proposed fix
use solver_types::{ - Address, ConfigSchema, Field, FieldType, Schema, SecretString, Signature, Transaction, + Address, ConfigSchema, Field, FieldType, Schema, Signature, Transaction, ValidationError, };
125-127: The defaultget_private_key()implementation already provides a descriptive panic message.The trait default implementation at
lib.rs:85-87panics with "get_private_key() not supported - use signer() instead", and the method is marked#[deprecated]in the public wrapper. No code paths unconditionally call this without type awareness—usage is confined toLocalWallettests.However, the design could be improved: returning a
Result<SecretString, AccountError>instead of panicking would provide better error handling than relying on panic even with a descriptive message. Alternatively, the method could remain deprecated as-is if panic-on-misuse is the intended design.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 22: CI uses an older rust toolchain than the crate MSRV: update the
prepare action reference dtolnay/rust-toolchain@1.86.0 in
.github/actions/prepare/action.yaml to dtolnay/rust-toolchain@1.88.0 so it
matches Cargo.toml's rust-version = "1.88.0" and rust-toolchain.toml; ensure the
action reference string is changed and commit the update so all toolchain
specifications align.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/prepare/action.yaml:
- Line 17: Update the stale inline comment next to the GitHub Action pin so it
reflects the actual toolchain version: change the comment after uses:
dtolnay/rust-toolchain@1.88.0 to mention v1.88.0 (replacing v1.86.0). Ensure the
comment text matches the pinned tag in the action declaration to avoid confusion
when using the dtolnay/rust-toolchain@1.88.0 reference.
- Changed functions in local and KMS implementations to return async futures. - Updated related tests to use async/await syntax for account creation. - Modified the type alias to reflect the new async signature. - Adjusted the and other components to accommodate the async account creation process.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-account/src/lib.rs`:
- Around line 203-288: Tests in AccountService (e.g., test_account_service_new,
test_account_service_get_address, test_account_service_sign,
test_account_service_get_private_key, test_account_service_signer) include a
hardcoded private key literal which triggers secret scanners; replace this by
centralizing a test-only key provider (e.g., a helper function like
test_helpers::test_private_key or implementations::local::test_key()) and have
tests obtain the key by assembling it from non-secret parts or reading from a
test-only env/config variable, then pass that config into
create_account(&config).update all tests to use that helper instead of the
inline literal or explicitly allowlist the single known test key in CI instead.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/solver-service/src/apis/quote/validation.rs (1)
412-425:⚠️ Potential issue | 🟠 MajorDon’t swallow invalid amounts when falling back to request values.
amount_as_u256()parse errors are discarded viaok().flatten(), turning invalid amounts into0instead of rejecting the request. This can silently produce incorrect quotes. Propagate the parse error instead.✅ Suggested fix (propagate parsing errors)
- let amount = cost_context - .swap_amounts - .get(asset_addr) - .map(|info| info.amount) - .unwrap_or_else(|| { - // Fallback to request amount if not in cost context - asset_info - .amount_as_u256() - .map_err(|e| QuoteError::InvalidRequest(format!("Invalid amount: {e}"))) - .ok() - .flatten() - .unwrap_or_default() - }); + let amount = if let Some(info) = cost_context.swap_amounts.get(asset_addr) { + info.amount + } else { + // Fallback to request amount if not in cost context + asset_info + .amount_as_u256() + .map_err(|e| QuoteError::InvalidRequest(format!("Invalid amount: {e}")))? + .unwrap_or_default() + };- let amount = cost_context - .swap_amounts - .get(asset_addr) - .map(|info| info.amount) - .unwrap_or_else(|| { - // Fallback to request amount if not in cost context - asset_info - .amount_as_u256() - .map_err(|e| QuoteError::InvalidRequest(format!("Invalid amount: {e}"))) - .ok() - .flatten() - .unwrap_or_default() - }); + let amount = if let Some(info) = cost_context.swap_amounts.get(asset_addr) { + info.amount + } else { + // Fallback to request amount if not in cost context + asset_info + .amount_as_u256() + .map_err(|e| QuoteError::InvalidRequest(format!("Invalid amount: {e}")))? + .unwrap_or_default() + };Also applies to: 475-488
crates/solver-demo/src/operations/env/deploy.rs (1)
228-234:⚠️ Potential issue | 🟡 MinorSilent failure on ABI encoding error.
unwrap_or_default()returns empty bytes if encoding fails, which would cause contract deployment to proceed with missing constructor arguments - leading to hard-to-debug failures or incorrect contract initialization.🛡️ Proposed fix to propagate encoding errors
fn encode_constructor_args(&self, constructor_args: Vec<DynSolValue>) -> Result<Bytes> { let tuple_value = DynSolValue::Tuple(constructor_args); - let encoded_args = tuple_value.abi_encode_sequence().unwrap_or_default(); - Bytes::from(encoded_args) + let encoded_args = tuple_value + .abi_encode_sequence() + .ok_or_else(|| Error::InvalidConfig("Failed to ABI encode constructor arguments".to_string()))?; + Ok(Bytes::from(encoded_args)) }Note: This would require updating all call sites to handle the
Result.crates/solver-core/src/handlers/transaction.rs (1)
600-606:⚠️ Potential issue | 🟠 MajorFix assertion to check for correct status transition:
OrderStatus::ExecutedThe panic message formatting is correct. However, line 605 has a significant bug: the assertion checks
OrderStatus::Executingbut should checkOrderStatus::Executed. The handler at line 198 explicitly transitions the status toExecutedviatransition_order_status(&order_id, OrderStatus::Executed), and the comment at line 603 confirms this intent ("updated to Executed"). The test only passes because the mock returns a hard-codedExecutingstatus that bypasses the actual state transition logic.Change line 605 to:
assert_eq!(updated_order.status, OrderStatus::Executed);crates/solver-settlement/src/implementations/direct.rs (1)
169-175:⚠️ Potential issue | 🟠 MajorUse a stable hash for deterministic oracle selection.
The code explicitly intends deterministic oracle selection (as stated in the comments), but
DefaultHasheris not guaranteed to be stable across Rust versions. Per official Rust documentation, its algorithm is unspecified and must not be relied upon across releases. This means different Rust toolchains could pick different oracles for the same order, breaking the stated determinism guarantee in a distributed system.The codebase already has
sha3as a dependency, so the fix is straightforward:🔧 Suggested change
- let order_id_bytes = order.id.as_bytes(); - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - std::hash::Hasher::write(&mut hasher, order_id_bytes); - let selection_context = std::hash::Hasher::finish(&hasher); + use sha3::{Digest, Keccak256}; + let mut hasher = Keccak256::new(); + hasher.update(order.id.as_bytes()); + let digest = hasher.finalize(); + let selection_context = u64::from_be_bytes(digest[0..8].try_into().unwrap());crates/solver-types/src/utils/conversion.rs (1)
145-170:⚠️ Potential issue | 🟠 MajorGuard against truncating non-address high bytes after padding.
With the new 41–63 length branch, values larger than 20 bytes are now accepted, padded to 32 bytes, and then silently truncated to the last 20 bytes. That can mis-parse inputs where the upper 12 bytes are non-zero (i.e., not a valid left‑padded address). Consider validating/normalizing the 32‑byte form before slicing.
Suggested fix
- 32 => { - // bytes32 representation - extract last 20 bytes as address - let mut addr_bytes = vec![0u8; 20]; - addr_bytes.copy_from_slice(&bytes[12..32]); - Ok(Address(addr_bytes)) - }, + 32 => { + // Normalize right-padded forms and ensure left-padding is zeroed + let bytes32: [u8; 32] = + bytes.try_into().map_err(|_| "Failed to convert to 32-byte array")?; + let normalized = normalize_bytes32_address(bytes32); + if normalized[..12].iter().any(|&b| b != 0) { + return Err("Invalid bytes32 address: non-zero left padding".into()); + } + let mut addr_bytes = vec![0u8; 20]; + addr_bytes.copy_from_slice(&normalized[12..32]); + Ok(Address(addr_bytes)) + },crates/solver-delivery/src/implementations/evm/alloy.rs (1)
576-612:⚠️ Potential issue | 🟠 MajorAvoid
block_in_placeon current-thread runtimes.
tokio::task::block_in_placepanics under a current-thread runtime, which is the default for#[tokio::test]. Moving this async initialization into a context where it's safe is necessary.Consider checking the runtime flavor and handling current-thread runtimes separately:
🔧 Suggested runtime-safe approach
- let delivery = tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - AlloyDelivery::new(network_ids, networks, network_signers, default_signer).await - }) - })?; + let delivery = match tokio::runtime::Handle::try_current() { + Ok(handle) => { + if handle.runtime_flavor() == tokio::runtime::RuntimeFlavor::MultiThread { + tokio::task::block_in_place(|| { + handle.block_on(async { + AlloyDelivery::new(network_ids, networks, network_signers, default_signer).await + }) + })? + } else { + // Current-thread runtime: spawn blocking task on a separate thread + std::thread::spawn(move || { + tokio::runtime::Runtime::new() + .and_then(|rt| { + Ok(rt.block_on(async { + AlloyDelivery::new(network_ids, networks, network_signers, default_signer).await + })) + }) + }) + .join() + .expect("delivery init thread panicked")? + } + } + Err(_) => { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .map_err(|e| DeliveryError::Network(format!("Failed to build runtime: {e}")))?; + rt.block_on(async { + AlloyDelivery::new(network_ids, networks, network_signers, default_signer).await + })? + } + };
🤖 Fix all issues with AI agents
In `@crates/solver-core/src/engine/mod.rs`:
- Around line 483-488: The expression EngineError::Service(...) is constructed
and immediately discarded; update the error handling in the block that also
calls tracing::error! for the failed order_id retrieval: either remove the dead
EngineError::Service(...) expression entirely if you only want to log the
failure, or use it (e.g., return Err(EngineError::Service(...)) or propagate it
to the caller) instead of continuing; ensure the chosen fix touches the Err(e)
arm that currently logs with tracing::error!, references order_id and e, and
removes the unused EngineError::Service construction.
In `@crates/solver-delivery/src/implementations/evm/alloy.rs`:
- Around line 645-650: The hardcoded private key literal in create_test_signer
(and the similar literal at the other location referenced) should be moved out
of inline code into a centralized, test-only source so secret scanners won't
flag it: create a test utilities module or const guarded by cfg(test) (e.g.,
TEST_PRIVATE_KEY) or read from an explicit test-only env var (TEST_PRIVATE_KEY)
with a test-only fallback, then have create_test_signer and the other usage
reference that symbol instead of embedding the hex string; ensure the
centralized symbol is only compiled for tests (cfg(test)) or documented as
allowlisted for CI.
In `@crates/solver-types/src/standards/eip7683.rs`:
- Around line 912-918: In from_batch_compact, parsing for token_bytes and
recipient_bytes uses unwrap_or([0u8;32]) while oracle_bytes and settler_bytes
propagate errors via map_err + ?; this is inconsistent with from_permit2 and
causes silent defaults for some invalid hex inputs—choose one behavior and apply
it to all four fields: either make token_bytes and recipient_bytes propagate
parse errors like oracle_bytes/settler_bytes by replacing unwrap_or with
map_err(...)? or make oracle_bytes/settler_bytes use unwrap_or([0u8;32]) to
match from_permit2; update parse_bytes32_from_hex usages in from_batch_compact
accordingly so all four fields (token_bytes, recipient_bytes, oracle_bytes,
settler_bytes) follow the same error-handling strategy.
🧹 Nitpick comments (6)
README.md (2)
407-409: Add language specifier to fenced code block.The log output example is missing a language specifier, which triggers a markdownlint warning (MD040).
📝 Suggested fix
-``` +```text INFO solver_core::builder: Solver address initialized component="account" address=0x1234...abcd +```
413-418: Complex shell pipeline for address derivation may be fragile.The multi-step pipeline to derive an Ethereum address from KMS public key uses several tools (
openssl,keccak-256sum) that may not be available on all systems. The suggestion to "run the solver once" (line 421) is more reliable.Consider simplifying this section to just recommend running the solver, or add a note about required dependencies (
openssl, a keccak-256 utility).crates/solver-service/src/apis/quote/generation.rs (1)
2861-2861: Debug print retained in test.The
println!statement is useful for debugging test failures but will produce output duringcargo test. Consider usingeprintln!or removing it if the assertion message on line 2863-2865 provides sufficient context for failure diagnosis.crates/solver-service/src/server.rs (1)
911-929: Test isolation differs from production hot-reload behavior.The test helper creates a new
Arc<RwLock<Config>>(line 914) rather than usingsolver.dynamic_config().clone()as the production code does (line 72). This means config updates to the solver won't propagate toAppState.configin tests.This is fine for the current tests which don't verify hot-reload behavior, but if future tests need to validate that admin API updates propagate to the solver, this would need adjustment.
♻️ Optional: Align with production behavior
async fn build_test_app_state(discovery_url: Option<String>) -> AppState { let solver = build_test_solver_engine().await; - let config = solver.config().clone(); - let dynamic_config = Arc::new(RwLock::new(config)); + let dynamic_config = solver.dynamic_config().clone(); let http_client = Client::builder()crates/solver-demo/src/operations/init/mod.rs (2)
604-641: Use the placeholder prefix constants for lookups to avoid drift.
Right now the lookup strings are hardcoded; if the prefix constants ever change, these lookups will break.♻️ Suggested change
- let oracle_addr = placeholders - .get(&format!("ORACLE_PLACEHOLDER_INPUT_{chain_id}")) + let oracle_addr = placeholders + .get(&format!("{ORACLE_PLACEHOLDER_INPUT_PREFIX}{chain_id}")) .ok_or_else(|| { - Error::InvalidConfig(format!("Missing ORACLE_PLACEHOLDER_INPUT_{chain_id}")) + Error::InvalidConfig(format!( + "Missing {ORACLE_PLACEHOLDER_INPUT_PREFIX}{chain_id}" + )) })?; @@ - let oracle_addr = placeholders - .get(&format!("ORACLE_PLACEHOLDER_OUTPUT_{chain_id}")) + let oracle_addr = placeholders + .get(&format!("{ORACLE_PLACEHOLDER_OUTPUT_PREFIX}{chain_id}")) .ok_or_else(|| { - Error::InvalidConfig(format!("Missing ORACLE_PLACEHOLDER_OUTPUT_{chain_id}")) + Error::InvalidConfig(format!( + "Missing {ORACLE_PLACEHOLDER_OUTPUT_PREFIX}{chain_id}" + )) })?;Also applies to: 656-657
676-749: Prefer prefix constants for placeholder key lookups here too.
Keeps generation and lookup in sync and prevents mismatches if constants change.♻️ Suggested change (pattern)
- let input_settler = placeholders - .get(&format!("PLACEHOLDER_INPUT_SETTLER_{chain_id}")) + let input_settler = placeholders + .get(&format!("{PLACEHOLDER_INPUT_SETTLER_PREFIX}{chain_id}")) .ok_or_else(|| { - Error::InvalidConfig(format!("Missing PLACEHOLDER_INPUT_SETTLER_{chain_id}")) + Error::InvalidConfig(format!( + "Missing {PLACEHOLDER_INPUT_SETTLER_PREFIX}{chain_id}" + )) })?; @@ - let token_a = placeholders - .get(&format!("PLACEHOLDER_TOKEN_A_{chain_id}")) + let token_a = placeholders + .get(&format!("{PLACEHOLDER_TOKEN_A_PREFIX}{chain_id}")) .ok_or_else(|| { - Error::InvalidConfig(format!("Missing PLACEHOLDER_TOKEN_A_{chain_id}")) + Error::InvalidConfig(format!( + "Missing {PLACEHOLDER_TOKEN_A_PREFIX}{chain_id}" + )) })?;Apply the same pattern for the other placeholder lookups in this function.
| fn create_test_signer() -> AccountSigner { | ||
| let private_key_signer: PrivateKeySigner = | ||
| "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" | ||
| .parse() | ||
| .unwrap(); | ||
| AccountSigner::Local(private_key_signer) |
There was a problem hiding this comment.
Hardcoded test private keys can trigger secret scanners.
Even test-only literals are frequently flagged (and can fail CI). Consider centralizing and splitting the literals (or allowlisting explicitly).
🛡️ Suggested test-key centralization/splitting
#[cfg(test)]
mod tests {
use super::*;
use alloy_signer_local::PrivateKeySigner;
use solver_types::utils::tests::builders::{NetworkConfigBuilder, NetworksConfigBuilder};
use std::collections::HashMap;
+
+ const TEST_DEFAULT_PRIVATE_KEY: &str = concat!(
+ "0xac0974bec39a17e3",
+ "6ba4a6b4d238ff94",
+ "4bacb478cbed5efc",
+ "ae784d7bf4f2ff80",
+ );
+ const TEST_NETWORK_PRIVATE_KEY: &str = concat!(
+ "0x59c6995e998f97a5",
+ "a0044966f0945389",
+ "dc9e86dae88c7a84",
+ "12f4603b6b78690d",
+ );
fn create_test_networks() -> NetworksConfig {
NetworksConfigBuilder::new()
.add_network(1, NetworkConfigBuilder::new().build())
.add_network(137, NetworkConfigBuilder::new().build())
.build()
}
fn create_test_signer() -> AccountSigner {
- let private_key_signer: PrivateKeySigner =
- "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"
- .parse()
- .unwrap();
+ let private_key_signer: PrivateKeySigner = TEST_DEFAULT_PRIVATE_KEY.parse().unwrap();
AccountSigner::Local(private_key_signer)
}
@@
async fn test_network_specific_signers() {
@@
- let network_signer_key: PrivateKeySigner =
- "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"
- .parse()
- .unwrap();
+ let network_signer_key: PrivateKeySigner = TEST_NETWORK_PRIVATE_KEY.parse().unwrap();
let network_signer = AccountSigner::Local(network_signer_key);Also applies to: 875-879
🤖 Prompt for AI Agents
In `@crates/solver-delivery/src/implementations/evm/alloy.rs` around lines 645 -
650, The hardcoded private key literal in create_test_signer (and the similar
literal at the other location referenced) should be moved out of inline code
into a centralized, test-only source so secret scanners won't flag it: create a
test utilities module or const guarded by cfg(test) (e.g., TEST_PRIVATE_KEY) or
read from an explicit test-only env var (TEST_PRIVATE_KEY) with a test-only
fallback, then have create_test_signer and the other usage reference that symbol
instead of embedding the hex string; ensure the centralized symbol is only
compiled for tests (cfg(test)) or documented as allowlisted for CI.
| let token_bytes = parse_bytes32_from_hex(token_str).unwrap_or([0u8; 32]); | ||
| let recipient_bytes = | ||
| parse_bytes32_from_hex(recipient_str).unwrap_or([0u8; 32]); | ||
| let oracle_bytes = parse_bytes32_from_hex(oracle_str).map_err(|e| { | ||
| format!("Invalid oracle address '{}': {}", oracle_str, e) | ||
| })?; | ||
| let settler_bytes = parse_bytes32_from_hex(settler_str).map_err(|e| { | ||
| format!("Invalid settler address '{}': {}", settler_str, e) | ||
| })?; | ||
| let oracle_bytes = parse_bytes32_from_hex(oracle_str) | ||
| .map_err(|e| format!("Invalid oracle address '{oracle_str}': {e}"))?; | ||
| let settler_bytes = parse_bytes32_from_hex(settler_str) | ||
| .map_err(|e| format!("Invalid settler address '{settler_str}': {e}"))?; |
There was a problem hiding this comment.
Inconsistent error handling for field parsing within from_batch_compact.
token_bytes and recipient_bytes (lines 912-914) use unwrap_or([0u8; 32]) for silent fallback, while oracle_bytes and settler_bytes (lines 915-918) now use ? for error propagation. This behavioral change means:
- Invalid token/recipient hex → silently defaults to zero bytes
- Invalid oracle/settler hex → returns an error
Additionally, this differs from from_permit2 (lines 534-538) which uses unwrap_or for all fields including oracle/settler.
Consider aligning the error handling strategy across all fields and order type handlers for consistency.
🤖 Prompt for AI Agents
In `@crates/solver-types/src/standards/eip7683.rs` around lines 912 - 918, In
from_batch_compact, parsing for token_bytes and recipient_bytes uses
unwrap_or([0u8;32]) while oracle_bytes and settler_bytes propagate errors via
map_err + ?; this is inconsistent with from_permit2 and causes silent defaults
for some invalid hex inputs—choose one behavior and apply it to all four fields:
either make token_bytes and recipient_bytes propagate parse errors like
oracle_bytes/settler_bytes by replacing unwrap_or with map_err(...)? or make
oracle_bytes/settler_bytes use unwrap_or([0u8;32]) to match from_permit2; update
parse_bytes32_from_hex usages in from_batch_compact accordingly so all four
fields (token_bytes, recipient_bytes, oracle_bytes, settler_bytes) follow the
same error-handling strategy.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-account/src/implementations/kms.rs`:
- Around line 102-108: The trait AccountInterface must document the KMS
asymmetry: add a clear doc comment on AccountInterface's sign_transaction method
explaining that some implementations (e.g., the KMS implementation in kms.rs)
may not perform transaction signing and will return an Implementation error
directing callers to use signer() (as EthereumWallet does), and clarify when
signer() should be used versus sign_transaction; also mention get_private_key()
is deprecated for KMS-backed accounts and callers should prefer signer() for
signing operations so implementations like LocalWallet and KMS are reconciled by
documentation.
🧹 Nitpick comments (2)
crates/solver-account/src/implementations/kms.rs (2)
124-126: Document the panic behavior forget_private_key()in the struct docs.The comment notes that
get_private_key()uses the default panicking implementation, which is appropriate since KMS keys cannot be exported. However, this could surprise callers who expect allAccountInterfaceimplementations to support this method.Consider adding this limitation to the
KmsWalletstruct-level documentation.📝 Suggested documentation addition
/// KMS-backed wallet implementation. /// /// Uses AWS KMS for all signing operations. The private key never leaves /// the KMS hardware security module. Public key is cached per-instance /// by the underlying `AwsSigner`. +/// +/// # Note +/// `get_private_key()` is not supported and will panic if called. +/// Use `signer()` to obtain an `AccountSigner` for signing operations. pub struct KmsWallet {
160-172: Consider adding integration tests for KMS wallet.The KMS implementation lacks test coverage. While testing against real KMS requires AWS credentials, you could add:
- Schema validation tests (similar to
LocalWalletSchematests)- Integration tests guarded by an environment variable or feature flag for CI with LocalStack
Do you want me to generate schema validation tests for
KmsWalletSchemathat can run without AWS credentials?
| async fn sign_transaction(&self, _tx: &Transaction) -> Result<Signature, AccountError> { | ||
| // Transaction signing is handled by EthereumWallet using signer() | ||
| // in the delivery layer, not through AccountInterface directly | ||
| Err(AccountError::Implementation( | ||
| "Use signer() with EthereumWallet for transaction signing".into(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check AccountInterface trait definition and sign_transaction documentation
rg -n -B2 -A10 "pub trait AccountInterface" crates/solver-account/src/lib.rs
# Check how LocalWallet implements sign_transaction
rg -n -B2 -A10 "async fn sign_transaction" crates/solver-account/src/implementations/
# Check all usages of sign_transaction in the codebase
rg -n "sign_transaction" --type rust -B1 -A2Repository: openintentsframework/oif-solver
Length of output: 7775
🏁 Script executed:
# Get full trait definition with documentation
rg -n -B10 -A15 "async fn sign_transaction.*Transaction.*Result" crates/solver-account/src/lib.rs
# Check if there's any note about optional implementations in AccountInterface
rg -n -B5 "sign_transaction" crates/solver-account/src/lib.rs | head -50Repository: openintentsframework/oif-solver
Length of output: 1856
🏁 Script executed:
# Search for architectural decisions or comments about KMS transaction signing
rg -n "delivery layer" crates/solver-account/src/ -B5 -A5
# Check if there are integration tests that show how sign_transaction should be used
rg -n "test.*sign_transaction" crates/solver-account/src/ -B2 -A10
# Search for any docs or comments about the signer() method as alternative
rg -n -B5 -A5 "signer\(\)" crates/solver-account/src/lib.rsRepository: openintentsframework/oif-solver
Length of output: 7695
Document or reconcile the sign_transaction asymmetry between implementations.
The AccountInterface trait requires sign_transaction as a non-optional method, but KMS deliberately returns an error directing callers to use signer() instead. LocalWallet fully implements signing, creating an inconsistency that breaks the trait contract. While the pattern (delegating to signer() for KMS) appears intentional—similar to how get_private_key() is deprecated for KMS—this design decision is not documented in the trait definition.
Either document in AccountInterface that implementations may handle transaction signing differently (with guidance on when to use sign_transaction vs signer()), or have KMS implement sign_transaction for consistency.
🤖 Prompt for AI Agents
In `@crates/solver-account/src/implementations/kms.rs` around lines 102 - 108, The
trait AccountInterface must document the KMS asymmetry: add a clear doc comment
on AccountInterface's sign_transaction method explaining that some
implementations (e.g., the KMS implementation in kms.rs) may not perform
transaction signing and will return an Implementation error directing callers to
use signer() (as EthereumWallet does), and clarify when signer() should be used
versus sign_transaction; also mention get_private_key() is deprecated for
KMS-backed accounts and callers should prefer signer() for signing operations so
implementations like LocalWallet and KMS are reconciled by documentation.
- Introduced and structures to manage account signing backends. - Updated functions to support account configuration overrides, allowing for KMS and other implementations. - Enhanced logging for account configurations during loading and seeding processes. - Added utility functions for converting between JSON and TOML formats for account configurations.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Style