feat(signer): Implement CDP signing for evm+solana #457
Conversation
WalkthroughAdds Coinbase CDP signer support across the codebase: new dependencies, OpenAPI/schema and docs updates, example projects for EVM/Solana, domain/storage/request/response model extensions, a CDP client service, EVM and Solana CDP signer implementations, and explicit rejection of CDP for Stellar. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Relayer API
participant Factory as SignerFactory
participant Signer as CdpSigner
participant CDP as Coinbase CDP
Client->>API: POST /sign-transaction
API->>Factory: instantiate signer from SignerConfig::Cdp
Factory->>Signer: create CdpSigner (with CdpService)
Signer->>CDP: request sign (message/tx)
rect rgba(220,240,255,0.9)
CDP-->>Signer: signed payload (hex/base64/base58)
Signer->>Signer: decode & assemble signature/tx
end
Signer-->>API: SignTransactionResponse / Signature
API-->>Client: 200 OK
sequenceDiagram
autonumber
actor Operator
participant API as Relayer API
participant Models as Request→Domain
participant Repo as Repository
Operator->>API: POST /signers { type: "cdp", config: {...} }
API->>Models: validate & map to SignerConfig::Cdp
Models->>Repo: persist SignerConfigStorage::Cdp
Repo-->>API: persist OK
API-->>Operator: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/modules/ROOT/pages/evm.adoc (1)
184-195: Fix relayer id and JSON typos in EVM curl examplesThe examples use relayer_id "solana-example" under EVM and contain JSON syntax issues (extra "{", trailing comma). Replace relayer id with an EVM one (e.g., "sepolia-example") and fix JSON.
Apply diffs per block:
-curl --location --request POST 'http://localhost:8080/api/v1/relayers/solana-example/transactions' \ +curl --location --request POST 'http://localhost:8080/api/v1/relayers/sepolia-example/transactions' \ ... ---data-raw '{ - { +--data-raw '{ "value": 1, "data": "0x", "to": "0xd9b55a2ba539031e3c18c9528b0dc3a7f603a93b", "speed": "average" }'-curl --location --request POST 'http://localhost:8080/api/v1/relayers/solana-example/transactions' \ +curl --location --request POST 'http://localhost:8080/api/v1/relayers/sepolia-example/transactions' \ ... ---data-raw '{ - { +--data-raw '{ "value": 1, "data": "0x", "to": "0xd9b55a2ba539031e3c18c9528b0dc3a7f603a93b", "speed": "average", "gas_limit": 21000 }'-curl --location --request POST 'http://localhost:8080/api/v1/relayers/solana-example/transactions' \ +curl --location --request POST 'http://localhost:8080/api/v1/relayers/sepolia-example/transactions' \ ... ---data-raw '{ - { +--data-raw '{ "value": 1, "data": "0x", "to": "0xd9b55a2ba539031e3c18c9528b0dc3a7f603a93b", "max_fee_per_gas": 30000000000, - "max_priority_fee_per_gas": 20000000000, + "max_priority_fee_per_gas": 20000000000 }'-curl --location --request POST 'http://localhost:8080/api/v1/relayers/solana-example/transactions' \ +curl --location --request POST 'http://localhost:8080/api/v1/relayers/sepolia-example/transactions' \ ... ---data-raw '{ - { +--data-raw '{ "value": 1, "data": "0x", "to": "0xd9b55a2ba539031e3c18c9528b0dc3a7f603a93b", "gas_price": "12312313123" }'-curl --location --request GET 'http://localhost:8080/api/v1/relayers/solana-example/transactions/<transaction_id>' \ +curl --location --request GET 'http://localhost:8080/api/v1/relayers/sepolia-example/transactions/<transaction_id>' \Also applies to: 201-213, 219-231, 237-248, 254-256
docs/modules/ROOT/pages/signers.adoc (1)
46-79: Fix table syntax: CDP row must be inside the table, and remove extra delimiters.Current placement renders an invalid table.
[cols="1,1,1,1"] |=== |Signer Type |EVM Networks |Solana Networks |Stellar Networks @@ |`aws_kms` |✅ Supported |❌ Not supported |❌ Not supported -|=== - -|`cdp` -|✅ Supported -|✅ Supported -|❌ Not supported -|=== +|`cdp` +|✅ Supported +|✅ Supported +|❌ Not supported |===Also applies to: 81-86
🧹 Nitpick comments (41)
docs/modules/ROOT/pages/evm.adoc (1)
296-300: Include CDP in Signer Security note for consistencyEarlier you recommend CDP as hosted; mirror that here.
- Use hosted signers (AWS KMS, Google Cloud KMS, Turnkey) in production + Use hosted signers (AWS KMS, Google Cloud KMS, Turnkey, CDP) in productiondocs/modules/ROOT/pages/configuration.adoc (1)
235-235: Reference CDP signers — OK (consider cross-link)Optionally link to the CDP section in signers.adoc for quick navigation.
- - CDP signers + - CDP signers (see xref:signers.adoc#cdp-signer-configuration[CDP Signer Configuration])README.md (1)
53-67: Add Solana CDP example row for parity with EVM.The repo includes examples/solana-cdp-signer/, but the table lists only the EVM CDP example.
| [`solana-turnkey-signer`](./examples/solana-turnkey-signer/) | Using Turnkey Signer for Solana secure signing | | [`solana-google-cloud-kms-signer`](./examples/solana-google-cloud-kms-signer/) | Using Google Cloud KMS Signer for Solana secure signing | | [`evm-cdp-signer`](./examples/evm-cdp-signer/) | Using CDP Signer for EVM secure signing | +| [`solana-cdp-signer`](./examples/solana-cdp-signer/) | Using CDP Signer for Solana secure signing | | [`network-configuration-config-file`](./examples/network-configuration-config-file/) | Using Custom network configuration via config file |examples/evm-cdp-signer/.env.example (1)
1-5: Order and annotate env keys; add safety note.Reorder to satisfy dotenv-linter and add a comment to avoid committing secrets.
+# Example env file — do NOT commit real secrets +API_KEY= +CDP_API_KEY_SECRET= +CDP_WALLET_SECRET= REDIS_URL=redis://redis:6379 -API_KEY= -WEBHOOK_SIGNING_KEY= -CDP_API_KEY_SECRET= -CDP_WALLET_SECRET= +WEBHOOK_SIGNING_KEY=examples/solana-cdp-signer/.env.example (1)
1-5: Same ordering and safety annotation as EVM example.+# Example env file — do NOT commit real secrets +API_KEY= +CDP_API_KEY_SECRET= +CDP_WALLET_SECRET= REDIS_URL=redis://redis:6379 -API_KEY= -WEBHOOK_SIGNING_KEY= -CDP_API_KEY_SECRET= -CDP_WALLET_SECRET= +WEBHOOK_SIGNING_KEY=examples/solana-cdp-signer/README.md (1)
125-142: Avoid manual step numbering to prevent drift.“Step 6” is skipped; prefer unnumbered headings.
-### Step 7: Run the Service +### Run the Service ... -### Step 8: Test the Service +### Test the Serviceexamples/evm-cdp-signer/README.md (1)
125-142: Avoid manual step numbering; remove “Step 7/8”.-### Step 7: Run the Service +### Run the Service ... -### Step 8: Test the Service +### Test the Serviceexamples/solana-cdp-signer/config/config.json (3)
4-10: Relayer missing notification_id; align with EVM example.If notifications are intended, add notification_id for consistency and to enable delivery.
"id": "solana-example", "name": "Solana Example", "network": "devnet", "paused": false, - "signer_id": "cdp-signer-solana", + "notification_id": "notification-example", + "signer_id": "cdp-signer-solana", "network_type": "solana"
16-21: Empty webhook URL.Provide a placeholder or remove the notification block to avoid misconfig.
- "url": "", + "url": "https://example.com/webhook",
28-33: Make api_key_id configurable via env (parity with secrets).Keeps secrets out of files and mirrors other fields.
- "api_key_id": "", + "api_key_id": { + "type": "env", + "value": "CDP_API_KEY_ID" + },examples/evm-cdp-signer/config/config.json (3)
20-25: Avoid live webhook endpoint in example.Use a neutral placeholder to prevent accidental data exfiltration during trials.
- "url": "https://webhook.site/db57ae0f-15d0-4d2c-be1c-09c450008128", + "url": "https://example.com/webhook",
32-36: Make api_key_id env-based for consistency.- "api_key_id": "", + "api_key_id": { + "type": "env", + "value": "CDP_API_KEY_ID" + },
41-42: Placeholder EVM address may fail validation.Ensure a 20-byte hex address (0x + 40 hex chars) is provided in examples, or clarify it’s a placeholder.
examples/evm-cdp-signer/docker-compose.yaml (2)
10-12: Secrets handling is inconsistent (defined but unused).You define cdp_api_key_secret/cdp_wallet_secret under secrets, but the service doesn’t consume them and values are passed via environment. Pick one approach to avoid confusion. Minimal change: remove unused secret definitions.
secrets: api_key: environment: API_KEY webhook_signing_key: environment: WEBHOOK_SIGNING_KEY - cdp_api_key_secret: - environment: CDP_API_KEY_SECRET - cdp_wallet_secret: - environment: CDP_WALLET_SECRETAlternatively, if you prefer Docker secrets, add them to the service’s secrets list and read from /run/secrets in the app, removing the env variables.
Also applies to: 18-19, 65-68
30-30: Quote restart policy for YAML clarity.- restart: on-failure:5 + restart: "on-failure:5"examples/solana-cdp-signer/docker-compose.yaml (1)
30-30: Quote restart policy for YAML clarity.- restart: on-failure:5 + restart: "on-failure:5"docs/modules/ROOT/pages/signers.adoc (2)
531-534: Permissions nit: use 0400 for files, 0500 for directories.Execute bit isn’t needed on files; recommend split guidance.
- - Set restrictive permissions on keystore files: `chmod 0500 config/keys/*` + - Set restrictive permissions on keystore files: `chmod 0400 config/keys/*` + - For key directories, use: `chmod 0500 config/keys`
519-526: Clarify field requirements.State that at least one of evm_account_address or solana_account_address must be provided depending on networks in use.
src/models/error/signer.rs (1)
75-216: Add a unit test for CdpError -> SignerError conversion.Keeps parity with other providers and guards display text.
@@ #[test] fn test_signer_error_from_turnkey_error() { @@ } + #[test] + fn test_signer_error_from_cdp_error() { + // Assuming a simple CdpError variant exists; adjust if the constructor differs. + let cdp_error = CdpError::ConfigError("bad config".to_string()); + let signer_error = SignerError::from(cdp_error); + match signer_error { + SignerError::CdpError(e) => { + assert!(e.to_string().contains("bad config")); + } + _ => panic!("Expected SignerError::CdpError"), + } + }src/services/signer/stellar/mod.rs (1)
137-137: Explicitly rejecting CDP for Stellar is correct. Add a negative test.Ensures API returns UnsupportedType for CDP on Stellar path and guards future regressions.
@@ impl StellarSignerFactory { pub fn create_stellar_signer( m: &SignerDomainModel, ) -> Result<StellarSigner, SignerFactoryError> { @@ - SignerConfig::Cdp(_) => return Err(SignerFactoryError::UnsupportedType("CDP".into())), + SignerConfig::Cdp(_) => return Err(SignerFactoryError::UnsupportedType("CDP".into())), @@ } } + +#[cfg(test)] +mod cdp_reject_tests { + use super::*; + use crate::models::signer::{CdpSignerConfig, SignerConfig}; + use crate::models::SecretString; + + #[test] + fn cdp_not_supported_for_stellar() { + let cfg = CdpSignerConfig { + api_key_id: "id".into(), + api_key_secret: SecretString::new("s"), + wallet_secret: SecretString::new("w"), + evm_account_address: None, + solana_account_address: None, + }; + let signer = SignerDomainModel::new("id".into(), SignerConfig::Cdp(cfg)); + let err = StellarSignerFactory::create_stellar_signer(&signer).unwrap_err(); + matches!(err, SignerFactoryError::UnsupportedType(_)); + } +}src/models/signer/response.rs (1)
53-59: Consider omitting null address fields in responses (cleaner API).Current tests assert on
null. If you prefer not emitting nulls, add serde hints.Cdp { api_key_id: String, - evm_account_address: Option<String>, - solana_account_address: Option<String>, + #[serde(skip_serializing_if = "Option::is_none")] + evm_account_address: Option<String>, + #[serde(skip_serializing_if = "Option::is_none")] + solana_account_address: Option<String>, // api_key_secret: SecretString, hidden from response due to security concerns // wallet_secret: SecretString, hidden from response due to security concerns },If you keep serializing nulls, ensure OpenAPI allows
["string","null"](see docs comment).src/services/signer/solana/mod.rs (4)
47-47: Remove unused importCdpService.
CdpServiceisn’t referenced in this module; keeping it will trigger an unused import warning.- services::{CdpService, GoogleCloudKmsService, TurnkeyService, VaultConfig, VaultService}, + services::{GoogleCloudKmsService, TurnkeyService, VaultConfig, VaultService},
185-190: Align factory error semantics with other signers.Other branches map construction failures to
InvalidConfig; CDP usesCreationFailed. Consider unifying for predictability. Also, avoid cloning the config ifCdpSigner::newcan borrow.- let cdp_signer = CdpSigner::new(config.clone()).map_err(|e| { - SignerFactoryError::CreationFailed(format!("CDP service error: {}", e)) - })?; + let cdp_signer = CdpSigner::new(config.clone()).map_err(|e| { + SignerFactoryError::InvalidConfig(format!("Failed to create CDP signer: {}", e)) + })?;If
CdpSigner::newcan accept a reference:// Outside the diff range; example only let cdp_signer = CdpSigner::new(config).map_err(|e| SignerFactoryError::InvalidConfig(e.to_string()))?;
224-227: Trim unused test imports.
AwsKmsSignerConfig,SignerRepoModel, andSolanaTransactionDataaren’t used in this module’s tests.- AwsKmsSignerConfig, CdpSignerConfig, GoogleCloudKmsSignerConfig, + CdpSignerConfig, GoogleCloudKmsSignerConfig, - GoogleCloudKmsSignerKeyConfig, GoogleCloudKmsSignerServiceAccountConfig, LocalSignerConfig, + GoogleCloudKmsSignerKeyConfig, GoogleCloudKmsSignerServiceAccountConfig, LocalSignerConfig, - SecretString, SignerConfig, SignerRepoModel, SolanaTransactionData, TurnkeySignerConfig, + SecretString, SignerConfig, TurnkeySignerConfig, VaultSignerConfig, VaultTransitSignerConfig,
468-492: CDP address/pubkey test LGTM; consider a “no address provided” case.Add a negative test where
solana_account_addressisNoneto verify error mapping or fallback behavior.I can add that test if you confirm desired behavior (error vs. lookup via CDP).
src/models/signer/mod.rs (2)
182-200: Add address format validation (optional) and clarify presence requirements.Currently, EVM/Solana addresses are unvalidated and optional. Consider:
- Validating EVM as 0x-prefixed 20-byte hex.
- Validating Solana as base58 with 32-byte decoded length.
- Clarifying whether at least one address must be provided.
pub struct CdpSignerConfig { #[validate(length(min = 1, message = "API Key ID cannot be empty"))] pub api_key_id: String, @@ pub wallet_secret: SecretString, - pub evm_account_address: Option<String>, - pub solana_account_address: Option<String>, + #[validate(custom(function = "validate_evm_address", message = "Invalid EVM address"))] + pub evm_account_address: Option<String>, + #[validate(custom(function = "validate_solana_address", message = "Invalid Solana address"))] + pub solana_account_address: Option<String>, }Helper validators (outside this hunk):
// Add near other validators fn validate_evm_address(addr: &Option<String>) -> Result<(), validator::ValidationError> { if let Some(a) = addr { if !regex::Regex::new(r"^(?i)0x[0-9a-f]{40}$").unwrap().is_match(a) { return Err(validator::ValidationError::new("invalid_evm_address")); } } Ok(()) } fn validate_solana_address(addr: &Option<String>) -> Result<(), validator::ValidationError> { if let Some(a) = addr { if bs58::decode(a).into_vec().map(|v| v.len() == 32).unwrap_or(false) { return Ok(()); } return Err(validator::ValidationError::new("invalid_solana_address")); } Ok(()) }If at-least-one is required, we can add a manual
impl Validate for CdpSignerConfigto enforce it.
437-438:SignerType::Cdp— add serialization test coverage.Extend
test_signer_type_serializationto assert"cdp"round-trip.assert_eq!( to_string(&SignerType::VaultTransit).unwrap(), "\"vault_transit\"" ); + assert_eq!(to_string(&SignerType::Cdp).unwrap(), "\"cdp\""); @@ assert_eq!( from_str::<SignerType>("\"aws_kms\"").unwrap(), SignerType::AwsKms ); + assert_eq!( + from_str::<SignerType>("\"cdp\"").unwrap(), + SignerType::Cdp + );I can add unit tests for
get_cdp()and CDP config validation as well.src/models/signer/repository.rs (2)
304-326: Domain↔storage conversions for CDP — OK; add round-trip tests.Recommend tests to ensure secrets and optional addresses round-trip correctly.
I can add a
test_cdp_config_storage_conversionsimilar to the local key test.
479-486: Fix docstring to match return type.It returns
Option, not an error.- /// Get CDP signer config, returns error if not a CDP signer + /// Get CDP signer config; returns None if not a CDP signersrc/services/signer/solana/cdp_signer.rs (3)
18-18: Remove unused imports
- log::{debug, info} are unused.
- transaction::Transaction is imported but the code uses fully-qualified paths instead.
- crate::config::network::solana is unused.
Clean these up to avoid warnings.
- use log::{debug, info}; - use solana_sdk::{pubkey::Pubkey, transaction::Transaction}; + use solana_sdk::pubkey::Pubkey; - use crate::{ - config::network::solana, - domain::SignTransactionResponse, - models::{Address, CdpSignerConfig, NetworkTransactionData, SignerError}, - services::{signer::Signer, CdpService, CdpServiceTrait}, - }; + use crate::{ + domain::SignTransactionResponse, + models::{Address, CdpSignerConfig, NetworkTransactionData, SignerError}, + services::{signer::Signer, CdpService, CdpServiceTrait}, + };Also applies to: 20-20, 24-24
110-116: Avoid duplicate address lookup inside sign()You fetch address_solana() here and also expose pubkey(); consider caching the pubkey/address within the signer (once) or fetching it once per sign call and passing it through to reduce an extra network hop in high-throughput paths.
231-274: Add tests for base64 response and signer-not-in-tx paths
- Add a test where CDP returns base64-encoded bytes to cover the new decode fallback.
- Add a negative test where the CDP pubkey isn’t among message.signer keys to assert the “pubkey not found” error.
I can provide the test scaffolding if helpful.
Also applies to: 315-353, 391-423
src/models/signer/request.rs (2)
112-124: Auto-zero secrets on dropDeriving Zeroize is good, but it requires manual zeroize() calls. Consider deriving ZeroizeOnDrop so api_key_secret and wallet_secret are wiped automatically.
-#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] +#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize, zeroize::ZeroizeOnDrop)] pub struct CdpSignerRequestConfig {
961-980: Expand negative tests for CDPAdd tests for empty wallet_secret and for both addresses being None (if disallowed by domain validation) to prevent regressions.
src/services/signer/evm/mod.rs (1)
339-357: Nice coverage for CDP creation and address path.The added tests validate factory wiring and address formatting. Consider a follow-up test that exercises EvmSigner::sign_transaction through the CDP variant (mocking the inner service) to catch regressions in delegation.
Also applies to: 417-439
src/services/cdp/mod.rs (4)
121-132: Fix incorrect doc comment for Solana helper.Method get_solana_account_address refers to EVM in the docstring.
Apply:
- /// Get the configured EVM account address + /// Get the configured Solana account address
276-284: Nit: rename local for clarity (signed tx, not signature).Variable signature_bytes holds the signed transaction bytes; rename to signed_tx_bytes for readability.
Apply:
- // Parse the signed transaction base64 string to bytes - let signature_bytes = general_purpose::STANDARD + // Parse the base64-encoded signed transaction to bytes + let signed_tx_bytes = general_purpose::STANDARD .decode(result.signed_transaction) .map_err(|e| { CdpError::SigningError(format!("Invalid Solana signed transaction base64: {}", e)) })?; - - Ok(signature_bytes) + + Ok(signed_tx_bytes)
134-154: Optional: accept EIP-55 checksum and normalize address casing.Currently hex-decoding accepts any case and length 20 bytes. Consider validating EIP-55 checksum (when mixed-case) to catch misconfigured addresses earlier.
231-236: Consider accepting non-UTF8 Solana messages.CDP “sign message” accepts a string, but upstream callers might provide arbitrary bytes. Optionally encode non-UTF8 as base64 or document the UTF-8 requirement at the trait boundary to prevent surprising SerializationError.
Reference: API body is a string field; clarify acceptable inputs. (docs.cdp.coinbase.com)
src/services/signer/evm/cdp_signer.rs (2)
181-189: Add EIP-712 typed data signing via CDP.CDP exposes SignEvmTypedData; consider implementing rather than returning NotImplemented.
Reference: Builder exists in the Rust SDK (SignEvmTypedData). (docs.rs)
86-94: Minor: avoid the extra get_evm_transaction_data() read.You only use evm_data to branch; you can branch directly from NetworkTransactionData (or reuse evm_data to build the Tx) to avoid redundant parsing.
Apply:
- let evm_data = transaction.get_evm_transaction_data()?; - - // Prepare data for signing based on transaction type - let (unsigned_tx_bytes, is_eip1559) = if evm_data.is_eip1559() { + // Prepare data for signing based on transaction type + let (unsigned_tx_bytes, is_eip1559) = if transaction.get_evm_transaction_data()?.is_eip1559() { let tx = TxEip1559::try_from(transaction)?; (tx.encoded_for_signing(), true) } else { let tx = TxLegacy::try_from(transaction)?; (tx.encoded_for_signing(), false) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)README.md(1 hunks)docs/modules/ROOT/pages/configuration.adoc(1 hunks)docs/modules/ROOT/pages/evm.adoc(1 hunks)docs/modules/ROOT/pages/index.adoc(1 hunks)docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/modules/ROOT/pages/solana.adoc(1 hunks)docs/openapi.json(3 hunks)examples/evm-cdp-signer/.env.example(1 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/evm-cdp-signer/docker-compose.yaml(1 hunks)examples/solana-cdp-signer/.env.example(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/docker-compose.yaml(1 hunks)src/models/error/signer.rs(2 hunks)src/models/signer/config.rs(5 hunks)src/models/signer/mod.rs(6 hunks)src/models/signer/repository.rs(7 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)src/services/signer/stellar/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/models/error/signer.rs (1)
src/models/signer/repository.rs (13)
from(59-63)from(67-71)from(190-195)from(200-205)from(209-214)from(218-223)from(227-236)from(240-249)from(253-263)from(267-277)from(281-289)from(293-301)from(305-313)
src/services/signer/solana/cdp_signer.rs (5)
src/services/signer/solana/mod.rs (6)
pubkey(100-100)pubkey(116-125)address(66-75)sign(111-111)sign(127-136)sign_transaction(77-89)src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (5)
new(49-54)new_with_service(59-61)new_for_testing(63-65)address(70-78)sign_transaction(80-146)src/models/signer/repository.rs (15)
from(59-63)from(67-71)from(190-195)from(200-205)from(209-214)from(218-223)from(227-236)from(240-249)from(253-263)from(267-277)from(281-289)from(293-301)from(305-313)from(317-325)from(329-334)
src/models/signer/response.rs (3)
src/models/signer/mod.rs (2)
new(442-444)from(491-501)src/services/cdp/mod.rs (1)
new(90-106)src/models/signer/repository.rs (15)
from(59-63)from(67-71)from(190-195)from(200-205)from(209-214)from(218-223)from(227-236)from(240-249)from(253-263)from(267-277)from(281-289)from(293-301)from(305-313)from(317-325)from(329-334)
src/models/signer/request.rs (5)
src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/models/signer/mod.rs (2)
new(442-444)signer_type(447-449)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(42-48)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-226)src/services/signer/solana/cdp_signer.rs (3)
new(42-48)address(150-158)test_address(186-206)
src/models/signer/config.rs (1)
src/models/signer/request.rs (2)
try_from(214-280)try_from(286-327)
src/models/signer/repository.rs (1)
src/models/signer/mod.rs (1)
get_cdp(359-364)
src/models/signer/mod.rs (1)
src/models/signer/repository.rs (1)
get_cdp(480-485)
src/services/signer/evm/cdp_signer.rs (3)
src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/solana/cdp_signer.rs (5)
new(42-48)new_with_service(53-55)new_for_testing(57-59)address(150-158)sign_transaction(160-173)src/services/signer/evm/mod.rs (4)
address(76-85)sign_transaction(87-99)sign_data(56-56)sign_data(104-113)
🪛 dotenv-linter (3.3.0)
examples/evm-cdp-signer/.env.example
[warning] 2-2: [UnorderedKey] The API_KEY key should go before the REDIS_URL key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The CDP_API_KEY_SECRET key should go before the REDIS_URL key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The CDP_WALLET_SECRET key should go before the REDIS_URL key
(UnorderedKey)
examples/solana-cdp-signer/.env.example
[warning] 2-2: [UnorderedKey] The API_KEY key should go before the REDIS_URL key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The CDP_API_KEY_SECRET key should go before the REDIS_URL key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The CDP_WALLET_SECRET key should go before the REDIS_URL key
(UnorderedKey)
🪛 LanguageTool
examples/evm-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new EVM EOA Wallet using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - EVM Account Address ### Step 5: Configu...
(QB_NEW_EN)
examples/solana-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new Solana Account using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - Solana Account Address ### Step 5: Conf...
(QB_NEW_EN)
🪛 Gitleaks (8.27.2)
examples/evm-cdp-signer/README.md
136-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
examples/solana-cdp-signer/README.md
136-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
🔇 Additional comments (37)
docs/modules/ROOT/pages/index.adoc (1)
234-235: Examples list updated — looks goodAdds CDP examples for EVM and Solana. No issues spotted.
docs/modules/ROOT/pages/evm.adoc (2)
40-41: Add CDP to supported signers — OKAccurate and consistent with broader PR.
46-47: Production signers note — OKCDP is correctly included in hosted signers recommendation.
Cargo.toml (1)
90-92: Dep versions and MSRV verified; confirm feature flags
reqwest-middleware 0.4.2 requires reqwest ^0.12.0 (your “0.12” spec is compatible) and cdp-sdk 0.1.0 declares MSRV 1.86. Please review your reqwest feature flags to ensure TLS, compression, and tracing support align with your client’s needs.docs/modules/ROOT/pages/solana.adoc (1)
67-68: Add CDP to Solana signers — OKReads well and matches EVM docs.
examples/solana-cdp-signer/config/config.json (1)
37-38: Validate solana_account_address requirement.If Solana signing is used, this cannot be empty; otherwise document that it’s optional.
src/models/error/signer.rs (1)
4-5: Good: plumbs CDP errors through SignerError.Variant and import look correct and consistent with other providers.
Also applies to: 32-34
src/services/mod.rs (1)
29-31: LGTM: CDP module re-export wired.Keeps error/type visibility consistent with other services.
src/models/signer/response.rs (2)
117-121: LGTM: CDP response mapping excludes secrets and forwards addresses.Matches storage/domain mapping.
308-387: Nice tests covering CDP roundtrip and secret omission.Covers both conversion and JSON shape.
src/services/signer/solana/mod.rs (7)
30-32: CDP module wiring looks good.Module inclusion and re-exports are consistent with existing signer modules.
60-60: New enum variantCdp(CdpSigner)is appropriate.Public API surface aligns with other signer variants.
66-74: Address dispatch includes CDP.Match arm forwards correctly to
signer.address().
81-89: Transaction signing dispatch includes CDP.Forwarding to
signer.sign_transaction(...)is consistent.
116-124: Pubkey dispatch includes CDP.Consistent with other variants.
127-135: Message signing dispatch includes CDP.Pattern and error propagation are correct.
345-366: CDP factory test LGTM.Covers construction and variant matching.
src/models/signer/mod.rs (4)
270-270:SignerConfig::Cdpvariant added — OK.
303-308: Validation mapping includes CDP — OK.Error text mirrors other variants.
358-365: Accessorget_cdp()— OK.Completes the accessor set.
382-383:get_signer_typeincludes CDP — OK.src/models/signer/repository.rs (4)
16-20: Imports include CDP domain types — OK.
44-46: Storage enum addsCdp— OK.
131-147:CdpSignerConfigStorageshape and secret (de)serializers — OK.Matches existing secret-handling patterns.
424-426: Enum conversions include CDP — OK.Also applies to: 440-442
src/services/signer/solana/cdp_signer.rs (2)
41-48: Constructor wiring and error mapping look correctCreating the CDP service and mapping errors to SignerError::Configuration is consistent with the EVM signer.
127-145: Signature extraction logic is soundIndexing by account_keys position and bounds-checking against signatures length correctly aligns with Solana’s required signer ordering.
src/models/signer/config.rs (3)
288-307: TryFrom maps secrets correctlySecrets are resolved via PlainOrEnvValue::get_value and forwarded to the domain type with appropriate error mapping.
156-161: Enum wiring for CDP is correctAdding Cdp to SignerFileConfigEnum and mapping later to SignerConfig is consistent with other signers.
49-57: Error: Unable to access code to verify presence of validation onevm_account_addressandsolana_account_address. Please manually confirm thatCdpSignerConfiginsrc/models/signer/mod.rsincludes custom validation functions forevm_account_addressandsolana_account_address; if not, enforce at least one address is set in domain validation.src/models/signer/request.rs (2)
261-267: Request→domain mapping for CDP looks correctSecrets are wrapped with SecretString and optional addresses are passed through.
150-153: Signer type discriminator includes CDPEnum extension and downstream validation in TryFrom are consistent.
src/services/signer/evm/mod.rs (2)
16-23: CDP wiring into EvmSigner looks correct.Module import, enum variant, and delegations for address/sign_data/sign_typed_data/sign_transaction are consistent with existing signers.
Also applies to: 69-72, 78-84, 92-98, 106-112, 120-126
173-178: Factory branch for CDP is consistent with other backends.Clean error mapping via SignerFactoryError::CreationFailed; constructor aligns with cdp_signer::new signature.
src/services/cdp/mod.rs (1)
169-181: Verify placeholder X-Wallet-Auth handling with middleware.Calls set .x_wallet_auth("") as a placeholder expecting WalletAuth middleware to inject the JWT. Please confirm this does not overwrite the header added by the middleware at send time. If needed, drop these setters and rely solely on the middleware.
References:
- X-Wallet-Auth header is required by the API. (docs.cdp.coinbase.com)
- The Rust SDK exposes x_wallet_auth on builders; ensure interplay with WalletAuth middleware. (docs.rs)
Also applies to: 207-215, 239-246, 264-271
src/services/signer/evm/cdp_signer.rs (2)
80-146: Signed transaction handling looks solid (EIP-1559 and legacy).Good use of encoded_for_signing(), CDP signing, and EIP-2718 decode; v normalization for 1559 is correct.
151-179: Signature length check and EVM response mapping are correct.65-byte validation and r/s/v extraction align with ECDSA expectations.
| // The CDP API returns a complete base64-encoded signed transaction | ||
| // We need to decode it back to a Transaction and extract the signature | ||
| let signed_tx_encoded = general_purpose::STANDARD.encode(signed_tx_bytes); | ||
|
|
||
| let signed_tx_data = crate::models::EncodedSerializedTransaction::new(signed_tx_encoded); | ||
| let signed_transaction: solana_sdk::transaction::Transaction = | ||
| signed_tx_data.try_into().map_err(|e| { | ||
| SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
CDP signed tx decoding: avoid re-encoding; accept both raw bytes and base64
Comment says CDP returns base64, but tests mock raw bytes. The current code always base64-encodes the response, which will double-encode if CDP ever returns base64. Make decoding robust by trying raw bytes first, then falling back to base64 string. Also update the misleading comment.
- // The CDP API returns a complete base64-encoded signed transaction
- // We need to decode it back to a Transaction and extract the signature
- let signed_tx_encoded = general_purpose::STANDARD.encode(signed_tx_bytes);
-
- let signed_tx_data = crate::models::EncodedSerializedTransaction::new(signed_tx_encoded);
- let signed_transaction: solana_sdk::transaction::Transaction =
- signed_tx_data.try_into().map_err(|e| {
- SignerError::SigningError(format!("Failed to decode signed transaction: {}", e))
- })?;
+ // CDP may return raw serialized transaction bytes; some implementations may return base64.
+ // Try raw bytes first; fall back to UTF-8 base64 string.
+ let signed_transaction: solana_sdk::transaction::Transaction =
+ if let Ok(tx) = bincode::deserialize(&signed_tx_bytes) {
+ tx
+ } else {
+ let s = std::str::from_utf8(&signed_tx_bytes).map_err(|_| {
+ SignerError::SigningError(
+ "CDP signed transaction is neither raw bytes nor UTF-8 base64".to_string(),
+ )
+ })?;
+ let signed_tx_data = crate::models::EncodedSerializedTransaction::new(s.to_string());
+ signed_tx_data.try_into().map_err(|e| {
+ SignerError::SigningError(format!("Failed to decode signed transaction: {}", 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.
| // The CDP API returns a complete base64-encoded signed transaction | |
| // We need to decode it back to a Transaction and extract the signature | |
| let signed_tx_encoded = general_purpose::STANDARD.encode(signed_tx_bytes); | |
| let signed_tx_data = crate::models::EncodedSerializedTransaction::new(signed_tx_encoded); | |
| let signed_transaction: solana_sdk::transaction::Transaction = | |
| signed_tx_data.try_into().map_err(|e| { | |
| SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) | |
| })?; | |
| // CDP may return raw serialized transaction bytes; some implementations may return base64. | |
| // Try raw bytes first; fall back to UTF-8 base64 string. | |
| let signed_transaction: solana_sdk::transaction::Transaction = | |
| if let Ok(tx) = bincode::deserialize(&signed_tx_bytes) { | |
| tx | |
| } else { | |
| let s = std::str::from_utf8(&signed_tx_bytes).map_err(|_| { | |
| SignerError::SigningError( | |
| "CDP signed transaction is neither raw bytes nor UTF-8 base64".to_string(), | |
| ) | |
| })?; | |
| let signed_tx_data = crate::models::EncodedSerializedTransaction::new(s.to_string()); | |
| signed_tx_data.try_into().map_err(|e| { | |
| SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) | |
| })? | |
| }; |
There was a problem hiding this comment.
The feedback is not valid for these reasons:
- Consistent behavior: The CDP SDK will always return decoded bytes, as you mentioned. There's no risk of double-encoding.
- Architectural consistency: The CDP signer uses EncodedSerializedTransaction which expects base64 strings, maintaining consistency with the rest of
the codebase. - Test accuracy: The tests correctly mock raw bytes because they're mocking the CDP service layer (which returns bytes), not the CDP API layer
(which returns base64).
The current implementation is correct. While there's a minor inefficiency of encoding-then-decoding, this maintains the abstraction boundaries and
uses the established EncodedSerializedTransaction type consistently.
There was a problem hiding this comment.
@sammccord Thank you for the clarification. You're absolutely right - my suggestion was based on an incomplete understanding of the CDP SDK's guaranteed behavior and the architectural decisions around EncodedSerializedTransaction.
Your points make perfect sense:
- If the CDP SDK always returns decoded bytes, there's no ambiguity to handle
- Using
EncodedSerializedTransactionmaintains consistency with the existing codebase patterns - The test mocking strategy at the service layer boundary is appropriate
I appreciate you taking the time to explain the reasoning behind the current implementation. The encoding/decoding inefficiency is indeed minor compared to maintaining clean abstraction boundaries.
✏️ Learnings added
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
sammccord
force-pushed
the
sammccord/implement-cdp-signing
branch
2 times, most recently
from
7904769 to
bf6b323
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
examples/solana-cdp-signer/README.md (1)
115-123: JSON snippet looks valid now.Trailing comma removed; JSON parses. Thanks for fixing.
examples/evm-cdp-signer/README.md (1)
115-123: JSON snippet is valid.Trailing comma issue addressed.
src/services/signer/solana/cdp_signer.rs (1)
100-108: Avoid base64 round-trip; deserialize the signed tx directly.cdp_service.sign_solana_transaction already returns raw serialized transaction bytes. Re-encoding to base64 and decoding back adds overhead and failure points.
Apply:
- // The CDP API returns a complete base64-encoded signed transaction - // We need to decode it back to a Transaction and extract the signature - let signed_tx_encoded = general_purpose::STANDARD.encode(signed_tx_bytes); - - let signed_tx_data = crate::models::EncodedSerializedTransaction::new(signed_tx_encoded); - let signed_transaction: solana_sdk::transaction::Transaction = - signed_tx_data.try_into().map_err(|e| { - SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) - })?; + // Decode the signed transaction from raw bytes returned by the CDP service. + let signed_transaction: solana_sdk::transaction::Transaction = + bincode::deserialize(&signed_tx_bytes).map_err(|e| { + SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) + })?;
🧹 Nitpick comments (24)
examples/evm-cdp-signer/.env.example (1)
1-5: Order env keys to satisfy dotenv-linter and improve readability.Alphabetize keys so the example passes dotenv-linter and groups secrets together.
-REDIS_URL=redis://redis:6379 -API_KEY= -WEBHOOK_SIGNING_KEY= -CDP_API_KEY_SECRET= -CDP_WALLET_SECRET= +API_KEY= +CDP_API_KEY_SECRET= +CDP_WALLET_SECRET= +REDIS_URL=redis://redis:6379 +WEBHOOK_SIGNING_KEY=examples/solana-cdp-signer/.env.example (1)
1-5: Align env key order with linter and EVM example.Same ordering tweak as the EVM example.
-REDIS_URL=redis://redis:6379 -API_KEY= -WEBHOOK_SIGNING_KEY= -CDP_API_KEY_SECRET= -CDP_WALLET_SECRET= +API_KEY= +CDP_API_KEY_SECRET= +CDP_WALLET_SECRET= +REDIS_URL=redis://redis:6379 +WEBHOOK_SIGNING_KEY=examples/solana-cdp-signer/README.md (2)
139-142: Use conventional header casing and avoid pasting real tokens.Switch to “Authorization” and reference $API_KEY to reduce accidental leaks.
-H "Content-Type: application/json" \ - -H "AUTHORIZATION: Bearer YOUR_API_KEY" + -H "Authorization: Bearer $API_KEY"
125-133: Fix step numbering (skips 6).Renumber to keep sequence consistent.
-### Step 7: Run the Service +### Step 6: Run the Service @@ -### Step 8: Test the Service +### Step 7: Test the Serviceexamples/evm-cdp-signer/docker-compose.yaml (3)
35-36: Do not publish Redis to the host in the example.Redis is only used by the relayer; keep it on the internal network to reduce exposure.
- ports: - - 6379:6379/tcp
30-31: Add Redis healthcheck and depend on readiness, not mere start.Prevents relayer boot races against Redis availability.
@@ - depends_on: - - redis + depends_on: + redis: + condition: service_healthy @@ image: redis:bookworm + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 5s + timeout: 3s + retries: 10Also applies to: 34-51
60-61: Remove unused volume.
vault-dataisn’t referenced by any service.- vault-data: - driver: localexamples/evm-cdp-signer/README.md (2)
139-142: Prefer “Authorization” header casing and env var.Avoids accidental key paste; aligns with common examples.
-H "Content-Type: application/json" \ - -H "AUTHORIZATION: Bearer YOUR_API_KEY" + -H "Authorization: Bearer $API_KEY"
125-133: Fix step numbering (skips 6).Keep sequential numbering.
-### Step 7: Run the Service +### Step 6: Run the Service @@ -### Step 8: Test the Service +### Step 7: Test the Serviceexamples/solana-cdp-signer/docker-compose.yaml (3)
9-13: Avoid duplicating CDP secrets as both env vars and Docker secrets.Pick one source of truth. Prefer Docker secrets to reduce in-process env exposure.
Option A — keep secrets, drop env vars:
environment: REDIS_URL: ${REDIS_URL} RATE_LIMIT_REQUESTS_PER_SECOND: 10 RATE_LIMIT_BURST: 50 WEBHOOK_SIGNING_KEY: ${WEBHOOK_SIGNING_KEY} API_KEY: ${API_KEY} - CDP_API_KEY_SECRET: ${CDP_API_KEY_SECRET} - CDP_WALLET_SECRET: ${CDP_WALLET_SECRET}Option B — keep env vars, drop secrets (service-level and top-level):
relayer: - secrets: - - api_key - - webhook_signing_key - - cdp_api_key_secret - - cdp_wallet_secret + # secrets removed; using env vars only ... -secrets: - api_key: - environment: API_KEY - webhook_signing_key: - environment: WEBHOOK_SIGNING_KEY - cdp_api_key_secret: - environment: CDP_API_KEY_SECRET - cdp_wallet_secret: - environment: CDP_WALLET_SECRETAlso applies to: 14-21
60-61: Remove unused volume ‘vault-data’.It’s defined but not referenced by any service.
volumes: redis_data: driver: local - vault-data: - driver: local
30-33: Add Redis healthcheck and wait-for-healthy dependency.Prevents relayer from starting before Redis is ready.
- depends_on: - - redis + depends_on: + redis: + condition: service_healthyredis: image: redis:bookworm ports: - 6379:6379/tcp security_opt: - no-new-privileges volumes: - redis_data:/data + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 5s + retries: 5 command: - redis-server - --appendonly - 'yes' - --save - '60' - '1'Also applies to: 33-51
src/models/signer/request.rs (2)
261-268: Validate “exactly one” address at the API boundary (EVM or Solana).Fast-fail with a clear 400 before domain conversion. If domain already enforces this, keeping both helps UX with precise error messages.
- SignerConfigRequest::Cdp(cdp_config) => SignerConfig::Cdp(CdpSignerConfig { + SignerConfigRequest::Cdp(cdp_config) => { + if cdp_config.evm_account_address.is_none() + && cdp_config.solana_account_address.is_none() + { + return Err(ApiError::BadRequest( + "At least one of evm_account_address or solana_account_address must be provided".into() + )); + } + if cdp_config.evm_account_address.is_some() + && cdp_config.solana_account_address.is_some() + { + return Err(ApiError::BadRequest( + "Provide only one of evm_account_address or solana_account_address".into() + )); + } + SignerConfig::Cdp(CdpSignerConfig { api_key_id: cdp_config.api_key_id, api_key_secret: SecretString::new(&cdp_config.api_key_secret), wallet_secret: SecretString::new(&cdp_config.wallet_secret), evm_account_address: cdp_config.evm_account_address, solana_account_address: cdp_config.solana_account_address, - }), + }) + }
888-980: Extend CDP tests: solana-only, both missing, both set.Covers the XOR and error messaging paths.
Example additions (place near existing CDP tests):
#[test] fn test_valid_cdp_create_request_solana_only() { let request = SignerCreateRequest { id: Some("test-cdp-sol".to_string()), signer_type: SignerTypeRequest::Cdp, config: SignerConfigRequest::Cdp(CdpSignerRequestConfig { api_key_id: "id".into(), api_key_secret: "sec".into(), wallet_secret: "wsec".into(), evm_account_address: None, solana_account_address: Some("So1ana1111111111111111111111111111111111111".into()), }), }; let result = Signer::try_from(request); assert!(result.is_ok()); } #[test] fn test_invalid_cdp_missing_both_addresses() { let request = SignerCreateRequest { id: Some("test-cdp-missing".to_string()), signer_type: SignerTypeRequest::Cdp, config: SignerConfigRequest::Cdp(CdpSignerRequestConfig { api_key_id: "id".into(), api_key_secret: "sec".into(), wallet_secret: "wsec".into(), evm_account_address: None, solana_account_address: None, }), }; let result = Signer::try_from(request); assert!(result.is_err()); } #[test] fn test_invalid_cdp_both_addresses_set() { let request = SignerCreateRequest { id: Some("test-cdp-both".to_string()), signer_type: SignerTypeRequest::Cdp, config: SignerConfigRequest::Cdp(CdpSignerRequestConfig { api_key_id: "id".into(), api_key_secret: "sec".into(), wallet_secret: "wsec".into(), evm_account_address: Some("0x0000000000000000000000000000000000000000".into()), solana_account_address: Some("So1ana1111111111111111111111111111111111111".into()), }), }; let result = Signer::try_from(request); assert!(result.is_err()); }src/models/signer/config.rs (1)
49-57: Add a focused unit test for CDP file-config conversion.Covers secret extraction and address passthrough.
Example:
#[test] fn test_cdp_file_config_conversion() { use crate::models::SecretString; let cfg = CdpSignerFileConfig { api_key_id: "id".into(), api_key_secret: PlainOrEnvValue::Plain { value: SecretString::new("asecret") }, wallet_secret: PlainOrEnvValue::Plain { value: SecretString::new("wsecret") }, evm_account_address: Some("0x0000000000000000000000000000000000000000".into()), solana_account_address: None, }; let res = CdpSignerConfig::try_from(cfg); assert!(res.is_ok()); let c = res.unwrap(); assert_eq!(c.api_key_id, "id"); assert_eq!(c.evm_account_address, Some("0x0000000000000000000000000000000000000000".into())); assert_eq!(c.solana_account_address, None); }Also applies to: 288-308
docs/modules/ROOT/pages/signers.adoc (1)
498-525: Clarify field requirement semantics.Add “At least one of … is required” to the descriptions of evm_account_address and solana_account_address to match service behavior.
Apply:
-| The address of the CDP EVM EOA that will be used for EVM signing operations. Optional if only configuring for Solana signing. +| The address of the CDP EVM EOA used for EVM signing operations. Optional if only configuring for Solana signing. At least one of the EVM or Solana address must be set. @@ -| The address of the CDP Solana Account that will be used for Solana signing operations. Optional if only configuring for EVM signing. +| The address of the CDP Solana account used for Solana signing operations. Optional if only configuring for EVM signing. At least one of the EVM or Solana address must be set.src/services/signer/solana/mod.rs (2)
185-190: Factory arm for CDP — map errors precisely, consider consistent return style.Creation path and error mapping are fine. For readability, consider returning a value from all match arms and using the single Ok(signer) at the end (avoid early returns in some arms).
262-277: Duplicate local signer test — remove to reduce noise.test_create_solana_signer_test duplicates test_create_solana_signer_local.
Apply:
- #[test] - fn test_create_solana_signer_test() { - let signer_model = SignerDomainModel { - id: "test".to_string(), - config: SignerConfig::Local(LocalSignerConfig { - raw_key: test_key_bytes(), - }), - }; - - let signer = SolanaSignerFactory::create_solana_signer(&signer_model).unwrap(); - - match signer { - SolanaSigner::Local(_) => {} - _ => panic!("Expected Local signer"), - } - }src/services/signer/solana/cdp_signer.rs (2)
17-19: Remove unused imports after the refactor.base64 and log are no longer used.
Apply:
-use base64::{engine::general_purpose, Engine as _}; -use log::{debug, info}; +// (no-op)
23-29: Drop unused solana config import.Not used in this module.
Apply:
-use crate::{ - config::network::solana, - domain::SignTransactionResponse, - models::{Address, CdpSignerConfig, NetworkTransactionData, SignerError}, - services::{signer::Signer, CdpService, CdpServiceTrait}, -}; +use crate::{ + domain::SignTransactionResponse, + models::{Address, CdpSignerConfig, NetworkTransactionData, SignerError}, + services::{signer::Signer, CdpService, CdpServiceTrait}, +};src/services/cdp/mod.rs (3)
121-133: Fix doc comment for Solana accessor.Comment says “EVM” but function returns the Solana address.
Apply:
- /// Get the configured EVM account address + /// Get the configured Solana account address fn get_solana_account_address(&self) -> Result<String, CdpError> {
269-274: Correct error message: transaction vs message.This path signs a Solana transaction.
Apply:
- .map_err(|e| CdpError::SigningError(format!("Failed to sign Solana message: {}", e)))?; + .map_err(|e| CdpError::SigningError(format!("Failed to sign Solana transaction: {}", e)))?;
101-106: Add a sane HTTP client timeout.Avoid hanging requests to the CDP API.
Apply:
- let wallet_client = ClientBuilder::new(reqwest::Client::new()) + let inner = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .map_err(|e| CdpError::ConfigError(format!("Failed to build HTTP client: {}", e)))?; + let wallet_client = ClientBuilder::new(inner) .with(wallet_auth) .build();docs/openapi.json (1)
6412-6428: CDP response schema — add “at least one address” and examples.Document that at least one of the optional addresses must be present, and add an example to improve client UX.
Apply:
"properties": { "api_key_id": { "type": "string" }, "evm_account_address": { "type": ["string", "null"] }, "solana_account_address": { "type": ["string", "null"] } }, + "description": "Non-secret CDP signer details. At least one of evm_account_address or solana_account_address must be configured.", "additionalProperties": false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)README.md(1 hunks)docs/modules/ROOT/pages/configuration.adoc(1 hunks)docs/modules/ROOT/pages/evm.adoc(2 hunks)docs/modules/ROOT/pages/index.adoc(1 hunks)docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/modules/ROOT/pages/solana.adoc(1 hunks)docs/openapi.json(5 hunks)examples/evm-cdp-signer/.env.example(1 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/evm-cdp-signer/docker-compose.yaml(1 hunks)examples/solana-cdp-signer/.env.example(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/docker-compose.yaml(1 hunks)src/models/error/signer.rs(2 hunks)src/models/signer/config.rs(5 hunks)src/models/signer/mod.rs(6 hunks)src/models/signer/repository.rs(7 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)src/services/signer/stellar/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/modules/ROOT/pages/configuration.adoc
- docs/modules/ROOT/pages/index.adoc
- examples/evm-cdp-signer/config/config.json
🚧 Files skipped from review as they are similar to previous changes (12)
- examples/solana-cdp-signer/config/config.json
- docs/modules/ROOT/pages/solana.adoc
- src/models/error/signer.rs
- docs/modules/ROOT/pages/evm.adoc
- src/models/signer/repository.rs
- README.md
- src/models/signer/mod.rs
- src/services/mod.rs
- Cargo.toml
- src/services/signer/evm/mod.rs
- src/services/signer/evm/cdp_signer.rs
- src/models/signer/response.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/services/signer/solana/cdp_signer.rs (4)
src/services/signer/solana/mod.rs (6)
pubkey(100-100)pubkey(116-125)address(66-75)sign(111-111)sign(127-136)sign_transaction(77-89)src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (5)
new(49-54)new_with_service(59-61)new_for_testing(63-65)address(70-78)sign_transaction(80-146)
src/models/signer/config.rs (1)
src/models/signer/request.rs (2)
try_from(214-280)try_from(286-327)
src/models/signer/request.rs (5)
src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/models/signer/mod.rs (2)
new(442-444)signer_type(447-449)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(42-48)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-226)src/services/signer/solana/cdp_signer.rs (3)
new(42-48)address(150-158)test_address(186-206)
🪛 LanguageTool
examples/evm-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new EVM EOA Wallet using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - EVM Account Address ### Step 5: Configu...
(QB_NEW_EN)
examples/solana-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new Solana Account using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - Solana Account Address ### Step 5: Conf...
(QB_NEW_EN)
🪛 dotenv-linter (3.3.0)
examples/evm-cdp-signer/.env.example
[warning] 2-2: [UnorderedKey] The API_KEY key should go before the REDIS_URL key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The CDP_API_KEY_SECRET key should go before the REDIS_URL key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The CDP_WALLET_SECRET key should go before the REDIS_URL key
(UnorderedKey)
examples/solana-cdp-signer/.env.example
[warning] 2-2: [UnorderedKey] The API_KEY key should go before the REDIS_URL key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The CDP_API_KEY_SECRET key should go before the REDIS_URL key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The CDP_WALLET_SECRET key should go before the REDIS_URL key
(UnorderedKey)
🪛 Gitleaks (8.27.2)
examples/solana-cdp-signer/README.md
136-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (21)
src/services/signer/stellar/mod.rs (1)
137-137: LGTM: explicit CDP rejection for StellarThis guard cleanly handles the new
SignerConfig::Cdpvariant and prevents accidental routing to unsupported functionality.Optional: add a unit test asserting
create_stellar_signerreturnsErr(SignerFactoryError::UnsupportedType("CDP".into()))when given a Stellar signer with CDP config, to lock this behavior in.examples/evm-cdp-signer/README.md (1)
93-99: No action needed—helper exists
Thegenerate_uuidexample is declared in Cargo.toml (path “helpers/generate_uuid.rs”) and implemented in the repo, socargo run --example generate_uuidin the README works as-is.src/models/signer/request.rs (2)
303-307: Type/config match extended for CDP — looks correct.
112-124: evm_account_address & solana_account_address are optional (not nullable) in OpenAPI
In the generated spec both fields appear as type "string" with validation patterns and are not listed in the required array, confirming #[schema(nullable = false)] yields “optional but not null.”src/models/signer/config.rs (3)
288-308: CDP file-config → domain conversion looks consistent with other signers.
148-162: Enum wiring for CDP in SignerFileConfigEnum is correct.
49-57: Ignore removal of Debug on CdpSignerFileConfig. PlainOrEnvValue’s Plain variant wraps SecretString, whose Debug impl always writes “SecretString(REDACTED)”, and the Env variant only prints the environment‐variable name (not its secret value). No sensitive data will be exposed by deriving Debug on CdpSignerFileConfig.Likely an incorrect or invalid review comment.
docs/modules/ROOT/pages/signers.adoc (2)
40-40: CDP added to supported signer types — looks good.Accurately reflects the new capability.
80-84: Network matrix row for CDP is correct.EVM and Solana supported; Stellar not supported.
src/services/signer/solana/mod.rs (4)
30-32: Module wiring for CDP signer — good integration.Imports and re-exports are consistent with other signer modules.
60-62: Enum variant added for CDP — LGTM.Keeps parity with other signers.
72-74: Forwarding to CdpSigner methods — LGTM.Address, transaction signing, pubkey, and message signing dispatches look correct.
Also applies to: 86-88, 122-124, 133-135
345-366: CDP tests — nice coverage.Covers creation and address/pubkey retrieval for CDP.
Also applies to: 468-491
src/services/signer/solana/cdp_signer.rs (3)
41-48: Constructor maps CDP service errors cleanly — LGTM.
127-142: Signature extraction logic — LGTM.Correctly locates signer index and returns the corresponding signature.
Also applies to: 144-145
160-173: sign_transaction passthrough — LGTM.Returns raw signed transaction bytes as expected by upstream.
src/services/cdp/mod.rs (1)
231-257: Ignore suggestion to allow non-UTF8 inputs
The Coinbase CDP Solana “sign a message” endpoint requires themessagefield as a UTF-8 string (e.g.{"message": "Hello, world!"}), so validating withstr::from_utf8is correct.Likely an incorrect or invalid review comment.
docs/openapi.json (4)
6289-6292: Expose CDP in SignerConfigRequest — LGTM.oneOf now includes CDPSignerRequestConfig.
6489-6491: SignerType enum updated with "cdp" — LGTM.
6502-6504: SignerTypeRequest enum updated with "cdp" — LGTM.
7271-7306: CDPSignerRequestConfig — solid; secrets marked writeOnly.Patterns and constraints look good. No issues.
| /// CDP signer configuration for API requests | ||
| #[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct CdpSignerRequestConfig { | ||
| pub api_key_id: String, | ||
| pub api_key_secret: String, | ||
| pub wallet_secret: String, | ||
| #[schema(nullable = false)] | ||
| pub evm_account_address: Option<String>, | ||
| #[schema(nullable = false)] | ||
| pub solana_account_address: Option<String>, | ||
| } | ||
|
|
There was a problem hiding this comment.
Redact and zeroize CDP secrets in API models.
Don’t derive Debug for secret-bearing structs and ensure automatic zeroization on drop.
-/// CDP signer configuration for API requests
-#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)]
+/// CDP signer configuration for API requests
+#[derive(Serialize, Deserialize, ToSchema, Zeroize)]
+#[zeroize(drop)]
pub struct CdpSignerRequestConfig {
pub api_key_id: String,
pub api_key_secret: String,
pub wallet_secret: String,
#[schema(nullable = false)]
pub evm_account_address: Option<String>,
#[schema(nullable = false)]
pub solana_account_address: Option<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.
| /// CDP signer configuration for API requests | |
| #[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct CdpSignerRequestConfig { | |
| pub api_key_id: String, | |
| pub api_key_secret: String, | |
| pub wallet_secret: String, | |
| #[schema(nullable = false)] | |
| pub evm_account_address: Option<String>, | |
| #[schema(nullable = false)] | |
| pub solana_account_address: Option<String>, | |
| } | |
| /// CDP signer configuration for API requests | |
| #[derive(Serialize, Deserialize, ToSchema, Zeroize)] | |
| #[zeroize(drop)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct CdpSignerRequestConfig { | |
| pub api_key_id: String, | |
| pub api_key_secret: String, | |
| pub wallet_secret: String, | |
| #[schema(nullable = false)] | |
| pub evm_account_address: Option<String>, | |
| #[schema(nullable = false)] | |
| pub solana_account_address: Option<String>, | |
| } |
🤖 Prompt for AI Agents
In src/models/signer/request.rs around lines 112-124, the CdpSignerRequestConfig
currently derives Debug and Zeroize but needs secrets redacted and automatically
zeroized on drop; remove Debug from the derive list, add automatic drop
zeroization (either add derive(ZeroizeOnDrop) or annotate the struct with
#[zeroize(drop)]), and ensure sensitive fields (api_key_secret and
wallet_secret) are zeroized (e.g., mark them with #[zeroize] or rely on
struct-level drop zeroize) while leaving non-secret fields unchanged; after this
change do not reintroduce a Debug impl that would print secret values.
bf6b323 to
168f9d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/models/signer/request.rs (1)
112-124: Remove Debug and enable zeroize-on-drop for CDP request (secrets).Deriving Debug on a struct carrying raw secrets risks leaking them via logs. Also, Zeroize without drop-hook won’t clear memory automatically.
-/// CDP signer configuration for API requests -#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] +/// CDP signer configuration for API requests +#[derive(Serialize, Deserialize, ToSchema, Zeroize)] +#[zeroize(drop)] pub struct CdpSignerRequestConfig {Consider applying the same pattern to other secret-bearing request types (Local key, Vault, VaultTransit, Turnkey, GoogleCloudKms service account) and to any parent structs/enums that might be logged.
🧹 Nitpick comments (21)
src/models/error/signer.rs (1)
32-34: Add a unit test for the new CDP error display.Mirror existing tests to cover the
CdpErrorvariant’sDisplay/serialization.Example:
#[test] fn test_signer_error_from_cdp_error() { let err = CdpError::SigningError("oops".into()); let se = SignerError::from(err); match se { SignerError::CdpError(e) => assert_eq!(e.to_string(), "Signing error: oops"), _ => panic!("Expected SignerError::CdpError"), } }examples/solana-cdp-signer/README.md (2)
139-142: Use canonical header casing.HTTP headers are case-insensitive, but examples typically use
Authorization.- -H "AUTHORIZATION: Bearer $API_KEY" + -H "Authorization: Bearer $API_KEY"
64-65: Disambiguate config path.Explicit file path avoids confusion when editing from repo root.
-Edit the `config.json` file and update the following variables: +Edit `examples/solana-cdp-signer/config/config.json` and update the following fields:examples/evm-cdp-signer/README.md (2)
139-142: Use canonical header casing.- -H "AUTHORIZATION: Bearer $API_KEY" + -H "Authorization: Bearer $API_KEY"
64-65: Disambiguate config path.-Edit the `config.json` file and update the following variables: +Edit `examples/evm-cdp-signer/config/config.json` and update the following fields:examples/solana-cdp-signer/docker-compose.yaml (1)
28-46: Add a Redis healthcheck for more reliable startup/restarts.Improves resilience and observability during local runs.
redis: image: redis:bookworm + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 3s + retries: 5 ports: - 6379:6379/tcpsrc/services/signer/solana/cdp_signer.rs (4)
97-101: Fix misleading comment about CDP response format.Per the established pattern, the CDP service returns raw serialized bytes; we encode to base64 only to reuse
EncodedSerializedTransaction.- // The CDP API returns a complete base64-encoded signed transaction - // We need to decode it back to a Transaction and extract the signature + // The CDP service returns raw serialized signed-transaction bytes. + // Encode to base64 to reuse EncodedSerializedTransaction for parsing.
124-141: Limit signer lookup to the required-signatures range.On Solana, only the first
num_required_signatureskeys correspond tosignatures. This avoids false “out of bounds” on valid TXs.- // Find the signature index for the CDP signer's pubkey - let signer_index = signed_transaction - .message - .account_keys - .iter() - .position(|key| *key == cdp_pubkey) + // Find the signature index within required signers + let num = signed_transaction.message.header.num_required_signatures as usize; + let signer_index = signed_transaction + .message + .account_keys[..num] + .iter() + .position(|key| *key == cdp_pubkey) .ok_or_else(|| { SignerError::SigningError("CDP pubkey not found in transaction signers".to_string()) })?; // Extract the signature at the correct index - if signer_index >= signed_transaction.signatures.len() { + if signer_index >= signed_transaction.signatures.len() { return Err(SignerError::SigningError( "Signature index out of bounds".to_string(), )); }
82-83: Use importedTransactionalias for consistency.Minor readability win;
Transactionis already imported.- let transaction = solana_sdk::transaction::Transaction::new_unsigned(solana_message); + let transaction = Transaction::new_unsigned(solana_message); @@ - let signed_transaction: solana_sdk::transaction::Transaction = + let signed_transaction: Transaction = signed_tx_data.try_into().map_err(|e| { SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) })?;Also applies to: 102-105
343-349: Add a test for “CDP pubkey not found” path.Covers the error branch where the signer pubkey isn’t among required signers.
#[tokio::test] async fn test_sign_missing_cdp_pubkey() { let mut mock = MockCdpServiceTrait::new(); // Build a tx whose required signer is NOT the CDP pubkey use solana_sdk::{message::Message, pubkey::Pubkey, signature::Keypair}; use solana_system_interface::instruction; let payer = Keypair::new(); let other = Pubkey::new_unique(); let ix = instruction::transfer(&payer.pubkey(), &other, 1); let msg = Message::new(&[ix], Some(&payer.pubkey())); let msg_bytes = bincode::serialize(&msg).unwrap(); // Return a signed tx with a signature but a different signer key ordering let mut tx = Transaction::new_unsigned(msg.clone()); tx.signatures = vec![Signature::from([2u8; 64])]; let tx_bytes = bincode::serialize(&tx).unwrap(); mock.expect_address_solana() .returning(move || Box::pin(async { Ok(Address::Solana(other.to_string())) })); mock.expect_sign_solana_transaction() .returning(move |_| Box::pin(async { Ok(tx_bytes.clone()) })); let signer = CdpSigner::new_for_testing(mock); let res = signer.sign(&msg_bytes).await; assert!(matches!(res, Err(SignerError::SigningError(_)))); }src/models/signer/repository.rs (2)
131-146: Avoid deriving Debug on storage structs holding secrets.Even with SecretString, deriving Debug risks accidental logging. Prefer removing Debug or implementing a redacted Debug impl; apply consistently across secret-bearing storage structs.
-#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct CdpSignerConfigStorage {
479-486: Docstring nit: Option-returning getter does not “return error”.Update the comment to reflect it returns None when not CDP.
- /// Get CDP signer config, returns error if not a CDP signer + /// Get CDP signer config, returns None if not a CDP signersrc/services/cdp/mod.rs (2)
272-273: Fix error string: transaction, not message.Minor clarity tweak in the Solana transaction signing error.
- .map_err(|e| CdpError::SigningError(format!("Failed to sign Solana message: {}", e)))?; + .map_err(|e| CdpError::SigningError(format!("Failed to sign Solana transaction: {}", e)))?;
55-57: Remove unused CdpResult alias.It’s not referenced; trimming avoids dead code.
-/// Result type for CDP operations -pub type CdpResult<T> = Result<T, CdpError>;docs/openapi.json (3)
4511-4525: Constrain EVM signature ‘v’ to expected set.
Client contracts often rely on {27,28} or {0,1}. Encoding this reduces ambiguity."v": { - "type": "integer", - "format": "int32", - "minimum": 0 + "type": "integer", + "format": "int32", + "enum": [0, 1, 27, 28] }
6117-6140: Apply the same ‘v’ constraint to SignDataResponseEvm.
Keeps message-signature shape consistent with transaction signatures."v": { - "type": "integer", - "format": "int32", - "minimum": 0 + "type": "integer", + "format": "int32", + "enum": [0, 1, 27, 28] }
1650-1769: Document 501 Not Implemented for sign-typed-data.
CDP EVM typed data returns NotImplemented; expose 501 to match runtime behavior."responses": { "200": { ... }, "400": { ... }, "401": { ... }, "404": { ... }, "429": { ... }, + "501": { + "description": "Not Implemented - signer backend does not support EIP-712" + }, "500": { ... } },src/models/signer/config.rs (2)
675-698: Add a negative test for missing addresses.
Catches config regressions early.#[test] fn test_cdp_file_config_conversion() { @@ } + + #[test] + fn test_cdp_file_config_requires_one_address() { + let cfg = CdpSignerFileConfig { + api_key_id: "id".into(), + api_key_secret: PlainOrEnvValue::Plain { value: SecretString::new("asecret") }, + wallet_secret: PlainOrEnvValue::Plain { value: SecretString::new("wsecret") }, + evm_account_address: None, + solana_account_address: None, + }; + let res = CdpSignerConfig::try_from(cfg); + assert!(res.is_err()); + }
288-308: Require at least one address inCdpSignerFileConfig::try_from
Since there’s no existing validation forCdpSignerConfig, add this guard to error fast when both account addresses are missing:fn try_from(config: CdpSignerFileConfig) -> Result<Self, Self::Error> { + if config.evm_account_address.is_none() && config.solana_account_address.is_none() { + return Err(ConfigFileError::InvalidFormat( + "CDP signer: provide at least one of evm_account_address or solana_account_address".into() + )); + } let api_key_secret = config.api_key_secret.get_value().map_err(|e| { ConfigFileError::InvalidFormat(format!("Failed to get API key secret: {}", e)) })?;src/services/signer/evm/cdp_signer.rs (2)
151-179: Clarify EIP-191 prefixing source of truth.
Comment claims EIP-191 prefixing, but code delegates raw message to CDP. Confirm CDP applies the prefix; otherwise hash with eip191 locally before signing.If CDP expects pre-hashed input, compute:
let hash = eip191_hash_message(request.message.as_bytes()); let signature_bytes = self.cdp_service.sign_evm_hash(hash).await?;
57-66: Deduplicate test-only constructors.
Keep a singlenew_with_serviceand alias or removenew_for_testing.- pub fn new_for_testing(cdp_service: T) -> Self { - Self { cdp_service } - } + // Prefer a single constructor in tests + pub fn new_for_testing(cdp_service: T) -> Self { Self::new_with_service(cdp_service) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)README.md(1 hunks)docs/modules/ROOT/pages/configuration.adoc(1 hunks)docs/modules/ROOT/pages/evm.adoc(2 hunks)docs/modules/ROOT/pages/index.adoc(1 hunks)docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/modules/ROOT/pages/solana.adoc(1 hunks)docs/openapi.json(5 hunks)examples/evm-cdp-signer/.env.example(1 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/evm-cdp-signer/docker-compose.yaml(1 hunks)examples/solana-cdp-signer/.env.example(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/docker-compose.yaml(1 hunks)src/models/error/signer.rs(2 hunks)src/models/signer/config.rs(6 hunks)src/models/signer/mod.rs(6 hunks)src/models/signer/repository.rs(7 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)src/services/signer/stellar/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- docs/modules/ROOT/pages/configuration.adoc
- examples/solana-cdp-signer/.env.example
- examples/evm-cdp-signer/.env.example
- README.md
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/modules/ROOT/pages/solana.adoc
- examples/solana-cdp-signer/config/config.json
- Cargo.toml
- src/services/signer/stellar/mod.rs
- examples/evm-cdp-signer/docker-compose.yaml
- docs/modules/ROOT/pages/index.adoc
- src/models/signer/response.rs
- src/services/mod.rs
- examples/evm-cdp-signer/config/config.json
- docs/modules/ROOT/pages/evm.adoc
- src/services/signer/solana/mod.rs
- docs/modules/ROOT/pages/signers.adoc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T04:46:17.538Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Applied to files:
src/services/signer/solana/cdp_signer.rsexamples/evm-cdp-signer/README.mdexamples/solana-cdp-signer/README.md
📚 Learning: 2025-09-05T04:46:17.538Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Applied to files:
src/services/signer/solana/cdp_signer.rsexamples/evm-cdp-signer/README.md
🧬 Code graph analysis (8)
src/services/signer/solana/cdp_signer.rs (4)
src/services/signer/solana/mod.rs (6)
pubkey(100-100)pubkey(116-125)address(66-75)sign(111-111)sign(127-136)sign_transaction(77-89)src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (5)
new(49-54)new_with_service(59-61)new_for_testing(63-65)address(70-78)sign_transaction(80-146)
src/models/error/signer.rs (1)
src/models/signer/repository.rs (13)
from(59-63)from(67-71)from(190-195)from(200-205)from(209-214)from(218-223)from(227-236)from(240-249)from(253-263)from(267-277)from(281-289)from(293-301)from(305-313)
src/models/signer/request.rs (6)
src/models/secret_string.rs (1)
schema(145-157)src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/models/signer/mod.rs (2)
new(442-444)signer_type(447-449)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/models/signer/repository.rs (2)
src/models/signer/mod.rs (2)
from(491-501)get_cdp(359-364)src/models/signer/response.rs (3)
from(91-140)from(154-163)from(167-173)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-226)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(147-155)test_address(183-203)
src/models/signer/mod.rs (1)
src/models/signer/repository.rs (2)
validate(408-411)get_cdp(480-485)
src/models/signer/config.rs (4)
src/models/signer/request.rs (2)
try_from(214-280)try_from(286-327)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/services/signer/evm/cdp_signer.rs (4)
src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/services/cdp/mod.rs (1)
new(90-106)src/services/signer/solana/cdp_signer.rs (5)
new(39-45)new_with_service(50-52)new_for_testing(54-56)address(147-155)sign_transaction(157-170)src/services/signer/evm/mod.rs (6)
address(76-85)sign_transaction(87-99)sign_data(56-56)sign_data(104-113)sign_typed_data(59-62)sign_typed_data(115-127)
🪛 LanguageTool
examples/evm-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new EVM EOA Wallet using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - EVM Account Address ### Step 5: Configu...
(QB_NEW_EN)
examples/solana-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new Solana Account using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - Solana Account Address ### Step 5: Conf...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (13)
src/models/error/signer.rs (1)
4-4: No action needed:CdpErroralready derivesserde::Serialize.examples/solana-cdp-signer/README.md (1)
115-123: JSON snippet looks valid (no trailing comma).The previous trailing-comma issue is resolved. LGTM.
examples/evm-cdp-signer/README.md (1)
115-123: JSON snippet looks valid (no trailing comma).LGTM.
src/models/signer/repository.rs (1)
44-46: CDP storage wiring looks consistent and reversible.Variant addition and both-way conversions are clean and match the domain types. No blockers from my side here.
Also applies to: 304-326, 424-426, 440-442
src/models/signer/mod.rs (1)
358-365: LGTM: Accessor and type plumbing for CDP.Accessor, signer type mapping, and enum variant additions are consistent with existing patterns.
Also applies to: 382-383, 437-438
src/services/signer/evm/mod.rs (1)
16-23: EVM CDP signer integration is correctly routed.Module wiring, enum variant, factory branch, and tests look good. Calls are forwarded uniformly alongside existing signers.
Also applies to: 65-72, 76-85, 91-98, 104-112, 118-126, 173-178, 339-357, 417-439
src/models/signer/request.rs (2)
261-267: LGTM: CDP request → domain conversion.Secrets are wrapped into SecretString and optional addresses are passed through as-is. Validation happens at the domain layer.
888-981: Tests cover CDP request path well.Deserialization, creation, and validation cases look good and are aligned with the domain model.
docs/openapi.json (1)
6288-6293: CDP config added to request enum — looks good.
CDPSignerRequestConfigis now included inSignerConfigRequest.oneOf. This unblocks CDP create requests in generated clients.src/models/signer/config.rs (2)
49-57: CDP file config shape — good addition.
Fields and deny_unknown_fields align with security expectations.
156-162: Enum wiring for CDP — LGTM.
Addscdpvariant to file config enum and integrates with serde tags.src/services/signer/evm/cdp_signer.rs (2)
86-146: Transaction signing path — solid implementation.
Correctly selects legacy vs EIP-1559, decodes signed bytes, and normalizes v for 1559.
181-189: Typed-data not implemented — ensure API maps to 501.
Make sureSignerError::NotImplementedbecomes HTTP 501; OpenAPI update suggested separately.
| async fn sign_solana_message(&self, message: &[u8]) -> Result<Vec<u8>, CdpError> { | ||
| let address = self.get_solana_account_address()?; | ||
| let encoded_message = str::from_utf8(message) | ||
| .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))? | ||
| .to_string(); | ||
|
|
||
| let message_body = types::SignSolanaMessageBody::builder().message(encoded_message); | ||
|
|
There was a problem hiding this comment.
Don’t require UTF-8 for Solana messages; base58-encode the raw bytes.
Messages can be arbitrary bytes; forcing UTF-8 will fail. Encode payload as base58 before sending.
- let address = self.get_solana_account_address()?;
- let encoded_message = str::from_utf8(message)
- .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))?
- .to_string();
+ let address = self.get_solana_account_address()?;
+ // CDP expects the message as base58-encoded bytes for Solana.
+ let encoded_message = bs58::encode(message).into_string();🤖 Prompt for AI Agents
In src/services/cdp/mod.rs around lines 231 to 238, the code currently forces
the Solana message bytes into UTF-8 which will fail for arbitrary binary
payloads; instead base58-encode the raw message bytes and pass that string into
the SignSolanaMessageBody. Replace the str::from_utf8(...) conversion with a
base58 encoding (e.g., using the bs58 crate:
bs58::encode(message).into_string()), keep error handling limited to encoding
failures if any, and use that encoded string as the message supplied to
types::SignSolanaMessageBody::builder().message(...).
168f9d1 to
56b5a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/services/cdp/mod.rs (1)
234-241: Bug: Solana message must be base58-encoded bytes, not UTF-8 string.Arbitrary messages may not be valid UTF-8. Encode raw bytes as base58 before calling CDP.
Apply:
- let address = self.get_solana_account_address()?; - let encoded_message = str::from_utf8(message) - .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))? - .to_string(); + let address = self.get_solana_account_address()?; + // CDP expects the message as base58-encoded bytes for Solana. + let encoded_message = bs58::encode(message).into_string();Also update the import to drop
str(outside this hunk):- use std::{str, time::Duration}; + use std::time::Duration;src/models/signer/mod.rs (1)
354-359: Past ask addressed via schema validator; mapping LGTM.The earlier request to ensure cross-field CDP validation is effectively handled by the struct-level schema validator now. Good.
🧹 Nitpick comments (20)
examples/solana-cdp-signer/README.md (4)
138-142: Use standard header casing in curl exampleHeader names are case-insensitive, but conventional casing avoids confusion in tooling and docs.
-H "Content-Type: application/json" \ - -H "AUTHORIZATION: Bearer $API_KEY" + -H "Authorization: Bearer $API_KEY"
133-142: Include a signing example and note on base64 payloadsReaders can list relayers but still be blocked on how to submit a sign request. Add a minimal POST example and call out that Solana payloads must be base64-encoded for the API, even though the SDK returns decoded bytes.
Proposed addition (please confirm endpoint and field names before merging):
### Step 7: Test the Service @@ -2. You can test it using curl or any HTTP client: +2. You can test it using curl or any HTTP client: @@ ```bash curl -X GET http://localhost:8080/api/v1/relayers \ -H "Content-Type: application/json" \ -H "Authorization: Bearer $API_KEY"
+Example: sign a Solana transaction (base64-encoded):
+```bash
+curl -X POST http://localhost:8080/api/v1/solana/sign \
- -H "Content-Type: application/json" \
- -H "Authorization: Bearer $API_KEY" \
- -d '{
- "signer_id": "cdp-signer-solana",
- "transaction": "BASE64_ENCODED_SERIALIZED_TX"
- }'
+```
+Note: Provide base64-encoded bytes for message/transaction; the CDP SDK operates on raw bytes, but the Relayer API expects base64 in requests.If helpful, I can update this with the exact route and schema from the OpenAPI once you confirm. --- `7-12`: **Normalize ordered list formatting** Markdown renders repeated “1.” fine, but raw readers and some linters complain. Consider explicit numbering or bullets for readability. ```diff -1. A CDP account - [Sign up here](https://portal.cdp.coinbase.com/) -1. Rust and Cargo installed -1. Git -1. [Docker](https://docs.docker.com/get-docker/) -1. [Docker Compose](https://docs.docker.com/compose/install/) +1. A CDP account - [Sign up here](https://portal.cdp.coinbase.com/) +2. Rust and Cargo installed +3. Git +4. [Docker](https://docs.docker.com/get-docker/) +5. [Docker Compose](https://docs.docker.com/compose/install/)Also applies to: 26-36, 39-44
49-51: Add a security reminder about.envin the README
Inexamples/solana-cdp-signer/README.mdafter the environment setup (lines 49–51), insert:+> Security tip: Never commit your `.env` file. Ensure it is listed in `.gitignore`.(Note:
.envis already ignored in.gitignoreat line 36.)examples/evm-cdp-signer/README.md (3)
139-142: Normalize Authorization header casing in curl example.Use "Authorization" (as elsewhere) for consistency and to avoid confusion with some tooling that prints canonical-case headers.
- -H "AUTHORIZATION: Bearer $API_KEY" + -H "Authorization: Bearer $API_KEY"
89-99: Verify availability of generate_uuid example; offer portable alternatives.Confirm that cargo example exists; if not, provide a fallback to generate keys locally.
-# Generate API key -cargo run --example generate_uuid - -# Generate webhook signing key -cargo run --example generate_uuid +# Generate API key (fallback) +uuidgen | tr '[:upper:]' '[:lower:]' +# Generate webhook signing key (fallback: 32 random bytes hex) +openssl rand -hex 32
110-116: Be explicit about config path.Readers may edit the wrong file; specify the full path.
-Update the `examples/evm-cdp-signer/config/config.json` file with your webhook configuration: +Update the file at `examples/evm-cdp-signer/config/config.json` with your webhook configuration:src/models/error/signer.rs (1)
32-34: Add unit test for CdpError parity.Mirror existing From/Display tests to keep coverage consistent across signer backends.
@@ #[test] fn test_signer_error_from_turnkey_error() { @@ } + + #[test] + fn test_signer_error_from_cdp_error() { + let cdp_error = CdpError::SigningError("bad sig".to_string()); + let signer_error = SignerError::from(cdp_error); + match signer_error { + SignerError::CdpError(e) => { + assert_eq!(e.to_string(), "Signing error: bad sig"); + } + _ => panic!("Expected SignerError::CdpError"), + } + }docs/modules/ROOT/pages/evm.adoc (2)
288-292: Include CDP in Signer Security section for consistency.Earlier sections recommend CDP; keep the list aligned.
-- Use hosted signers (AWS KMS, Google Cloud KMS, Turnkey) in production +- Use hosted signers (AWS KMS, Google Cloud KMS, Turnkey, CDP) in production
231-239: Use numeric type for gas_price to match other fee fields.Other fee fields are numeric; keep types consistent.
- "gas_price": "12312313123" + "gas_price": 12312313123src/services/signer/solana/cdp_signer.rs (2)
123-131: Limit pubkey search to required signers to avoid false matches.Currently searches all account keys; restrict to the first num_required_signatures.
- let signer_index = signed_transaction - .message - .account_keys - .iter() - .position(|key| *key == cdp_pubkey) + let required = signed_transaction.message.header.num_required_signatures as usize; + let signer_index = signed_transaction + .message + .account_keys[..required] + .iter() + .position(|key| *key == cdp_pubkey) .ok_or_else(|| { SignerError::SigningError("CDP pubkey not found in transaction signers".to_string()) })?;
59-69: Optional: cache CDP pubkey to avoid extra network round-trip.If config includes solana_account_address, store it to skip address_solana() calls in sign().
pub struct CdpSigner<T = DefaultCdpService> where T: CdpServiceTrait, { cdp_service: T, + cached_pubkey: Option<Pubkey>, } @@ - pub fn new(config: CdpSignerConfig) -> Result<Self, SignerError> { + pub fn new(config: CdpSignerConfig) -> Result<Self, SignerError> { let cdp_service = DefaultCdpService::new(config).map_err(|e| { SignerError::Configuration(format!("Failed to create CDP service: {}", e)) })?; - Ok(Self { cdp_service }) + let cached_pubkey = None; // optionally parse from config.solana_account_address + Ok(Self { cdp_service, cached_pubkey }) }src/services/cdp/mod.rs (3)
124-135: Optional: Validate Solana address format at config-read time.A quick base58 decode check (32 bytes) catches misconfigurations early.
fn get_solana_account_address(&self) -> Result<String, CdpError> { - self.config + let addr = self.config .solana_account_address .as_ref() .cloned() .ok_or_else(|| { CdpError::ConfigError( "Solana account address not configured for CDP signer".to_string(), ) })?; + // Basic format validation: base58 decode to 32 bytes + let pk = bs58::decode(&addr).into_vec().map_err(|e| { + CdpError::ConfigError(format!("Invalid Solana address (base58): {}", e)) + })?; + if pk.len() != 32 { + return Err(CdpError::ConfigError(format!( + "Invalid Solana address length: expected 32 bytes, got {}", + pk.len() + ))); + } + Ok(addr) - } + }
137-157: Optional: Consider EIP-55 checksum validation for EVM addresses.You already enforce 20-byte length; optionally validate mixed-case strings against EIP-55 to catch typos. Keep accepting all-lowercase for flexibility.
101-108: Nice-to-have: Add limited retries with jitter on transient failures.A small retry policy (e.g., reqwest-retry with exponential backoff) can smooth over occasional network blips to CDP.
Example (requires reqwest-retry):
use reqwest_retry::{policies::ExponentialBackoff, RetryTransientMiddleware}; let retry = ExponentialBackoff::builder().build_with_max_retries(2); let wallet_client = ClientBuilder::new(inner) .with(wallet_auth) .with(RetryTransientMiddleware::new_with_policy(retry)) .build();docs/openapi.json (1)
7280-7315: Require at least one blockchain address in CDPSignerRequestConfig
Add ananyOfconstraint to enforce that eitherevm_account_addressorsolana_account_addressis provided on creation."CDPSignerRequestConfig": { "type": "object", "description": "CDP signer configuration for API requests", "required": ["api_key_id","api_key_secret","wallet_secret"], "properties": { "api_key_id": { "type": "string", "minLength": 1 }, "api_key_secret": { "type": "string", "format": "password", "writeOnly": true, "minLength": 1 }, "wallet_secret": { "type": "string", "format": "password", "writeOnly": true, "minLength": 1 }, "evm_account_address": { "type": "string", "pattern": "^0x[0-9a-fA-F]{40}$" }, "solana_account_address": { "type": "string", "pattern": "^[1-9A-HJ-NP-Za-km-z]{32,44}$" } }, + "anyOf": [ + { "required": ["evm_account_address"] }, + { "required": ["solana_account_address"] } + ], "additionalProperties": false }src/services/signer/evm/cdp_signer.rs (1)
80-96: Minor: Avoid double extraction of EVM data.You fetch
evm_datato branch on 1559 but then buildTx*fromtransaction. Consider constructing fromevm_datato make intent explicit (no behavior change).- let evm_data = transaction.get_evm_transaction_data()?; + let evm_data = transaction.get_evm_transaction_data()?; // Prepare data for signing based on transaction type - let (unsigned_tx_bytes, is_eip1559) = if evm_data.is_eip1559() { - let tx = TxEip1559::try_from(transaction)?; + let (unsigned_tx_bytes, is_eip1559) = if evm_data.is_eip1559() { + let tx = TxEip1559::try_from(evm_data.clone())?; (tx.encoded_for_signing(), true) } else { - let tx = TxLegacy::try_from(transaction)?; + let tx = TxLegacy::try_from(evm_data)?; (tx.encoded_for_signing(), false) };src/models/signer/mod.rs (3)
263-312: Strengthen address validation (decode Solana Base58; optionally enforce EIP-55).Current Solana checks are heuristic (length/charset). Prefer decoding Base58 and verifying 32 bytes. For EVM, consider optional EIP-55 checksum validation for stronger guarantees.
Apply within this function (add bs58 dependency if not present):
- // Validate Solana address format if provided (Base58, typically 32-44 chars) + // Validate Solana address format if provided (Base58 → 32 bytes ed25519 pubkey) if let Some(ref solana_addr) = config.solana_account_address { - // Basic length check for Solana addresses - if solana_addr.len() < 32 || solana_addr.len() > 44 { - let mut error = validator::ValidationError::new("invalid_solana_address_length"); - error.message = Some("solana_account_address must be between 32-44 characters".into()); - return Err(error); - } - // Check for Base58 character set (no 0, O, I, l) - if !solana_addr - .chars() - .all(|c| c.is_ascii_alphanumeric() && c != '0' && c != 'O' && c != 'I' && c != 'l') - { - let mut error = validator::ValidationError::new("invalid_solana_address_charset"); - error.message = Some( - "solana_account_address contains invalid characters (must be valid Base58)".into(), - ); - return Err(error); - } + match bs58::decode(solana_addr).into_vec() { + Ok(bytes) if bytes.len() == 32 => {} + Ok(_) => { + let mut error = validator::ValidationError::new("invalid_solana_address_length"); + error.message = Some("solana_account_address must decode to 32 bytes".into()); + return Err(error); + } + Err(_) => { + let mut error = validator::ValidationError::new("invalid_solana_address_charset"); + error.message = Some("solana_account_address must be valid Base58".into()); + return Err(error); + } + } }Optional: enforce EIP-55 on evm_account_address via ethers-core utils (is_checksum) or alloy if those deps exist in the workspace.
321-321: CDP enum wiring and type exposure look correct; add serde roundtrip test.Add “cdp” to the existing SignerType serialization test to prevent regressions.
Example assertions to add:
use serde_json::{from_str, to_string}; assert_eq!(to_string(&SignerType::Cdp).unwrap(), "\"cdp\""); assert_eq!(from_str::<SignerType>("\"cdp\"").unwrap(), SignerType::Cdp);Also applies to: 433-433, 488-488
954-1096: CDP tests cover key paths; add two small cases for edge robustness.
- Both addresses provided, one invalid → verify field-specific error is surfaced.
- Empty secrets → verify correct messages for api_key_secret and wallet_secret.
Example tests:
#[test] fn test_cdp_both_addresses_one_invalid() { let config = CdpSignerConfig { api_key_id: "k".into(), api_key_secret: SecretString::new("s"), wallet_secret: SecretString::new("w"), evm_account_address: Some("invalid".into()), solana_account_address: Some("6s7RsvzcdXFJi1tXeDoGfSKZFzN3juVt9fTar6WEhEm2".into()), }; let signer = Signer::new("cdp".into(), SignerConfig::Cdp(config)); let err = signer.validate().unwrap_err(); if let SignerValidationError::InvalidConfig(msg) = err { assert!(msg.contains("evm_account_address")); } else { panic!("expected InvalidConfig"); } } #[test] fn test_cdp_empty_secrets() { let config = CdpSignerConfig { api_key_id: "k".into(), api_key_secret: SecretString::new(""), wallet_secret: SecretString::new(""), evm_account_address: Some("0x742d35Cc6634C0532925a3b844Bc454e4438f44f".into()), solana_account_address: None, }; let signer = Signer::new("cdp".into(), SignerConfig::Cdp(config)); let err = signer.validate().unwrap_err(); if let SignerValidationError::InvalidConfig(msg) = err { assert!(msg.contains("API Key Secret cannot be empty") || msg.contains("API Wallet Secret cannot be empty")); } else { panic!("expected InvalidConfig"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)README.md(1 hunks)docs/modules/ROOT/pages/configuration.adoc(1 hunks)docs/modules/ROOT/pages/evm.adoc(2 hunks)docs/modules/ROOT/pages/index.adoc(1 hunks)docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/modules/ROOT/pages/solana.adoc(1 hunks)docs/openapi.json(5 hunks)examples/evm-cdp-signer/.env.example(1 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/evm-cdp-signer/docker-compose.yaml(1 hunks)examples/solana-cdp-signer/.env.example(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/docker-compose.yaml(1 hunks)src/models/error/signer.rs(2 hunks)src/models/signer/config.rs(6 hunks)src/models/signer/mod.rs(8 hunks)src/models/signer/repository.rs(7 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)src/services/signer/stellar/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/modules/ROOT/pages/solana.adoc
🚧 Files skipped from review as they are similar to previous changes (18)
- Cargo.toml
- examples/solana-cdp-signer/.env.example
- docs/modules/ROOT/pages/configuration.adoc
- examples/evm-cdp-signer/.env.example
- examples/solana-cdp-signer/config/config.json
- src/services/signer/stellar/mod.rs
- examples/solana-cdp-signer/docker-compose.yaml
- README.md
- docs/modules/ROOT/pages/signers.adoc
- docs/modules/ROOT/pages/index.adoc
- src/services/mod.rs
- src/models/signer/repository.rs
- examples/evm-cdp-signer/docker-compose.yaml
- src/services/signer/evm/mod.rs
- examples/evm-cdp-signer/config/config.json
- src/models/signer/request.rs
- src/models/signer/response.rs
- src/models/signer/config.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T04:46:17.538Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Applied to files:
src/services/signer/solana/cdp_signer.rsexamples/evm-cdp-signer/README.mdexamples/solana-cdp-signer/README.md
📚 Learning: 2025-09-05T04:46:17.538Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.538Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Applied to files:
src/services/signer/solana/cdp_signer.rsexamples/evm-cdp-signer/README.md
🧬 Code graph analysis (6)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-226)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(146-154)test_address(183-203)
src/models/error/signer.rs (1)
src/models/signer/repository.rs (13)
from(59-63)from(67-71)from(190-195)from(200-205)from(209-214)from(218-223)from(227-236)from(240-249)from(253-263)from(267-277)from(281-289)from(293-301)from(305-313)
src/services/signer/solana/cdp_signer.rs (3)
src/services/signer/solana/mod.rs (6)
pubkey(100-100)pubkey(116-125)address(66-75)sign(111-111)sign(127-136)sign_transaction(77-89)src/services/cdp/mod.rs (1)
new(90-109)src/services/signer/evm/cdp_signer.rs (5)
new(49-54)new_with_service(59-61)new_for_testing(63-65)address(70-78)sign_transaction(80-146)
src/services/signer/solana/mod.rs (3)
src/services/signer/solana/local_signer.rs (1)
new(40-57)src/services/signer/solana/turnkey_signer.rs (1)
new(44-46)src/services/signer/solana/vault_transit_signer.rs (1)
new(50-61)
src/models/signer/mod.rs (4)
src/models/signer/repository.rs (2)
validate(408-411)get_cdp(480-485)src/services/cdp/mod.rs (1)
new(90-109)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/services/signer/evm/cdp_signer.rs (4)
src/models/signer/config.rs (10)
new(192-194)try_from(215-256)try_from(262-267)try_from(273-285)try_from(291-307)try_from(313-331)try_from(337-356)try_from(362-411)try_from(417-441)try_from(447-469)src/services/cdp/mod.rs (1)
new(90-109)src/services/signer/solana/cdp_signer.rs (5)
new(39-45)new_with_service(50-52)new_for_testing(54-56)address(146-154)sign_transaction(156-169)src/services/signer/evm/mod.rs (6)
address(76-85)sign_transaction(87-99)sign_data(56-56)sign_data(104-113)sign_typed_data(59-62)sign_typed_data(115-127)
🪛 LanguageTool
examples/evm-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new EVM EOA Wallet using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - EVM Account Address ### Step 5: Configu...
(QB_NEW_EN)
examples/solana-cdp-signer/README.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...inbase.com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...com/) 1. Rust and Cargo installed 1. Git 1. [Docker](https://docs.docker.com/get-dock...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...st and Cargo installed 1. Git 1. Docker 1. [Docker Compose](https://docs.docker.com/...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...shboard. 1. Create a new Secret API Key. 1. Save both the API Key ID and Secret - yo...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...t - you'll need these for configuration. 1. Note: The API Key Secret is only shown o...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... Generate a new Wallet Secret if needed. 1. Create a new Solana Account using either...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...sdk) 1. Note down the following details: - Solana Account Address ### Step 5: Conf...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (19)
src/models/error/signer.rs (1)
4-4: Add CDP error import — LGTM.docs/modules/ROOT/pages/evm.adoc (4)
40-41: Add CDP to supported signers — LGTM.
46-47: Production note updated to include CDP — LGTM.
250-250: Validate external docs link.The percent-encoded double hyphen in the URL looks suspicious; please verify the target renders.
54-56: No action needed: Rust version requirement is aligned. Docs specify “Rust 1.86 or later,” which matches therust-version = "1.86"in Cargo.toml and thechannel = "1.86.0"in rust-toolchain.toml (used by CI).src/services/signer/solana/mod.rs (3)
30-32: Wire CDP signer into Solana module — LGTM.
185-190: Factory: good error mapping on CDP service creation.Creation failures surface as CreationFailed with CDP context — LGTM.
468-491: CDP address/pubkey tests — LGTM.Covers address and pubkey retrieval paths.
src/services/signer/solana/cdp_signer.rs (3)
38-46: Constructor and service wiring — LGTM.
71-89: Message→transaction flow — LGTM.Deserialization and use of EncodedSerializedTransaction aligns with project conventions.
146-169: sign_transaction delegation — LGTM.Clean handoff to CDP service with correct response wrapping.
src/services/cdp/mod.rs (3)
101-108: Good: HTTP timeouts added on the underlying client.Prevents indefinite hangs on network calls.
172-199: Good: Hex decode of EVM signature with 0x handling.Robust against prefixed/unprefixed responses.
279-286: Good: Base64 decode for Solana signed transaction.Matches common encoding for serialized transactions.
docs/openapi.json (1)
6411-6437: Good: Response schema enforces “at least one address”.anyOf on evm_account_address/solana_account_address prevents empty CDP configs from leaking through responses.
src/services/signer/evm/cdp_signer.rs (2)
120-126: Good: Normalize EIP-1559 v to yParity (0/1).Ensures consistent v across EIP-2718 typed transactions.
151-179: Good: Validate 65-byte signature and return r/s/v and full hex.Matches common EVM expectations and prevents downstream panics.
src/models/signer/mod.rs (2)
182-201: CDP config struct looks solid.Fields and validations are appropriate; secrets use SecretString and are non-empty-validated.
409-416: Accessor is consistent with other getters.Provides ergonomic access without exposing internals.
dylankilkenny
left a comment
dylankilkenny
left a comment
There was a problem hiding this comment.
@sammccord Thanks for adding CDP signer support!
Just added the one suggestion to consolidate the CDP config to a single address file instead of 1 per network.
Let me know what you think or if you've any concerns
src/models/signer/config.rs
Outdated
| pub evm_account_address: Option<String>, | ||
| pub solana_account_address: Option<String>, |
There was a problem hiding this comment.
Lets add single required address field that works for all networks rather than network specific fields. As we add more networks the latter will lead to field sprawl and a single field is consistent with our other signers.
I would validate this field in validate_cdp_config() that it matches either evm/solana format. Perhaps we can add some util funcs for matching. One idea here might be to parse the addresses using solana and alloy sdk
src/models/signer/mod.rs
Outdated
| } | ||
|
|
||
| // Validate Solana address format if provided (Base58, typically 32-44 chars) | ||
| if let Some(ref solana_addr) = config.solana_account_address { |
There was a problem hiding this comment.
For validation we could leverage sdk Pubkey::from_str(s).is_ok()
| } | ||
|
|
||
| /// Converts a CDP EVM address to our Address type | ||
| fn address_from_string(&self, address_str: &str) -> Result<Address, CdpError> { |
There was a problem hiding this comment.
we could use the matching util for addresses in place of this
There was a problem hiding this comment.
Could you give an example of what that'd look like? I'm still getting my feet wet rust-wise.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
Looks great!
I left question around CDP API, can it be used to fetch address in runtime given wallet and api config are defined. That would make config simpler.
| "type": "env", | ||
| "value": "CDP_WALLET_SECRET" | ||
| }, | ||
| "solana_account_address": "" |
There was a problem hiding this comment.
I wonder can we fetch address in runtime given that we have api key info and wallet secret?
If that if feasible config would be simpler.
There was a problem hiding this comment.
It would be simpler, but given that CDP project can have many evm and solana accounts, we'd need to know ahead of time which one a user wants to sign with.
|
I tried testing EVM by sending a transaction and got the following error during transaction signing: " Signer error: CDP error: Signing error: Failed to sign transaction: Communication Error: Authentication error: Failed to encode wallet JWT: InvalidEcdsaKey". I used the API key and wallet secret from the official CDP SDK EVM create account example and then tried to use them in the relayer service. |
hm, I'm able to follow the evm example e2e - This is most likely a config issue. To sanity check, can you confirm your .env resembles the following? REDIS_URL=redis://redis:6379
API_KEY= arbitrary uuid
WEBHOOK_SIGNING_KEY= arbitrary uuid
CDP_API_KEY_SECRET= 86 character string ending in ==
CDP_WALLET_SECRET= 184 character string |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/models/signer/request.rs (1)
112-120: Remove Debug and zeroize CDP secrets on drop.Avoid accidental secret logging and ensure memory is wiped.
Apply:
-/// CDP signer configuration for API requests -#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] +/// CDP signer configuration for API requests +#[derive(Serialize, Deserialize, ToSchema, Zeroize)] +#[zeroize(drop)] pub struct CdpSignerRequestConfig { pub api_key_id: String, pub api_key_secret: String, pub wallet_secret: String, pub account_address: String, }src/services/cdp/mod.rs (1)
230-241: Solana message encoding must be base58 (not UTF-8).Signing arbitrary bytes will fail with current UTF-8 coercion. Encode the raw bytes to base58 before sending.
Apply:
- let address = self.get_account_address(); - let encoded_message = str::from_utf8(message) - .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))? - .to_string(); + let address = self.get_account_address(); + // CDP expects base58-encoded message bytes for Solana. + let encoded_message = bs58::encode(message).into_string();
🧹 Nitpick comments (2)
src/services/signer/evm/mod.rs (2)
49-49: Remove unused import CdpService.
CdpServiceisn’t referenced in this module; avoid warnings.- AwsKmsService, CdpService, GoogleCloudKmsService, TurnkeyServiceTrait, + AwsKmsService, GoogleCloudKmsService, TurnkeyServiceTrait,
173-178: Improve CDP creation error to be actionable.Common fielding error is “InvalidEcdsaKey”; map this to a clearer hint about wallet/API secrets format to speed up debugging.
- SignerConfig::Cdp(config) => { - let cdp_signer = CdpSigner::new(config.clone()).map_err(|e| { - SignerFactoryError::CreationFailed(format!("CDP service error: {}", e)) - })?; - EvmSigner::Cdp(cdp_signer) - } + SignerConfig::Cdp(config) => { + let cdp_signer = CdpSigner::new(config.clone()).map_err(|e| { + let msg = e.to_string(); + let hint = if msg.contains("InvalidEcdsaKey") { + " Hint: verify CDP_WALLET_SECRET is the correct wallet secret (base64, no whitespace) and CDP_API_KEY_SECRET matches the API key; re-copy from CDP to avoid truncation." + } else { "" }; + SignerFactoryError::CreationFailed(format!("CDP service error: {}{}", msg, hint)) + })?; + EvmSigner::Cdp(cdp_signer) + }If
CdpErrorexposes variants, prefer matching on them instead of string inspection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/openapi.json(5 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)src/models/signer/config.rs(6 hunks)src/models/signer/mod.rs(9 hunks)src/models/signer/repository.rs(7 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/solana-cdp-signer/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/evm-cdp-signer/config/config.json
- examples/evm-cdp-signer/README.md
- src/services/signer/solana/cdp_signer.rs
- src/services/signer/solana/mod.rs
- examples/solana-cdp-signer/config/config.json
🧰 Additional context used
🧬 Code graph analysis (8)
src/services/signer/evm/mod.rs (3)
src/models/signer/mod.rs (1)
new(479-481)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)
src/services/signer/evm/cdp_signer.rs (3)
src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/solana/cdp_signer.rs (5)
new(39-45)new_with_service(50-52)new_for_testing(54-56)address(146-154)sign_transaction(156-169)src/services/signer/evm/mod.rs (6)
address(76-85)sign_transaction(87-99)sign_data(56-56)sign_data(104-113)sign_typed_data(59-62)sign_typed_data(115-127)
src/models/signer/request.rs (5)
src/models/signer/config.rs (10)
new(191-193)try_from(214-255)try_from(261-266)try_from(272-284)try_from(290-305)try_from(311-329)try_from(335-354)try_from(360-409)try_from(415-439)try_from(445-467)src/models/signer/mod.rs (2)
new(479-481)signer_type(484-486)src/services/cdp/mod.rs (3)
new(87-106)account_address(65-65)account_address(153-156)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/models/signer/config.rs (4)
src/models/signer/request.rs (2)
try_from(211-276)try_from(282-323)src/services/cdp/mod.rs (3)
new(87-106)account_address(65-65)account_address(153-156)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/services/cdp/mod.rs (4)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-229)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(146-154)test_address(183-206)src/services/signer/evm/mod.rs (1)
address(76-85)src/services/signer/solana/mod.rs (1)
address(66-75)
src/models/signer/mod.rs (4)
src/services/signer/solana/cdp_signer.rs (2)
pubkey(61-69)new(39-45)src/services/signer/solana/mod.rs (2)
pubkey(100-100)pubkey(116-125)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)
src/models/signer/response.rs (2)
src/models/signer/mod.rs (2)
new(479-481)from(528-538)src/models/signer/repository.rs (15)
from(59-63)from(67-71)from(189-194)from(199-204)from(208-213)from(217-222)from(226-235)from(239-248)from(252-262)from(266-276)from(280-288)from(292-300)from(304-311)from(315-322)from(326-331)
src/models/signer/repository.rs (2)
src/models/signer/mod.rs (2)
from(528-538)get_cdp(396-401)src/models/signer/response.rs (3)
from(90-138)from(152-161)from(165-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (21)
docs/openapi.json (3)
6289-6292: CDP request config correctly added to oneOf.Wires the new type into SignerConfigRequest as expected.
6411-6427: CDP response shape looks good; non-secrets only.Fields and additionalProperties=false align with the Rust response enum.
6488-6490: Enums updated with "cdp".Prevents client/server schema mismatches.
Also applies to: 6501-6503
docs/modules/ROOT/pages/signers.adoc (2)
40-41: Docs: CDP listed as supported signer type.Matches API and code.
80-84: Network matrix entry for CDP is accurate.EVM ✅, Solana ✅, Stellar ❌.
src/models/signer/response.rs (3)
53-58: CDP response variant: minimal, non-secret.Appropriately exposes only
api_key_idandaccount_address.
116-119: From mapping for CDP is correct.Straight mapping, no secrets leaked.
306-377: Solid CDP tests.Covers conversion, serialization (secrets hidden), and deserialization.
src/models/signer/repository.rs (5)
16-20: Imports updated for CDP.Keeps repository in sync with domain.
44-46: Storage enum includes CDP.Consistent with other signer backends.
131-146: CDP storage struct uses secret-aware serde.Correct use of secure serializers for secrets.
303-323: Bi-directional conversions for CDP storage/domain are correct.Field-for-field mapping, including secrets.
476-482: Getter for CDP storage added.Parity with other getters.
src/services/signer/evm/mod.rs (5)
16-23: CDP module wired into EVM signer.Variant and imports added as expected.
65-72: EvmSigner::Cdp variant added.Completes the polymorphic surface.
76-85: Match arms include CDP across address/sign flows.Keeps behavior consistent with other backends.
Also applies to: 91-99, 104-112, 119-126
339-357: Factory test for CDP creation added.Basic path coverage looks good.
416-437: Address resolution test for CDP added.Confirms passthrough address behavior.
src/services/signer/evm/cdp_signer.rs (1)
80-146: EVM signing flow looks solid (legacy + EIP-1559).Good extraction of hash/signature and v-normalization for EIP-1559; tests exercise both paths.
src/models/signer/request.rs (1)
258-263: Conversion to domain config correctly wraps secrets.
SecretString::new(...)ensures secrets aren’t exposed downstream. LGTM.src/models/signer/config.rs (1)
287-306: Config-file → domain conversion for CDP is correct.Secrets retrieved via
PlainOrEnvValue::get_value()with clear error mapping. LGTM.
| == CDP Signer Configuration | ||
|
|
||
| Uses CDP's secure key management infrastructure. | ||
|
|
||
| [source,json] | ||
| ---- | ||
| { | ||
| "id": "cdp-signer", | ||
| "type": "cdp", | ||
| "config": { | ||
| "api_key_id": "your-cdp-api-key-id", | ||
| "api_key_secret": { | ||
| "type": "env", | ||
| "value": "CDP_API_KEY_SECRET" | ||
| }, | ||
| "wallet_secret": { | ||
| "type": "env", | ||
| "value": "CDP_WALLET_SECRET" | ||
| }, | ||
| "account_address": "your-cdp-evm-or-solana-account-address" | ||
| } | ||
| } | ||
| ---- | ||
|
|
||
| Configuration fields: | ||
| [cols="1,1,2"] | ||
| |=== | ||
| |Field |Type |Description | ||
|
|
||
| | api_key_id | ||
| | String | ||
| | The Key ID of a Secret API Key. Used for authentication to the CDP signing service | ||
|
|
||
| | api_key_secret.type | ||
| | String | ||
| | Type of value source (`env` or `plain`) | ||
|
|
||
| | api_key_secret.value | ||
| | String | ||
| | The API key secret or environment variable name containing it. Used with the Key ID to authenticate API requests | ||
|
|
||
| | wallet_secret.type | ||
| | String | ||
| | Type of value source (`env` or `plain`) | ||
|
|
||
| | wallet_secret.value | ||
| | String | ||
| | The Wallet Secret or environment variable name containing it. Used to authorize API requests for signing operations. | ||
|
|
||
| | account_address | ||
| | String | ||
| | The address of the CDP EVM EOA or CDP Solana Account used for signing operations. | ||
| |=== | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Call out that account_address is required and clarify format.
Prevents config drift with the API/domain and helps users pick the correct address string.
Apply below note right after the JSON block:
+NOTE: `account_address` is required. Use:
+- EVM: 0x-prefixed, 40-hex-chars (checksummed or lowercased)
+- Solana: base58, 32–44 chars📝 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.
| == CDP Signer Configuration | |
| Uses CDP's secure key management infrastructure. | |
| [source,json] | |
| ---- | |
| { | |
| "id": "cdp-signer", | |
| "type": "cdp", | |
| "config": { | |
| "api_key_id": "your-cdp-api-key-id", | |
| "api_key_secret": { | |
| "type": "env", | |
| "value": "CDP_API_KEY_SECRET" | |
| }, | |
| "wallet_secret": { | |
| "type": "env", | |
| "value": "CDP_WALLET_SECRET" | |
| }, | |
| "account_address": "your-cdp-evm-or-solana-account-address" | |
| } | |
| } | |
| ---- | |
| Configuration fields: | |
| [cols="1,1,2"] | |
| |=== | |
| |Field |Type |Description | |
| | api_key_id | |
| | String | |
| | The Key ID of a Secret API Key. Used for authentication to the CDP signing service | |
| | api_key_secret.type | |
| | String | |
| | Type of value source (`env` or `plain`) | |
| | api_key_secret.value | |
| | String | |
| | The API key secret or environment variable name containing it. Used with the Key ID to authenticate API requests | |
| | wallet_secret.type | |
| | String | |
| | Type of value source (`env` or `plain`) | |
| | wallet_secret.value | |
| | String | |
| | The Wallet Secret or environment variable name containing it. Used to authorize API requests for signing operations. | |
| | account_address | |
| | String | |
| | The address of the CDP EVM EOA or CDP Solana Account used for signing operations. | |
| |=== | |
| == CDP Signer Configuration | |
| Uses CDP's secure key management infrastructure. | |
| [source,json] | |
| ---- | |
| { | |
| "id": "cdp-signer", | |
| "type": "cdp", | |
| "config": { | |
| "api_key_id": "your-cdp-api-key-id", | |
| "api_key_secret": { | |
| "type": "env", | |
| "value": "CDP_API_KEY_SECRET" | |
| }, | |
| "wallet_secret": { | |
| "type": "env", | |
| "value": "CDP_WALLET_SECRET" | |
| }, | |
| "account_address": "your-cdp-evm-or-solana-account-address" | |
| } | |
| } | |
| ---- | |
| NOTE: `account_address` is required. Use: | |
| - EVM: 0x-prefixed, 40-hex-chars (checksummed or lowercased) | |
| - Solana: base58, 32–44 chars | |
| Configuration fields: | |
| [cols="1,1,2"] | |
| |=== | |
| |Field |Type |Description | |
| | api_key_id | |
| | String | |
| | The Key ID of a Secret API Key. Used for authentication to the CDP signing service | |
| | api_key_secret.type | |
| | String | |
| | Type of value source (`env` or `plain`) | |
| | api_key_secret.value | |
| | String | |
| | The API key secret or environment variable name containing it. Used with the Key ID to authenticate API requests | |
| | wallet_secret.type | |
| | String | |
| | Type of value source (`env` or `plain`) | |
| | wallet_secret.value | |
| | String | |
| | The Wallet Secret or environment variable name containing it. Used to authorize API requests for signing operations. | |
| | account_address | |
| | String | |
| | The address of the CDP EVM EOA or CDP Solana Account used for signing operations. | |
| |=== |
🤖 Prompt for AI Agents
In docs/modules/ROOT/pages/signers.adoc around lines 468 to 521, the docs do not
state that account_address is required nor clarify its expected format; add a
short note immediately after the JSON code block stating that account_address is
required and must be the CDP EVM externally-owned account address (hex with 0x
prefix) or the CDP Solana account address (base58), and that the value must
match the account registered in the CDP service or API/domain; keep the note
concise and show both accepted formats and the requirement.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/models/signer/request.rs (1)
112-120: Do not derive Debug for secret-bearing config; zeroize on drop.Deriving Debug risks leaking secrets in logs, and
Zeroizealone doesn’t wipe on drop.-/// CDP signer configuration for API requests -#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] +/// CDP signer configuration for API requests +#[derive(Serialize, Deserialize, ToSchema, Zeroize)] +#[zeroize(drop)] pub struct CdpSignerRequestConfig { pub api_key_id: String, pub api_key_secret: String, pub wallet_secret: String, pub account_address: String, }If you need Debug for troubleshooting, implement a custom redacting Debug instead of deriving it.
🧹 Nitpick comments (7)
docs/openapi.json (3)
6288-6293: Reduce client ambiguity: add discriminator guidance between type and config.Because
SignerConfigRequestis a oneOf without a discriminator, many generators struggle. Add a note/discriminator mapping onSignerCreateRequestindicating thattypeselects theconfigschema, or document this explicitly in the description/examples."SignerCreateRequest": { "type": "object", "description": "Request model for creating a new signer", @@ "properties": { "config": { - "$ref": "#/components/schemas/SignerConfigRequest", - "description": "The signer configuration" + "$ref": "#/components/schemas/SignerConfigRequest", + "description": "The signer configuration (must match the 'type' field; e.g. type=cdp -> CDPSignerRequestConfig)" }, "id": { "type": "string", "description": "Optional ID - if not provided, a UUID will be generated" }, "type": { "$ref": "#/components/schemas/SignerTypeRequest", - "description": "The type of signer" + "description": "The type of signer (selects the shape of 'config')" } }, + "discriminator": { + "propertyName": "type", + "mapping": { + "cdp": "#/components/schemas/CDPSignerRequestConfig" + } + }, "additionalProperties": false },
6411-6427: Add address format constraint to CDP response for symmetry.Consider applying the same address regex used in the request to
SignerConfigResponse’s CDP entry to keep schema contracts aligned."account_address": { - "type": "string" + "type": "string", + "pattern": "^((0x[0-9a-fA-F]{40})|([1-9A-HJ-NP-Za-km-z]{32,44}))$" }
7270-7302: CDP request schema looks solid.Required fields, writeOnly, and patterns are in place. Consider adding examples for expected values to aid integrators.
"CDPSignerRequestConfig": { "type": "object", "description": "CDP signer configuration for API requests", @@ "properties": { "api_key_id": { "type": "string", "minLength": 1 + ,"example": "abcd1234-...-ef01" }, "api_key_secret": { "type": "string", "format": "password", "writeOnly": true, "minLength": 1 + ,"example": "example-api-key-secret" }, "wallet_secret": { "type": "string", "format": "password", "writeOnly": true, "minLength": 1 + ,"example": "example-wallet-secret" }, "account_address": { "type": "string", "pattern": "^((0x[0-9a-fA-F]{40})|([1-9A-HJ-NP-Za-km-z]{32,44}))$" + ,"examples": ["0x742d35Cc6634C0532925a3b844Bc454e4438f44f","6s7RsvzcdXFJi1tXeDoGfSKZFzN3juVt9fTar6WEhEm2"] } }, "additionalProperties": false },src/services/cdp/mod.rs (3)
123-148: Validate Solana addresses to catch obvious config errors early.Treating any non-0x string as Solana allows malformed inputs. Light validation via base58 decode helps produce actionable errors.
} else { - // Solana address (Base58) - Ok(Address::Solana(address_str.to_string())) + // Solana address (Base58) + if bs58::decode(address_str).into_vec().is_err() { + return Err(CdpError::ConfigError("Invalid Solana address (not base58)".to_string())); + } + Ok(Address::Solana(address_str.to_string())) }
81-84: Avoid exposing config/client publicly.
CdpSignerConfigholds secrets; make fields crate-visible to reduce accidental exposure.-#[derive(Clone)] -pub struct CdpService { - pub config: CdpSignerConfig, - pub client: Client, -} +#[derive(Clone)] +pub struct CdpService { + pub(crate) config: CdpSignerConfig, + pub(crate) client: Client, +}
168-189: Error mapping is clear. Minor polish (optional).If feasible, map 401/403 HTTP statuses to
AuthenticationFailedto improve operator feedback.Also applies to: 206-228, 283-293
src/models/signer/mod.rs (1)
960-1124: Augment test coverage for edge formats.
- Add an EVM address test with 0X prefix (now supported).
- Add tests covering base64url (URL-safe) secrets to lock in the flexible decoding.
Proposed tests:
+ #[test] + fn test_valid_cdp_signer_evm_uppercase_prefix() { + let config = CdpSignerConfig { + api_key_id: "test-api-key".to_string(), + api_key_secret: SecretString::new("dGVzdC1hcGkta2V5LXNlY3JldA=="), + wallet_secret: SecretString::new("dGVzdC13YWxsZXQtc2VjcmV0"), + account_address: "0X742d35Cc6634C0532925a3b844Bc454e4438f44f".to_string(), + }; + let signer = Signer::new("cdp-signer".to_string(), SignerConfig::Cdp(config)); + assert!(signer.validate().is_ok()); + } + + #[test] + fn test_valid_cdp_signer_with_base64url_secrets() { + // "test-api-key-secret" and "test-wallet-secret" in base64url, no padding + let api_key_b64url = "dGVzdC1hcGkta2V5LXNlY3JldA"; + let wallet_b64url = "dGVzdC13YWxsZXQtc2VjcmV0"; + let config = CdpSignerConfig { + api_key_id: "test-api-key".to_string(), + api_key_secret: SecretString::new(api_key_b64url), + wallet_secret: SecretString::new(wallet_b64url), + account_address: "0x742d35Cc6634C0532925a3b844Bc454e4438f44f".to_string(), + }; + let signer = Signer::new("cdp-signer".to_string(), SignerConfig::Cdp(config)); + assert!(signer.validate().is_ok()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/openapi.json(5 hunks)src/models/signer/mod.rs(9 hunks)src/models/signer/request.rs(7 hunks)src/services/cdp/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-229)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(146-154)test_address(183-206)
src/models/signer/request.rs (4)
src/models/signer/mod.rs (2)
new(499-501)signer_type(504-506)src/services/cdp/mod.rs (3)
new(87-106)account_address(65-65)account_address(153-156)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/models/signer/config.rs (10)
new(191-193)try_from(214-255)try_from(261-266)try_from(272-284)try_from(290-305)try_from(311-329)try_from(335-354)try_from(360-409)try_from(415-439)try_from(445-467)
src/models/signer/mod.rs (6)
src/utils/base64.rs (1)
base64_decode(8-10)src/services/signer/solana/mod.rs (2)
pubkey(100-100)pubkey(116-125)src/services/signer/solana/cdp_signer.rs (2)
pubkey(61-69)new(39-45)src/models/signer/repository.rs (2)
validate(405-408)get_cdp(477-482)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)
🪛 Gitleaks (8.27.2)
src/models/signer/request.rs
[high] 891-891: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 892-892: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 911-911: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 912-912: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 929-929: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 930-930: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 960-960: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 961-961: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (7)
docs/openapi.json (2)
6480-6491: Enums updated with "cdp" — good.
SignerTypecorrectly includes "cdp". No issues.
6492-6504: Request enums updated with "cdp" — good.
SignerTypeRequestcorrectly includes "cdp". No issues.src/models/signer/request.rs (2)
131-133: Enum integration looks correct.
SignerConfigRequest::Cdpaddition is consistent with other variants.
258-263: Conversion correctly wraps secrets and forwards address.Mapping to
SecretStringand preservingaccount_addressis correct.src/services/cdp/mod.rs (1)
86-106: HTTP client with timeouts — good.Connect and total timeouts are set; solid default values.
src/models/signer/mod.rs (2)
31-35: LGTM on imports.Pulling in Pubkey and FromStr for Solana validation and base64_decode is appropriate.
327-327: CDP variant wiring looks correct.Enum variant, validator branch, accessor, and SignerType mapping are consistent.
Also applies to: 360-365, 415-422, 439-439, 494-495
| /// CDP signer configuration | ||
| #[derive(Debug, Clone, Serialize, Deserialize, Validate)] | ||
| #[validate(schema(function = "validate_cdp_config"))] | ||
| pub struct CdpSignerConfig { | ||
| #[validate(length(min = 1, message = "API Key ID cannot be empty"))] | ||
| pub api_key_id: String, | ||
| #[validate(custom( | ||
| function = "validate_secret_string", | ||
| message = "API Key Secret cannot be empty" | ||
| ))] | ||
| pub api_key_secret: SecretString, | ||
| #[validate(custom( | ||
| function = "validate_secret_string", | ||
| message = "API Wallet Secret cannot be empty" | ||
| ))] | ||
| pub wallet_secret: SecretString, | ||
| #[validate(length(min = 1, message = "Account address cannot be empty"))] | ||
| pub account_address: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
Redact CDP secrets on serialization to prevent leakage.
CdpSignerConfig derives Serialize but api_key_secret and wallet_secret are not redacted. If this struct is ever serialized (logs, DB snapshots, debug endpoints), secrets can leak.
Apply this diff to redact on serialize:
#[derive(Debug, Clone, Serialize, Deserialize, Validate)]
#[validate(schema(function = "validate_cdp_config"))]
pub struct CdpSignerConfig {
#[validate(length(min = 1, message = "API Key ID cannot be empty"))]
pub api_key_id: String,
- #[validate(custom(
+ #[serde(serialize_with = "serialize_secret_string_redacted")]
+ #[validate(custom(
function = "validate_secret_string",
message = "API Key Secret cannot be empty"
))]
pub api_key_secret: SecretString,
- #[validate(custom(
+ #[serde(serialize_with = "serialize_secret_string_redacted")]
+ #[validate(custom(
function = "validate_secret_string",
message = "API Wallet Secret cannot be empty"
))]
pub wallet_secret: SecretString,
#[validate(length(min = 1, message = "Account address cannot be empty"))]
pub account_address: String,
}Add this helper near serialize_secret_redacted:
fn serialize_secret_string_redacted<S>(_secret: &SecretString, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
serializer.serialize_str("[REDACTED]")
}There was a problem hiding this comment.
This approach is consistent with other config implementations.
There was a problem hiding this comment.
@sammccord You're absolutely right! Looking at the other signer configurations in the codebase, I can see that VaultSignerConfig, VaultTransitSignerConfig, TurnkeySignerConfig, and GoogleCloudKmsSignerServiceAccountConfig all use SecretString fields without explicit serialize_with redaction attributes. Only LocalSignerConfig uses explicit redaction, but that's for a SecretVec<u8> field, not SecretString.
This suggests that SecretString has its own built-in serialization behavior that handles redaction automatically, making the current CDP implementation consistent with the established patterns. Thanks for the clarification!
🐰✨
(\ /)
( .-. )
o_(")(")
✏️ Learnings added
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/models/signer/mod.rs:184-203
Timestamp: 2025-09-11T22:32:27.565Z
Learning: SecretString fields in signer configurations do not require explicit serialize_with redaction attributes, as SecretString handles redaction automatically during serialization. Only SecretVec<u8> fields like LocalSignerConfig's raw_key require explicit serialize_with = "serialize_secret_redacted".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| /// Custom validator for CDP signer configuration | ||
| fn validate_cdp_config(config: &CdpSignerConfig) -> Result<(), validator::ValidationError> { | ||
| // Validate api_key_secret is valid base64 | ||
| let api_key_valid = config | ||
| .api_key_secret | ||
| .as_str(|secret_str| base64_decode(secret_str).is_ok()); | ||
| if !api_key_valid { | ||
| let mut error = validator::ValidationError::new("invalid_base64_api_key_secret"); | ||
| error.message = Some("API Key Secret is not valid base64".into()); | ||
| return Err(error); | ||
| } | ||
|
|
||
| // Validate wallet_secret is valid base64 | ||
| let wallet_secret_valid = config | ||
| .wallet_secret | ||
| .as_str(|secret_str| base64_decode(secret_str).is_ok()); | ||
| if !wallet_secret_valid { | ||
| let mut error = validator::ValidationError::new("invalid_base64_wallet_secret"); | ||
| error.message = Some("Wallet Secret is not valid base64".into()); | ||
| return Err(error); | ||
| } | ||
|
|
||
| let addr = &config.account_address; | ||
|
|
||
| // Check if it's an EVM address (0x-prefixed hex) | ||
| if addr.starts_with("0x") { | ||
| if addr.len() != 42 { | ||
| let mut error = validator::ValidationError::new("invalid_evm_address_format"); | ||
| error.message = Some( | ||
| "EVM account address must be a valid 0x-prefixed 40-character hex string".into(), | ||
| ); | ||
| return Err(error); | ||
| } | ||
|
|
||
| // Check if the hex part is valid | ||
| if let Some(end) = addr.strip_prefix("0x") { | ||
| if !end.chars().all(|c| c.is_ascii_hexdigit()) { | ||
| let mut error = validator::ValidationError::new("invalid_evm_address_hex"); | ||
| error.message = Some("EVM account address contains invalid hex characters".into()); | ||
| return Err(error); | ||
| } | ||
| } | ||
| } else { | ||
| // Assume it's a Solana address - validate using Pubkey::from_str | ||
| if Pubkey::from_str(addr).is_err() { | ||
| let mut error = validator::ValidationError::new("invalid_solana_address"); | ||
| error.message = Some("Invalid Solana account address format".into()); | ||
| return Err(error); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make base64 validation robust (accept padded/unpadded and URL-safe) and accept 0X EVM prefix.
Many CDP secrets are distributed with varying base64 variants. Also, EVM addresses may use 0X. Improve resilience and reduce config-nag churn.
Apply this diff:
fn validate_cdp_config(config: &CdpSignerConfig) -> Result<(), validator::ValidationError> {
- // Validate api_key_secret is valid base64
- let api_key_valid = config
- .api_key_secret
- .as_str(|secret_str| base64_decode(secret_str).is_ok());
+ // Validate api_key_secret as base64 (accept padded/unpadded and URL-safe)
+ let api_key_valid = config.api_key_secret.as_str(|s| {
+ base64::engine::general_purpose::STANDARD
+ .decode(s)
+ .or_else(|_| base64::engine::general_purpose::STANDARD_NO_PAD.decode(s))
+ .or_else(|_| base64::engine::general_purpose::URL_SAFE.decode(s))
+ .or_else(|_| base64::engine::general_purpose::URL_SAFE_NO_PAD.decode(s))
+ .is_ok()
+ });
if !api_key_valid {
let mut error = validator::ValidationError::new("invalid_base64_api_key_secret");
error.message = Some("API Key Secret is not valid base64".into());
return Err(error);
}
- // Validate wallet_secret is valid base64
- let wallet_secret_valid = config
- .wallet_secret
- .as_str(|secret_str| base64_decode(secret_str).is_ok());
+ // Validate wallet_secret as base64 (accept padded/unpadded and URL-safe)
+ let wallet_secret_valid = config.wallet_secret.as_str(|s| {
+ base64::engine::general_purpose::STANDARD
+ .decode(s)
+ .or_else(|_| base64::engine::general_purpose::STANDARD_NO_PAD.decode(s))
+ .or_else(|_| base64::engine::general_purpose::URL_SAFE.decode(s))
+ .or_else(|_| base64::engine::general_purpose::URL_SAFE_NO_PAD.decode(s))
+ .is_ok()
+ });
if !wallet_secret_valid {
let mut error = validator::ValidationError::new("invalid_base64_wallet_secret");
error.message = Some("Wallet Secret is not valid base64".into());
return Err(error);
}
let addr = &config.account_address;
- // Check if it's an EVM address (0x-prefixed hex)
- if addr.starts_with("0x") {
+ // Check if it's an EVM address (0x/0X-prefixed hex)
+ if addr.starts_with("0x") || addr.starts_with("0X") {
if addr.len() != 42 {
let mut error = validator::ValidationError::new("invalid_evm_address_format");
error.message = Some(
"EVM account address must be a valid 0x-prefixed 40-character hex string".into(),
);
return Err(error);
}
// Check if the hex part is valid
- if let Some(end) = addr.strip_prefix("0x") {
+ if let Some(end) = addr.strip_prefix("0x").or_else(|| addr.strip_prefix("0X")) {
if !end.chars().all(|c| c.is_ascii_hexdigit()) {
let mut error = validator::ValidationError::new("invalid_evm_address_hex");
error.message = Some("EVM account address contains invalid hex characters".into());
return Err(error);
}
}
} else {
// Assume it's a Solana address - validate using Pubkey::from_str
if Pubkey::from_str(addr).is_err() {
let mut error = validator::ValidationError::new("invalid_solana_address");
error.message = Some("Invalid Solana account address format".into());
return Err(error);
}
}
Ok(())
}Also add at the top of this file (imports):
use base64::{engine::general_purpose, Engine as _};| fn test_json_deserialization_cdp_signer() { | ||
| let json = r#"{ | ||
| "id": "test-cdp-signer", | ||
| "type": "cdp", | ||
| "config": { | ||
| "api_key_id": "test-api-key-id", | ||
| "api_key_secret": "dGVzdC1hcGkta2V5LXNlY3JldA==", | ||
| "wallet_secret": "dGVzdC13YWxsZXQtc2VjcmV0", | ||
| "account_address": "0x742d35Cc6634C0532925a3b844Bc454e4438f44f" | ||
| } | ||
| }"#; | ||
|
|
||
| let result: Result<SignerCreateRequest, _> = serde_json::from_str(json); | ||
|
|
||
| assert!( | ||
| result.is_ok(), | ||
| "Failed to deserialize CDP signer: {:?}", | ||
| result.err() | ||
| ); | ||
|
|
||
| let request = result.unwrap(); | ||
| assert_eq!(request.id, Some("test-cdp-signer".to_string())); | ||
|
|
||
| match request.config { | ||
| SignerConfigRequest::Cdp(cdp_config) => { | ||
| assert_eq!(cdp_config.api_key_id, "test-api-key-id"); | ||
| assert_eq!(cdp_config.api_key_secret, "dGVzdC1hcGkta2V5LXNlY3JldA=="); | ||
| assert_eq!(cdp_config.wallet_secret, "dGVzdC13YWxsZXQtc2VjcmV0"); | ||
| assert_eq!( | ||
| cdp_config.account_address, | ||
| "0x742d35Cc6634C0532925a3b844Bc454e4438f44f" | ||
| ); | ||
| } | ||
| _ => panic!("Expected CDP config variant"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_valid_cdp_create_request() { | ||
| let request = SignerCreateRequest { | ||
| id: Some("test-cdp-signer".to_string()), | ||
| signer_type: SignerTypeRequest::Cdp, | ||
| config: SignerConfigRequest::Cdp(CdpSignerRequestConfig { | ||
| api_key_id: "test-api-key-id".to_string(), | ||
| api_key_secret: "dGVzdC1hcGkta2V5LXNlY3JldA==".to_string(), // Valid base64: "test-api-key-secret" | ||
| wallet_secret: "dGVzdC13YWxsZXQtc2VjcmV0".to_string(), // Valid base64: "test-wallet-secret" | ||
| account_address: "0x742d35Cc6634C0532925a3b844Bc454e4438f44f".to_string(), | ||
| }), | ||
| }; | ||
|
|
||
| let result = Signer::try_from(request); | ||
| assert!(result.is_ok()); | ||
|
|
||
| let signer = result.unwrap(); | ||
| assert_eq!(signer.id, "test-cdp-signer"); | ||
| assert_eq!(signer.signer_type(), SignerType::Cdp); | ||
|
|
||
| if let Some(cdp_config) = signer.config.get_cdp() { | ||
| assert_eq!(cdp_config.api_key_id, "test-api-key-id"); | ||
| assert_eq!( | ||
| cdp_config.account_address, | ||
| "0x742d35Cc6634C0532925a3b844Bc454e4438f44f" | ||
| ); | ||
| } else { | ||
| panic!("Expected CDP config"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_invalid_cdp_empty_api_key_id() { | ||
| let request = SignerCreateRequest { | ||
| id: Some("test-signer".to_string()), | ||
| signer_type: SignerTypeRequest::Cdp, | ||
| config: SignerConfigRequest::Cdp(CdpSignerRequestConfig { | ||
| api_key_id: "".to_string(), // Empty | ||
| api_key_secret: "dGVzdC1hcGkta2V5LXNlY3JldA==".to_string(), // Valid base64: "test-api-key-secret" | ||
| wallet_secret: "dGVzdC13YWxsZXQtc2VjcmV0".to_string(), // Valid base64: "test-wallet-secret" | ||
| account_address: "0x742d35Cc6634C0532925a3b844Bc454e4438f44f".to_string(), | ||
| }), | ||
| }; | ||
|
|
||
| let result = Signer::try_from(request); | ||
| assert!(result.is_err()); | ||
| if let Err(ApiError::BadRequest(msg)) = result { | ||
| assert!(msg.contains("API Key ID cannot be empty")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix gitleaks false positives in tests (generic API key).
Static analysis flags the base64-like literals as secrets. Use unmistakably fake placeholders to avoid pipeline blocks.
- "api_key_secret": "dGVzdC1hcGkta2V5LXNlY3JldA==",
- "wallet_secret": "dGVzdC13YWxsZXQtc2VjcmV0",
+ "api_key_secret": "example-api-key-secret",
+ "wallet_secret": "example-wallet-secret",
@@
- api_key_secret: "dGVzdC1hcGkta2V5LXNlY3JldA==".to_string(), // Valid base64: "test-api-key-secret"
- wallet_secret: "dGVzdC13YWxsZXQtc2VjcmV0".to_string(), // Valid base64: "test-wallet-secret"
+ api_key_secret: "example-api-key-secret".to_string(),
+ wallet_secret: "example-wallet-secret".to_string(),Alternatively, add a repo-level gitleaks allowlist for these test files, but changing literals is simpler and safer.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.27.2)
[high] 891-891: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 892-892: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 911-911: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 912-912: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 929-929: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 930-930: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 960-960: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 961-961: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/models/signer/request.rs around lines 885 to 971, test literals that look
like base64 are triggering gitleaks; replace those values with unmistakably fake
placeholders (e.g., "TEST_API_KEY_SECRET_PLACEHOLDER" and
"TEST_WALLET_SECRET_PLACEHOLDER") in the JSON sample and all SignerCreateRequest
instances, and update the corresponding assertions that compare these fields to
expect the new placeholder strings; ensure comments that claim "Valid base64"
are removed or updated so tests and expectations remain consistent.
There was a problem hiding this comment.
This recommendation would fail the base64 validation tests, and is not necessary as these are fake secrets for testing.
There was a problem hiding this comment.
@sammccord You're absolutely right! I apologize for not considering that the base64 format is likely required for proper validation testing in the CDP signer configuration. Changing those to plaintext would indeed break any base64 decoding or format validation logic.
Since these are clearly test fixtures with obvious fake values, the gitleaks warnings are acceptable false positives. Thanks for the clarification - maintaining functional test coverage is more important than avoiding static analysis noise in this case.
✏️ Learnings added
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/models/signer/request.rs:885-971
Timestamp: 2025-09-11T22:35:15.532Z
Learning: In test files for OpenZeppelin Relayer, base64-encoded test secrets (like "dGVzdC1hcGkta2V5LXNlY3JldA==" which decodes to "test-api-key-secret") should be kept in their base64 format to properly test validation logic, even if they trigger gitleaks false positives. These test fixtures with obviously fake values are acceptable exceptions to secret detection rules.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let address = self.get_account_address(); | ||
| let encoded_message = str::from_utf8(message) | ||
| .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))? | ||
| .to_string(); | ||
|
|
||
| let message_body = types::SignSolanaMessageBody::builder().message(encoded_message); | ||
|
|
There was a problem hiding this comment.
Don’t force UTF-8 for Solana messages; base58-encode bytes.
Solana messages are arbitrary bytes. Forcing UTF-8 will break binary payloads. Encode the raw bytes as base58 before sending to CDP.
- let encoded_message = str::from_utf8(message)
- .map_err(|e| CdpError::SerializationError(format!("Invalid UTF-8 message: {}", e)))?
- .to_string();
+ let encoded_message = bs58::encode(message).into_string();Also applies to: 255-258
🤖 Prompt for AI Agents
In src/services/cdp/mod.rs around lines 236-242 (and also update the same
pattern at 255-258), don't convert the Solana message bytes to UTF-8; instead
base58-encode the raw byte slice and pass that string to the
SignSolanaMessageBody builder. Replace the str::from_utf8(...) -> to_string()
flow with bs58::encode(message).into_string(), remove the UTF-8 error mapping,
add use of the bs58 crate (add dependency if missing), and update both
occurrences so binary payloads are preserved by encoding bytes as base58 before
sending to CDP.
There was a problem hiding this comment.
This function is the solana equivalent of signEvmMessage in the CDP API, which doesn't require a base58 encoded value.
There was a problem hiding this comment.
@sammccord, thank you for the clarification! I understand now that the CDP API's Solana message signing endpoint expects a string format similar to the EVM equivalent, rather than base58-encoded bytes. This makes sense given the API consistency between EVM and Solana message signing operations.
I appreciate you providing this context about the CDP API contract - it's helpful to understand that the UTF-8 conversion aligns with the expected API interface.
✏️ Learnings added
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/cdp/mod.rs:236-242
Timestamp: 2025-09-11T22:37:15.543Z
Learning: The CDP API's sign_solana_message function is equivalent to signEvmMessage and expects a string parameter, not base58-encoded bytes. UTF-8 conversion is appropriate for this API contract.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Just double checked and you are right, I have not copied correctly wallet secret. All good! Thanks! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
=======================================
+ Coverage 92.9% 93.0% +0.1%
=======================================
Files 219 222 +3
Lines 74884 77105 +2221
=======================================
+ Hits 69600 71757 +2157
- Misses 5284 5348 +64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for your work!
LGTM
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/models/signer/repository.rs (1)
476-483: Docstring nit: Option, not error.The method returns Option. Update wording to avoid confusion.
Apply:
- /// Get CDP signer config, returns error if not a CDP signer + /// Get CDP signer config; returns None if not a CDP signersrc/models/signer/config.rs (1)
49-57: CDP file-config schema looks good.deny_unknown_fields is helpful. Consider brief field docs to aid users in config files.
src/services/cdp/mod.rs (4)
114-121: Be robust to '0X' and stray whitespace in address checks.Minor ergonomics: accept uppercase hex prefix and trim.
Apply:
- fn is_evm_address(&self) -> bool { - self.config.account_address.starts_with("0x") - } + fn is_evm_address(&self) -> bool { + let a = self.config.account_address.trim(); + a.starts_with("0x") || a.starts_with("0X") + } @@ - fn is_solana_address(&self) -> bool { - !self.config.account_address.starts_with("0x") - } + fn is_solana_address(&self) -> bool { + let a = self.config.account_address.trim(); + !(a.starts_with("0x") || a.starts_with("0X")) + }
123-148: Normalize EVM prefix in parser.Handle uppercase 0X and incidental whitespace before decoding.
Apply:
- fn address_from_string(&self, address_str: &str) -> Result<Address, CdpError> { - if address_str.starts_with("0x") { + fn address_from_string(&self, address_str: &str) -> Result<Address, CdpError> { + let addr = address_str.trim(); + if addr.starts_with("0x") || addr.starts_with("0X") { // EVM address (hex) - let hex_str = address_str.strip_prefix("0x").unwrap(); + let hex_str = addr.strip_prefix("0x").or_else(|| addr.strip_prefix("0X")).unwrap();
321-328: Test client: optional parity with production timeouts.Not required, but mirroring timeouts here can make flaky tests fail fast when endpoints aren’t reachable.
560-647: Great EVM address parsing tests. Add an uppercase 0X case?To guard the suggested prefix tweak, add a test with 0X… prefix.
Happy to draft the test if you adopt the normalization change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/models/signer/config.rs(6 hunks)src/models/signer/repository.rs(8 hunks)src/services/cdp/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-11T22:37:15.562Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/cdp/mod.rs:236-242
Timestamp: 2025-09-11T22:37:15.562Z
Learning: The CDP API's sign_solana_message function is equivalent to signEvmMessage and expects a string parameter, not base58-encoded bytes. UTF-8 conversion is appropriate for this API contract.
Applied to files:
src/services/cdp/mod.rs
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Applied to files:
src/services/cdp/mod.rs
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Applied to files:
src/services/cdp/mod.rs
📚 Learning: 2025-09-11T22:32:27.610Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/models/signer/mod.rs:184-203
Timestamp: 2025-09-11T22:32:27.610Z
Learning: SecretString fields in signer configurations do not require explicit serialize_with redaction attributes, as SecretString handles redaction automatically during serialization. Only SecretVec<u8> fields like LocalSignerConfig's raw_key require explicit serialize_with = "serialize_secret_redacted".
Applied to files:
src/models/signer/repository.rssrc/models/signer/config.rs
🧬 Code graph analysis (3)
src/services/cdp/mod.rs (4)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-229)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(146-154)test_address(183-206)src/services/signer/evm/mod.rs (1)
address(76-85)src/services/signer/solana/mod.rs (1)
address(66-75)
src/models/signer/repository.rs (5)
src/models/signer/mod.rs (3)
from(548-558)get_cdp(416-421)new(499-501)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)src/models/secret_string.rs (1)
to_str(72-78)
src/models/signer/config.rs (4)
src/models/signer/request.rs (2)
try_from(211-276)try_from(282-323)src/services/cdp/mod.rs (3)
new(87-106)account_address(65-65)account_address(153-156)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
🔇 Additional comments (18)
src/models/signer/repository.rs (9)
16-19: Imports look correct and scoped.Brings CdpSignerConfig into storage layer cleanly; no redundancy observed.
44-44: Enum variant wiring is correct.SignerConfigStorage::Cdp added without disturbing existing order; serde derives intact.
131-146: Secret handling in storage struct is consistent with repo patterns.Using serialize_secret_string/deserialize_secret_string matches other storage models (Vault, GCP). Good to keep storage format consistent with existing data.
303-312: Bidirectional conversions are lossless.All fields (including SecretString) are transferred by move; no redaction/log risk introduced.
Also applies to: 314-323
421-422: Domain→storage mapping includes CDP arm.No gaps; ordering preserved.
437-438: Storage→domain mapping includes CDP arm.Round-trip completeness maintained.
576-603: CDP storage round-trip test reads well.Comparing through to_str() on SecretString is fine in tests; Zeroizing ensures safe drop.
605-621: get_cdp() positive case covered.Good assertion on api_key_id.
622-633: get_cdp() negative case covered.Behavior is clear and correct.
src/models/signer/config.rs (5)
15-17: Import additions are correct.Brings CdpSignerConfig into the file-config layer appropriately.
155-161: Enum variant wiring is correct.Cdp added with proper serde tagging and placement.
287-306: Secret resolution and mapping are sound.Errors map to InvalidFormat with clear messages; domain validation will catch empty/invalid addresses.
If you want early feedback at file-parse time, we can trim() api_key_id/account_address here before constructing the domain config. Want a quick patch?
426-428: End-to-end file-config→domain wiring includes CDP.Validation runs later via Signer::validate, maintaining consistency with other signers.
673-736: Tests cover success and both secret error paths.Clear assertions and messages.
src/services/cdp/mod.rs (4)
98-105: HTTP client construction with timeouts looks good.connect_timeout and total timeout set; propagated via middleware.
236-241: UTF-8 conversion for Solana message is OK per CDP API contract.Matches the learned behavior that CDP’s Solana sign-message expects a string, not base58 bytes.
180-188: Hex signature parsing is correct and resilient to missing 0x.Good error surface.
255-258: Accurate base58 decoding and error text.Comment and error strings align with bs58 usage.
fix(cdp): Solana signing chore: Address coderabbit feedback chore: Fix signers table formatting chore: Address nits chore: Address nits
sammccord
force-pushed
the
sammccord/implement-cdp-signing
branch
from
59a5476 to
41d7344
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
examples/solana-cdp-signer/README.md (2)
57-60: Add API key ID to env and reference it in config.json.Users are told to save API Key ID but it’s not used via env; prevents accidental hardcoding.
```env +CDP_API_KEY_ID=your_api_key_id CDP_API_KEY_SECRET=your_api_key_secret CDP_WALLET_SECRET=your_wallet_secret```diff { "signers": [ { "id": "cdp-signer-solana", "type": "cdp", "config": { - "api_key_id": "YOUR_API_KEY_ID", + "api_key_id": { + "type": "env", + "value": "CDP_API_KEY_ID" + }, "api_key_secret": { "type": "env", "value": "CDP_API_KEY_SECRET" }, "wallet_secret": { "type": "env", "value": "CDP_WALLET_SECRET" }, "account_address": "YOUR_SOLANA_ACCOUNT_ADDRESS" } } ] }Also applies to: 66-87
129-131: Ensure docker compose loads the example’s .env.Running from repo root with -f won’t auto-load examples/solana-cdp-signer/.env.
-```bash -docker compose -f examples/solana-cdp-signer/docker-compose.yaml up -``` +```bash +cd examples/solana-cdp-signer +docker compose up +# alternatively: +# docker compose --env-file ./.env -f ./docker-compose.yaml up +```src/models/signer/request.rs (1)
112-120: Do not derive Debug for secret-bearing request structs; zeroize on drop.Avoid accidental secret logging and ensure memory is wiped.
Apply:
-/// CDP signer configuration for API requests -#[derive(Debug, Serialize, Deserialize, ToSchema, Zeroize)] +/// CDP signer configuration for API requests +#[derive(Serialize, Deserialize, ToSchema, Zeroize)] +#[zeroize(drop)] pub struct CdpSignerRequestConfig { pub api_key_id: String, pub api_key_secret: String, pub wallet_secret: String, pub account_address: String, }Consider applying the same pattern to other request structs that contain secrets (e.g., Turnkey, Vault).
🧹 Nitpick comments (13)
examples/solana-cdp-signer/README.md (1)
139-142: Use standard Authorization header casing.Headers are case-insensitive, but “Authorization” is conventional and avoids confusion.
curl -X GET http://localhost:8080/api/v1/relayers \ -H "Content-Type: application/json" \ - -H "AUTHORIZATION: Bearer $API_KEY" + -H "Authorization: Bearer $API_KEY"src/services/signer/solana/cdp_signer.rs (1)
106-141: Match only required signers and avoid possible index mismatch; also fetch CDP pubkey once.Search within the first num_required_signatures to align with the signatures vector, and avoid a second account_address() call.
- // Get the CDP signer's address to find the correct signature index - let cdp_address = self - .cdp_service - .account_address() - .await - .map_err(SignerError::CdpError)?; - - let cdp_pubkey = match cdp_address { + // Resolve CDP signer pubkey once + let cdp_pubkey = match self + .cdp_service + .account_address() + .await + .map_err(SignerError::CdpError)? + { crate::models::Address::Solana(addr) => Pubkey::from_str(&addr) .map_err(|e| SignerError::SigningError(format!("Invalid CDP pubkey: {}", e)))?, _ => { return Err(SignerError::SigningError( "CDP address is not a Solana address".to_string(), )) } }; - // Find the signature index for the CDP signer's pubkey - let signer_index = signed_transaction - .message - .account_keys - .iter() - .position(|key| *key == cdp_pubkey) - .ok_or_else(|| { - SignerError::SigningError("CDP pubkey not found in transaction signers".to_string()) - })?; - - // Extract the signature at the correct index - if signer_index >= signed_transaction.signatures.len() { - return Err(SignerError::SigningError( - "Signature index out of bounds".to_string(), - )); - } + // Find the signature index among required signers only + let required = signed_transaction.message.header.num_required_signatures as usize; + let signer_index = signed_transaction.message.account_keys[..required] + .iter() + .position(|key| *key == cdp_pubkey) + .ok_or_else(|| { + SignerError::SigningError( + "CDP pubkey not found among required signers".to_string(), + ) + })?; Ok(signed_transaction.signatures[signer_index])src/services/signer/evm/mod.rs (1)
48-49: Remove unused import to avoid warnings.
CdpServiceisn't referenced here.Apply:
- AwsKmsService, CdpService, GoogleCloudKmsService, TurnkeyServiceTrait, + AwsKmsService, GoogleCloudKmsService, TurnkeyServiceTrait,src/services/cdp/mod.rs (1)
123-148: Validate Solana address format (optional).Currently any non-0x string is accepted. A quick base58 length check reduces misconfigurations.
Apply:
- } else { - // Solana address (Base58) - Ok(Address::Solana(address_str.to_string())) - } + } else { + // Solana address (Base58-encoded 32-byte pubkey) + match bs58::decode(address_str).into_vec() { + Ok(bytes) if bytes.len() == 32 => Ok(Address::Solana(address_str.to_string())), + _ => Err(CdpError::ConfigError( + "Invalid Solana address: must be base58-encoded 32-byte public key".into(), + )), + } + }src/models/signer/repository.rs (1)
476-482: Docstring nit: “returns error” → “returns None”.Method returns
Option, not an error.Apply:
- /// Get CDP signer config, returns error if not a CDP signer + /// Get CDP signer config, returns None if not a CDP signer pub fn get_cdp(&self) -> Option<&CdpSignerConfigStorage> {src/services/signer/evm/cdp_signer.rs (3)
8-10: Clarify EIP-191 note in docs.CDP applies EIP-191 semantics; this signer forwards the message.
Apply:
-//! - Message signing with EIP-191 prefixing +//! - Message signing (CDP applies EIP-191 semantics)
16-24: Prune unused imports.Removes noise and warning potential.
Apply:
-use std::str::FromStr; - use alloy::{ - consensus::{SignableTransaction, TxEip1559, TxLegacy}, - primitives::{eip191_hash_message, keccak256}, + consensus::{SignableTransaction, TxEip1559, TxLegacy}, }; use async_trait::async_trait; -use log::{debug, info}; @@ - Address, CdpSignerConfig, EvmTransactionDataSignature, EvmTransactionDataTrait, - NetworkTransactionData, SignerError, SignerRepoModel, + Address, CdpSignerConfig, EvmTransactionDataSignature, EvmTransactionDataTrait, + NetworkTransactionData, SignerError,Also applies to: 25-25, 31-35
181-189: Typed data unimplemented guard is appropriate.If/when CDP adds support, we can wire it similarly.
Want me to scaffold EIP-712 support behind a feature flag and add tests?
src/models/signer/mod.rs (5)
31-35: Broaden base64 decoding support (prep for resilient validation).To support URL-safe and unpadded variants in the validator (next comment), import the base64 engines.
use crate::{constants::ID_REGEX, models::SecretString, utils::base64_decode}; +use base64::{engine::general_purpose, Engine as _}; use secrets::SecretVec; use serde::{Deserialize, Serialize, Serializer}; use solana_sdk::pubkey::Pubkey; use std::str::FromStr;
184-203: Clarify accepted address formats in docs (minor).Add a short field doc that it accepts either EVM (0x/0X) or Solana (base58) to reduce misconfiguration.
pub struct CdpSignerConfig { @@ - #[validate(length(min = 1, message = "Account address cannot be empty"))] + /// Either an EVM 0x/0X-prefixed 40-hex address or a Solana base58 public key. + #[validate(length(min = 1, message = "Account address cannot be empty"))] pub account_address: String,
265-317: Make CDP validation resilient: accept 0X prefix and URL-safe/unpadded base64.Reduces config friction and matches how secrets are often distributed.
fn validate_cdp_config(config: &CdpSignerConfig) -> Result<(), validator::ValidationError> { - // Validate api_key_secret is valid base64 - let api_key_valid = config - .api_key_secret - .as_str(|secret_str| base64_decode(secret_str).is_ok()); + // Validate api_key_secret as base64 (accept padded/unpadded and URL-safe) + let api_key_valid = config.api_key_secret.as_str(|s| { + base64_decode(s).is_ok() + || general_purpose::STANDARD_NO_PAD.decode(s).is_ok() + || general_purpose::URL_SAFE.decode(s).is_ok() + || general_purpose::URL_SAFE_NO_PAD.decode(s).is_ok() + }); if !api_key_valid { let mut error = validator::ValidationError::new("invalid_base64_api_key_secret"); error.message = Some("API Key Secret is not valid base64".into()); return Err(error); } - // Validate wallet_secret is valid base64 - let wallet_secret_valid = config - .wallet_secret - .as_str(|secret_str| base64_decode(secret_str).is_ok()); + // Validate wallet_secret as base64 (accept padded/unpadded and URL-safe) + let wallet_secret_valid = config.wallet_secret.as_str(|s| { + base64_decode(s).is_ok() + || general_purpose::STANDARD_NO_PAD.decode(s).is_ok() + || general_purpose::URL_SAFE.decode(s).is_ok() + || general_purpose::URL_SAFE_NO_PAD.decode(s).is_ok() + }); if !wallet_secret_valid { let mut error = validator::ValidationError::new("invalid_base64_wallet_secret"); error.message = Some("Wallet Secret is not valid base64".into()); return Err(error); } let addr = &config.account_address; - // Check if it's an EVM address (0x-prefixed hex) - if addr.starts_with("0x") { + // Check if it's an EVM address (0x/0X-prefixed hex) + if addr.starts_with("0x") || addr.starts_with("0X") { if addr.len() != 42 { let mut error = validator::ValidationError::new("invalid_evm_address_format"); error.message = Some( "EVM account address must be a valid 0x-prefixed 40-character hex string".into(), ); return Err(error); } - // Check if the hex part is valid - if let Some(end) = addr.strip_prefix("0x") { + // Check if the hex part is valid + if let Some(end) = addr.strip_prefix("0x").or_else(|| addr.strip_prefix("0X")) { if !end.chars().all(|c| c.is_ascii_hexdigit()) { let mut error = validator::ValidationError::new("invalid_evm_address_hex"); error.message = Some("EVM account address contains invalid hex characters".into()); return Err(error); } } } else { // Assume it's a Solana address - validate using Pubkey::from_str if Pubkey::from_str(addr).is_err() { let mut error = validator::ValidationError::new("invalid_solana_address"); error.message = Some("Invalid Solana account address format".into()); return Err(error); } } Ok(()) }Optionally, add an EIP-55 checksum advisory (non-blocking) if you prefer stricter EVM address hygiene.
494-495: Add (de)serialization assertions for "cdp".Extend tests to ensure serde round-trips the new variant.
Add alongside existing serialization tests:
#[test] fn test_signer_type_serialization_cdp() { use serde_json::{from_str, to_string}; assert_eq!(to_string(&SignerType::Cdp).unwrap(), "\"cdp\""); assert_eq!(from_str::<SignerType>("\"cdp\"").unwrap(), SignerType::Cdp); }
960-1124: Top up coverage: uppercase 0X EVM and CDP accessor.Add tests to lock in the relaxed EVM prefix handling and the get_cdp() accessor.
#[test] fn test_valid_cdp_signer_with_uppercase_0x_address() { let config = CdpSignerConfig { api_key_id: "test-api-key".to_string(), api_key_secret: SecretString::new("dGVzdC1hcGkta2V5LXNlY3JldA=="), // "test-api-key-secret" wallet_secret: SecretString::new("dGVzdC13YWxsZXQtc2VjcmV0"), // "test-wallet-secret" account_address: "0X742d35Cc6634C0532925a3b844Bc454e4438f44f".to_string(), }; let signer = Signer::new("cdp-signer".to_string(), SignerConfig::Cdp(config)); assert!(signer.validate().is_ok()); } #[test] fn test_cdp_config_accessor() { let cfg = CdpSignerConfig { api_key_id: "test-api-key".to_string(), api_key_secret: SecretString::new("dGVzdC1hcGkta2V5LXNlY3JldA=="), wallet_secret: SecretString::new("dGVzdC13YWxsZXQtc2VjcmV0"), account_address: "6s7RsvzcdXFJi1tXeDoGfSKZFzN3juVt9fTar6WEhEm2".to_string(), }; let scfg = SignerConfig::Cdp(cfg); assert!(scfg.get_cdp().is_some()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(1 hunks)README.md(1 hunks)docs/modules/ROOT/pages/configuration.adoc(1 hunks)docs/modules/ROOT/pages/evm.adoc(2 hunks)docs/modules/ROOT/pages/index.adoc(1 hunks)docs/modules/ROOT/pages/signers.adoc(3 hunks)docs/modules/ROOT/pages/solana.adoc(1 hunks)docs/openapi.json(5 hunks)examples/evm-cdp-signer/.env.example(1 hunks)examples/evm-cdp-signer/README.md(1 hunks)examples/evm-cdp-signer/config/config.json(1 hunks)examples/evm-cdp-signer/docker-compose.yaml(1 hunks)examples/solana-cdp-signer/.env.example(1 hunks)examples/solana-cdp-signer/README.md(1 hunks)examples/solana-cdp-signer/config/config.json(1 hunks)examples/solana-cdp-signer/docker-compose.yaml(1 hunks)src/models/error/signer.rs(2 hunks)src/models/signer/config.rs(6 hunks)src/models/signer/mod.rs(9 hunks)src/models/signer/repository.rs(8 hunks)src/models/signer/request.rs(7 hunks)src/models/signer/response.rs(3 hunks)src/services/cdp/mod.rs(1 hunks)src/services/mod.rs(1 hunks)src/services/signer/evm/cdp_signer.rs(1 hunks)src/services/signer/evm/mod.rs(11 hunks)src/services/signer/solana/cdp_signer.rs(1 hunks)src/services/signer/solana/mod.rs(11 hunks)src/services/signer/stellar/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/evm-cdp-signer/.env.example
🚧 Files skipped from review as they are similar to previous changes (17)
- Cargo.toml
- docs/modules/ROOT/pages/index.adoc
- src/models/signer/response.rs
- examples/solana-cdp-signer/.env.example
- src/services/signer/stellar/mod.rs
- docs/modules/ROOT/pages/evm.adoc
- examples/solana-cdp-signer/config/config.json
- src/models/error/signer.rs
- README.md
- docs/modules/ROOT/pages/solana.adoc
- docs/modules/ROOT/pages/configuration.adoc
- src/models/signer/config.rs
- docs/modules/ROOT/pages/signers.adoc
- examples/evm-cdp-signer/config/config.json
- examples/solana-cdp-signer/docker-compose.yaml
- docs/openapi.json
- examples/evm-cdp-signer/README.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: The CDP SDK in the OpenZeppelin Relayer consistently returns decoded bytes from signing operations, not base64 strings. The codebase intentionally uses EncodedSerializedTransaction for architectural consistency even when it requires encoding/decoding round-trips.
Applied to files:
examples/solana-cdp-signer/README.mdsrc/services/cdp/mod.rssrc/services/signer/solana/cdp_signer.rssrc/models/signer/mod.rs
📚 Learning: 2025-09-11T22:37:15.562Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/cdp/mod.rs:236-242
Timestamp: 2025-09-11T22:37:15.562Z
Learning: The CDP API's sign_solana_message function is equivalent to signEvmMessage and expects a string parameter, not base58-encoded bytes. UTF-8 conversion is appropriate for this API contract.
Applied to files:
src/services/cdp/mod.rssrc/services/signer/solana/cdp_signer.rs
📚 Learning: 2025-09-05T04:46:17.576Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/services/signer/solana/cdp_signer.rs:100-109
Timestamp: 2025-09-05T04:46:17.576Z
Learning: In the OpenZeppelin Relayer CDP signer tests, the mocks simulate the CDP service layer (which returns bytes) rather than the external CDP API layer (which returns base64), which is why tests use raw bytes.
Applied to files:
src/services/cdp/mod.rssrc/services/signer/solana/cdp_signer.rs
📚 Learning: 2025-09-11T22:32:27.610Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/models/signer/mod.rs:184-203
Timestamp: 2025-09-11T22:32:27.610Z
Learning: SecretString fields in signer configurations do not require explicit serialize_with redaction attributes, as SecretString handles redaction automatically during serialization. Only SecretVec<u8> fields like LocalSignerConfig's raw_key require explicit serialize_with = "serialize_secret_redacted".
Applied to files:
src/models/signer/request.rssrc/models/signer/repository.rssrc/models/signer/mod.rs
📚 Learning: 2025-09-11T22:35:15.583Z
Learnt from: sammccord
PR: OpenZeppelin/openzeppelin-relayer#457
File: src/models/signer/request.rs:885-971
Timestamp: 2025-09-11T22:35:15.583Z
Learning: In test files for OpenZeppelin Relayer, base64-encoded test secrets (like "dGVzdC1hcGkta2V5LXNlY3JldA==" which decodes to "test-api-key-secret") should be kept in their base64 format to properly test validation logic, even if they trigger gitleaks false positives. These test fixtures with obviously fake values are acceptable exceptions to secret detection rules.
Applied to files:
src/models/signer/request.rssrc/models/signer/mod.rs
🧬 Code graph analysis (8)
src/services/cdp/mod.rs (2)
src/services/signer/evm/cdp_signer.rs (3)
new(49-54)address(70-78)test_address(202-229)src/services/signer/solana/cdp_signer.rs (3)
new(39-45)address(146-154)test_address(183-206)
src/services/signer/solana/mod.rs (3)
src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)
src/services/signer/solana/cdp_signer.rs (4)
src/services/signer/solana/mod.rs (6)
pubkey(100-100)pubkey(116-125)address(66-75)sign(111-111)sign(127-136)sign_transaction(77-89)src/models/signer/config.rs (10)
new(191-193)try_from(214-255)try_from(261-266)try_from(272-284)try_from(290-305)try_from(311-329)try_from(335-354)try_from(360-409)try_from(415-439)try_from(445-467)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (5)
new(49-54)new_with_service(59-61)new_for_testing(63-65)address(70-78)sign_transaction(80-146)
src/models/signer/request.rs (3)
src/models/signer/config.rs (10)
new(191-193)try_from(214-255)try_from(261-266)try_from(272-284)try_from(290-305)try_from(311-329)try_from(335-354)try_from(360-409)try_from(415-439)try_from(445-467)src/services/cdp/mod.rs (3)
new(87-106)account_address(65-65)account_address(153-156)src/models/signer/mod.rs (2)
new(499-501)signer_type(504-506)
src/models/signer/repository.rs (5)
src/models/signer/mod.rs (3)
from(548-558)get_cdp(416-421)new(499-501)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)src/services/signer/solana/cdp_signer.rs (1)
new(39-45)src/models/secret_string.rs (1)
to_str(72-78)
src/models/signer/mod.rs (5)
src/utils/base64.rs (1)
base64_decode(8-10)src/services/signer/solana/cdp_signer.rs (2)
pubkey(61-69)new(39-45)src/services/signer/solana/mod.rs (2)
pubkey(100-100)pubkey(116-125)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)
src/services/signer/evm/cdp_signer.rs (4)
src/models/signer/config.rs (10)
new(191-193)try_from(214-255)try_from(261-266)try_from(272-284)try_from(290-305)try_from(311-329)try_from(335-354)try_from(360-409)try_from(415-439)try_from(445-467)src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/solana/cdp_signer.rs (5)
new(39-45)new_with_service(50-52)new_for_testing(54-56)address(146-154)sign_transaction(156-169)src/services/signer/evm/mod.rs (6)
address(76-85)sign_transaction(87-99)sign_data(56-56)sign_data(104-113)sign_typed_data(59-62)sign_typed_data(115-127)
src/services/signer/evm/mod.rs (2)
src/services/cdp/mod.rs (1)
new(87-106)src/services/signer/evm/cdp_signer.rs (1)
new(49-54)
🪛 Gitleaks (8.27.2)
src/models/signer/request.rs
[high] 891-891: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 892-892: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 911-911: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 912-912: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 929-929: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 930-930: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 960-960: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 961-961: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (21)
src/services/mod.rs (1)
29-31: LGTM: clean re-export for CDP services.Module wiring looks correct and minimal.
src/services/signer/solana/mod.rs (1)
30-32: LGTM: CDP signer is cleanly integrated across enum, trait impls, and factory.Variant routing and factory construction look correct; errors are mapped appropriately.
Also applies to: 60-60, 72-72, 86-86, 122-122, 133-133, 185-190
src/services/signer/evm/mod.rs (5)
16-16: Wire-up for CDP signer looks correct.Module declaration and import enable the new EVM CDP signer without side effects.
Also applies to: 22-22
69-72: Enum integration LGTM.New
EvmSigner::Cdpvariant is consistently handled across methods.
81-84: Method dispatch for CDP path is correct.Address, tx, data, and typed-data routes all delegate appropriately.
Also applies to: 95-98, 109-112, 123-126
173-178: Factory branch for CDP is sound.Error mapping is clear and consistent with other providers.
339-356: New CDP tests cover creation and address resolution.Good sanity coverage without external calls.
Also applies to: 416-437
src/models/signer/request.rs (2)
258-264: Conversion to domain config is correct.Secrets wrapped into
SecretStringand validated downstream.
299-304: Type/Config matching includes CDP and is consistent.Prevents mismatches early.
src/services/cdp/mod.rs (3)
98-105: HTTP client hard timeouts added — good.Prevents indefinite hangs on external calls.
230-261: Solana message signing path is consistent with CDP contract.UTF-8 conversion and base58 signature decoding are aligned.
285-293: Base64 decoding for signed Solana transactions is correct.Clear error reporting on decode failures.
src/models/signer/repository.rs (3)
131-146: CDP storage model wiring is correct.Secrets use the existing (de)serialization helpers; conversions are symmetric.
303-323: Enum conversions include CDP and look complete.Round-trip conversions preserve fields.
Also applies to: 411-440
576-633: Storage tests for CDP are solid.Covers conversion and accessor behavior.
src/services/signer/evm/cdp_signer.rs (2)
86-94: Tx encoding/signing/decoding flow LGTM.Handles legacy and EIP-1559 correctly; yParity normalization for 27/28 → 0/1 is defensive and fine.
Also applies to: 102-146
149-179: Message signing path is straightforward; length check is good.Returns standard r/s/v and full hex.
src/models/signer/mod.rs (4)
327-327: LGTM: added Cdp variant to SignerConfig.
360-365: LGTM: CDP branch wired into validation flow with consistent error formatting.
415-421: LGTM: get_cdp accessor mirrors other getters.
439-439: LGTM: SignerType mapping includes Cdp.
| security_opt: | ||
| - no-new-privileges |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Set no-new-privileges correctly in Compose.
security_opt expects key=value; use no-new-privileges:true. Current form may be ignored by Docker.
security_opt:
- - no-new-privileges
+ - no-new-privileges:true security_opt:
- - no-new-privileges
+ - no-new-privileges:trueRun to confirm no remaining bare entries:
Also applies to: 32-33
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*-\s*no-new-privileges\s*$' examples/evm-cdp-signer/docker-compose.yaml || echo "OK: all set with :true"Length of output: 57
Set no-new-privileges correctly in Compose.
security_opt expects key=value; change to no-new-privileges:true — bare entries may be ignored.
File: examples/evm-cdp-signer/docker-compose.yaml — lines 18, 33
security_opt:
- - no-new-privileges
+ - no-new-privileges:true📝 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.
| security_opt: | |
| - no-new-privileges | |
| security_opt: | |
| - no-new-privileges:true |
🤖 Prompt for AI Agents
In examples/evm-cdp-signer/docker-compose.yaml around lines 17-18 (and also
update line 33), the security_opt entry uses a bare value which may be ignored;
replace the bare entry with the key=value form expected by Compose (e.g.
no-new-privileges:true) so the option is applied correctly.
3 participants
Summary
This PR introduces CDP (Coinbase Developer Platform) signer implementations for both EVM and Solana networks, enabling secure remote signing through Coinbase's infrastructure.
Implementation Details
API Layer Updates
Testing Process
Tested the new
CdpSignerimplementations by going through the new examples;evm-cdp-signer, andsolana-cdp-signer.Checklist
Summary by CodeRabbit
New Features
Documentation
Examples
Dependencies
Tests