feat: Solana send transaction endpoint #521
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces a comprehensive refactoring of Solana transaction handling and signer architecture. Key updates include upgrading Solana dependencies to interface-based crates (v3.0.0), restructuring domain models with typed request/response containers, introducing a new SolanaSignTrait for low-level signing operations, adding instruction-based transaction building, and expanding validation and error classification logic throughout the stack. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Relayer as Relayer API
participant TxService as Transaction Service
participant SignerService as Signer Service
participant OnChain as Solana Chain
participant DB as Database
Client->>Relayer: sign_and_send_transaction(request)
Relayer->>TxService: validate & prepare
par Concurrent Validations
TxService->>SignerService: validate_blockhash
TxService->>OnChain: simulate_transaction
TxService->>TxService: validate_token_transfers
TxService->>TxService: validate_lamports_transfers
end
TxService->>SignerService: sign_transaction
SignerService->>OnChain: submit transaction
OnChain-->>SignerService: signature
SignerService->>DB: persist transaction
SignerService->>TxService: enqueue status_check job
TxService-->>Relayer: response
Relayer-->>Client: SignTransactionResponseSolana
Note over TxService,OnChain: Async: status_check job
TxService->>OnChain: check_onchain_transaction_status
alt signature found on-chain
TxService->>DB: update status → Mined
else blockhash expired
TxService->>TxService: mark_as_expired
else timeout exceeded
TxService->>TxService: handle_resubmit_or_expiration
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: This PR spans 40+ files with heterogeneous, interconnected changes including dependency upgrades (interface crates), major architectural shifts (new SolanaSignTrait, restructured signers, on-chain status polling), complex transaction lifecycle management with parallelized validation and resubmission logic, and comprehensive error classification and transient handling. While many changes follow consistent patterns (e.g., token interface migrations), the logic density in transaction status handling and signer refactoring requires careful reasoning per section. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements the Solana send transaction endpoint by adding comprehensive transaction lifecycle management including preparation, validation, signing, submission, status tracking, and resubmission logic for expired blockhashes.
Key changes:
- Added transaction building from instructions with fresh blockhash support
- Implemented transaction validation, signing, and submission logic
- Added status tracking with resubmission for expired blockhashes
- Migrated to interface-based SPL token dependencies
Reviewed Changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/domain/transaction/solana/solana_transaction.rs | Core transaction lifecycle methods (prepare, submit, resubmit, validate) |
| src/domain/transaction/solana/status.rs | Status checking and resubmission logic for expired blockhash handling |
| src/domain/transaction/solana/utils.rs | Utility functions for transaction decoding, building, and status mapping |
| src/domain/transaction/solana/validation.rs | Transaction validation error types with transient/permanent classification |
| src/services/signer/solana/mod.rs | Consolidated transaction signing logic with sign_sdk_transaction helper |
| src/services/provider/solana/mod.rs | Enhanced error classification for transient vs permanent failures |
| src/models/transaction/request/solana.rs | Request validation for transactions and instructions |
| src/domain/relayer/solana/solana_relayer.rs | Process transaction request and sign transaction endpoints |
| Cargo.toml | Upgraded Solana SDK dependencies to version 3 with interface packages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
helpers/test_tx.rs (1)
99-106: Fix transfer signers: pass empty slice for single-signer authority.Using &[payer] indicates multisig and can duplicate the authority account. For a non‑multisig authority, pass &[].
Apply this diff:
- let ix = token_instruction::transfer( - &spl_token_interface::id(), - token_account, - recipient_token_account, - payer, - &[payer], - amount, - )?; + let ix = token_instruction::transfer( + &spl_token_interface::id(), + token_account, + recipient_token_account, + payer, + &[], // no multisig signers + amount, + )?;src/services/signer/solana/cdp_signer.rs (1)
111-145: Limit search to required signer keys and reuse pubkey()Signature slots map to the first num_required_signatures keys. Searching the full key list risks mismatched indices. Also, reuse
pubkey()to avoid duplicating service calls.- // 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)?; + // Get the CDP signer's address to find the correct signature index + let cdp_address = self.pubkey().await?; @@ - // 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()) - })?; + // Find the signature index among required signer keys + let num_signers = signed_transaction.message.header.num_required_signatures as usize; + let signer_index = signed_transaction + .message + .account_keys + .iter() + .take(num_signers) + .position(|key| *key == cdp_pubkey) + .ok_or_else(|| { + SignerError::SigningError( + "CDP pubkey not among required transaction signers".to_string(), + ) + })?;src/models/relayer/mod.rs (2)
742-748: Logic inversion: swap_config should be allowed for Relayer fee paymentComment says swap keeps relayer funded; that’s needed when the relayer pays fees. Current check rejects swap_config when strategy == Relayer. Invert the condition and adjust the error text.
Apply this diff:
- // Swap config only supported for user fee payment strategy - if let Some(fee_payment_strategy) = &policy.fee_payment_strategy { - if *fee_payment_strategy == SolanaFeePaymentStrategy::Relayer { - return Err(RelayerValidationError::InvalidPolicy( - "Swap config only supported for user fee payment strategy".into(), - )); - } - } + // Swap config only supported when relayer pays fees (to keep relayer funded) + if let Some(fee_payment_strategy) = &policy.fee_payment_strategy { + if *fee_payment_strategy != SolanaFeePaymentStrategy::Relayer { + return Err(RelayerValidationError::InvalidPolicy( + "Swap config is only supported when fee_payment_strategy is 'relayer'".into(), + )); + } + }
1203-1210: Broken test: wrong type name (compile error)Use SolanaAllowedTokensSwapConfig instead of AllowedTokenSwapConfig.
Apply this diff:
- let config = AllowedTokenSwapConfig::default(); + let config = SolanaAllowedTokensSwapConfig::default();src/services/signer/solana/vault_signer.rs (1)
184-190: Zeroize decoded private key bytes to avoid lingering secrets in memory
hex::decodeallocates a Vec that isn’t zeroized on drop. Wrap it with Zeroizing and copy from that buffer.- let decoded_bytes = hex::decode(hex_str) - .map_err(|e| SignerError::KeyError(format!("Failed to decode hex: {}", e)))?; - - Ok(SecretVec::new(decoded_bytes.len(), |buffer| { - buffer.copy_from_slice(&decoded_bytes); - })) + let decoded_bytes = Zeroizing::new( + hex::decode(hex_str) + .map_err(|e| SignerError::KeyError(format!("Failed to decode hex: {}", e)))?, + ); + Ok(SecretVec::new(decoded_bytes.len(), |buffer| { + buffer.copy_from_slice(&decoded_bytes); + }))docs/openapi.json (1)
1566-1566: Update summary: now includes Solana.
SignTransactionRequest/Responseinclude Solana, but the summary says “Stellar only”. Update to avoid confusion.- "summary": "Signs a transaction using the specified relayer (Stellar only).", + "summary": "Signs a transaction using the specified relayer (Stellar, Solana).",src/domain/relayer/solana/rpc/methods/sign_transaction.rs (1)
23-23: Fix compile error: importerror!macro.
error!is used below but onlyinfois imported; this won’t compile.-use tracing::info; +use tracing::{info, error};src/domain/relayer/solana/rpc/methods/transfer_transaction.rs (1)
30-31: Fix: importerror!macro to avoid unresolved macro errors.
error!is used (e.g., Line 102, Line 130) but onlyinfois imported. Adderrorto the import.-use tracing::info; +use tracing::{info, error};src/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rs (1)
151-158: Persist the signed transaction to the DB, not the original unsigned bytes.You return the signed bytes to the caller but store the original input in network_data.transaction. This can desync repo state from what was actually sent and hinder resubmission/debugging.
Use the serialized signed tx for the DB update:
@@ - let update = TransactionUpdateRequest { + // Encode signed tx once and reuse for DB + response + let serialized_transaction = EncodedSerializedTransaction::try_from(&signed_transaction)?; + let update = TransactionUpdateRequest { status: Some(TransactionStatus::Submitted), sent_at: Some(Utc::now().to_rfc3339()), network_data: Some(NetworkTransactionData::Solana(SolanaTransactionData { signature: Some(send_signature.to_string()), - transaction: Some(params.transaction.clone().into_inner()), + transaction: Some(serialized_transaction.clone().into_inner()), ..Default::default() })), ..Default::default() }; @@ - let serialized_transaction = EncodedSerializedTransaction::try_from(&signed_transaction)?; + // already computed aboveAlso applies to: 171-177
src/services/provider/solana/mod.rs (1)
540-554: Broaden retry policy to all transient errors.You already classify transients via is_transient(), but retry_rpc_call only retries RpcError with certain substrings. Use is_transient() to cover NetworkError, BlockhashNotFound, SelectorError, etc.
- let is_retriable = |e: &SolanaProviderError| match e { - SolanaProviderError::RpcError(msg) => is_retriable_error(msg), - _ => false, - }; + let is_retriable = |e: &SolanaProviderError| e.is_transient();Optionally keep substring filter to down-rank noisy RPC errors, but don’t exclude other transient variants.
src/domain/relayer/solana/rpc/methods/prepare_transaction.rs (1)
179-182: Initialize signatures to match num_required_signaturesSetting
signatures: vec![Signature::default()]risks an under‑length signatures array when the message requires more than one signer, leading to invalid or ambiguous wire format later.Initialize to
num_required_signatures:- let transaction = Transaction { - signatures: vec![Signature::default()], - message, - }; + let required = message.header.num_required_signatures as usize; + let transaction = Transaction { + signatures: vec![Signature::default(); required], + message, + };
🧹 Nitpick comments (46)
.cursor/rules/rust_standards.mdc (4)
15-19: Error handling guidance should mention specific libraries.Your standards correctly emphasize idiomatic error handling, but the learnings provided indicate the project may use
eyre(0.6.x) orthiserrorfor error types. Adding concrete library recommendations would improve clarity.Consider enhancing line 18 with library guidance:
- Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations. + Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations. For libraries, prefer thiserror; for applications, consider eyre for rich error reporting.
39-43: Testing guidance could benefit from specificity.Line 40 advises "Prefer defining traits when implementing services," which is sound architectural guidance but lacks concrete context. Consider clarifying when to apply this (e.g., for dependency injection in tests, mocking external services).
Enhance line 40 with clarification:
- Prefer defining traits when implementing services to make mocking and testing easier. + Prefer defining traits for service boundaries to enable dependency injection and mocking in tests (e.g., database clients, external API clients).
45-46: Logging standards are concise and aligned with learnings.The tracing crate (0.1.x per learnings) is the right choice for structured logging. Consider one optional enhancement: mention that tracing integrates with tracing-subscriber for filtering and output configuration.
If you want to add context without expanding much, you could add a brief note:
+ (Compose logging output using tracing-subscriber for filtering, formatting, and export configuration.)
1-51: Suggested optional additions for completeness.The document is well-structured and covers the essential areas. Consider adding guidance on these topics in future iterations (not blockers):
- Dependency Management & Security: Guidance on pinning minor versions, vendoring, and auditing with
cargo-audit.- MSRV (Minimum Supported Rust Version): Document the project's MSRV and any platform-specific considerations.
- Unsafe Code & FFI: Clarify when unsafe is acceptable (beyond the brief mention in line 16) and how to document it.
- Const Generics & Advanced Type Features: Guidance on when to use advanced Rust features vs. simpler alternatives.
- Documentation Examples: Encourage including runnable examples in doc comments for complex APIs.
These could be addressed in a follow-up refinement as the project evolves.
.cursorignore (1)
3-3:.git/entry is redundant in.cursorignore.The
.git/directory should already be excluded by.gitignoreand is conventionally respected by editor ignore configs. This entry is unnecessary and can be safely removed, as it adds no practical filtering beyond what.gitignoreprovides.Remove line 3 to keep
.cursorignorefocused on editor-specific ignores:target/ Cargo.lock - -.git/Alternatively, if keeping VCS-related entries for belt-and-suspenders safety is preferred in your team's workflow, this is a minor organizational preference and can be deferred.
src/services/signer/solana/turnkey_signer.rs (1)
215-215: Deduplicate overlapping tests.test_address and test_pubkey assert the same behavior. Keep one to reduce noise.
src/domain/transaction/solana/validation.rs (2)
50-103: Make transient classification case-insensitive.Current contains() checks miss case/format variants (“rpc”, “Timeout”, etc.). Lowercase both sides to reduce false negatives.
Apply this diff:
- pub fn is_transient(&self) -> bool { - match self { + pub fn is_transient(&self) -> bool { + match self { // ... - Self::ValidationError(msg) | Self::SimulationError(msg) => { - // Check for known transient error patterns in the message - msg.contains("RPC") - || msg.contains("timeout") - || msg.contains("timed out") - || msg.contains("connection") - || msg.contains("network") - || msg.contains("Failed to check") - || msg.contains("Failed to get") - || msg.contains("node behind") - || msg.contains("rate limit") - } + Self::ValidationError(msg) | Self::SimulationError(msg) => { + let m = msg.to_ascii_lowercase(); + m.contains("rpc") + || m.contains("timeout") + || m.contains("timed out") + || m.contains("connection") + || m.contains("network") + || m.contains("failed to check") + || m.contains("failed to get") + || m.contains("node behind") + || m.contains("rate limit") + } - Self::InsufficientBalance(msg) | Self::InsufficientFunds(msg) => { - // If the message indicates an RPC failure, it's transient - msg.contains("Failed to get balance") - || msg.contains("RPC") - || msg.contains("timeout") - || msg.contains("network") - } + Self::InsufficientBalance(msg) | Self::InsufficientFunds(msg) => { + let m = msg.to_ascii_lowercase(); + m.contains("failed to get balance") + || m.contains("rpc") + || m.contains("timeout") + || m.contains("network") + } } }
752-767: Remove multi‑signer helper; use single‑signer tx in blockhash tests.The helper doesn’t actually mark the additional signer as a signer (recipient becomes signer due to header layout). Since single‑signer blockhash is now validated, simplify tests.
Apply this diff:
-fn create_multi_signer_test_transaction( - fee_payer: &Pubkey, - additional_signer: &Pubkey, -) -> Transaction { - let recipient = Pubkey::new_unique(); - let instruction = instruction::transfer(fee_payer, &recipient, 1000); - // Create message with 2 required signatures - let mut message = Message::new(&[instruction], Some(fee_payer)); - // Add second signer to account keys - if !message.account_keys.contains(additional_signer) { - message.account_keys.push(*additional_signer); - } - // Set num_required_signatures to 2 - message.header.num_required_signatures = 2; - Transaction::new_unsigned(message) -} +// No longer needed; blockhash is validated for single-signer txs.- // Use multi-signer transaction so blockhash validation actually runs - let fee_payer = Keypair::new().pubkey(); - let additional_signer = Keypair::new().pubkey(); - let transaction = create_multi_signer_test_transaction(&fee_payer, &additional_signer); + let transaction = create_test_transaction(&Keypair::new().pubkey());Also applies to: 794-814, 816-835, 838-856
src/models/error/transaction.rs (1)
53-92: Add unit tests for is_transient().Cover delegation paths and permanence vs transient cases to lock behavior.
I can add tests like:
- SolanaValidation(…RPC…) => true
- SolanaValidation(PolicyViolation) => false
- UnderlyingProvider(NetworkConfiguration) => delegated
- UnexpectedError => true
src/services/signer/solana/cdp_signer.rs (2)
101-108: Avoid base64 round‑trip for signed tx; deserialize directly with bincodeCDP returns raw signed transaction bytes. Encoding to base64 and then decoding adds allocations and CPU. Deserialize into
Transactiondirectly.- // The CDP service returns raw serialized signed-transaction bytes. - // Encode to base64 to reuse EncodedSerializedTransaction for parsing. - 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: Transaction = signed_tx_data.try_into().map_err(|e| { - SignerError::SigningError(format!("Failed to decode signed transaction: {}", e)) - })?; + // The CDP service returns raw serialized signed-transaction bytes. + // Deserialize directly to a Transaction (no base64 round-trip). + let signed_transaction: Transaction = bincode::deserialize(&signed_tx_bytes).map_err(|e| { + SignerError::SigningError(format!( + "Failed to deserialize signed transaction: {}", + e + )) + })?;Also remove the now-unused import:
- use base64::{engine::general_purpose, Engine as _};
158-182: Remove duplicate test;test_addressandtest_pubkeyassert the same behaviorBoth tests validate the same
pubkey()path with identical expectations. Keep one to reduce noise.- #[tokio::test] - async fn test_address() { - let mut mock_service = MockCdpServiceTrait::new(); - mock_service - .expect_account_address() - .times(1) - .returning(|| { - Box::pin(async { - Ok(Address::Solana( - "6s7RsvzcdXFJi1tXeDoGfSKZFzN3juVt9fTar6WEhEm2".to_string(), - )) - }) - }); - let signer = CdpSigner::new_for_testing(mock_service); - let result = signer.pubkey().await.unwrap(); - match result { - Address::Solana(addr) => { - assert_eq!(addr, "6s7RsvzcdXFJi1tXeDoGfSKZFzN3juVt9fTar6WEhEm2"); - } - _ => panic!("Expected Solana address"), - } - }src/domain/transaction/solana/status.rs (3)
287-297: Be robust: fallback to created_at if sent_at parsing failsIf
sent_atexists but is malformed, we currently returnNoneinstead of falling back tocreated_at. This weakens timeout/age checks.- fn get_time_since_sent_or_created_at(&self, tx: &TransactionRepoModel) -> Option<Duration> { - // Try sent_at first, fallback to created_at for Pending transactions - let timestamp = tx.sent_at.as_ref().or(Some(&tx.created_at))?; - match DateTime::parse_from_rfc3339(timestamp) { - Ok(dt) => Some(Utc::now().signed_duration_since(dt.with_timezone(&Utc))), - Err(e) => { - warn!(tx_id = %tx.id, ts = %timestamp, error = %e, "failed to parse timestamp"); - None - } - } - } + fn get_time_since_sent_or_created_at(&self, tx: &TransactionRepoModel) -> Option<Duration> { + // Prefer sent_at; if missing or invalid, fallback to created_at + if let Some(sent_at) = tx.sent_at.as_ref() { + match DateTime::parse_from_rfc3339(sent_at) { + Ok(dt) => { + return Some(Utc::now().signed_duration_since(dt.with_timezone(&Utc))); + } + Err(e) => { + warn!(tx_id = %tx.id, ts = %sent_at, error = %e, "failed to parse sent_at; falling back to created_at"); + } + } + } + match DateTime::parse_from_rfc3339(&tx.created_at) { + Ok(dt) => Some(Utc::now().signed_duration_since(dt.with_timezone(&Utc))), + Err(e) => { + warn!(tx_id = %tx.id, ts = %tx.created_at, error = %e, "failed to parse created_at"); + None + } + } + }
393-411: Unify status updates through the notification helper
mark_as_expiredbypassesupdate_transaction_status_and_send_notification, so no webhook is sent. Route through the shared method for consistency.- let update_request = TransactionUpdateRequest { - status: Some(TransactionStatus::Expired), - status_reason: Some(reason), - ..Default::default() - }; - - self.transaction_repository() - .partial_update(tx.id.clone(), update_request) - .await - .map_err(|e| TransactionError::UnexpectedError(e.to_string())) + self.update_transaction_status_and_send_notification( + tx, + TransactionStatus::Expired, + None, + ) + .await
413-431: Same as above: send notifications when marking Failed
mark_as_failedshould also use the unified updater to ensure webhooks are emitted.- let update_request = TransactionUpdateRequest { - status: Some(TransactionStatus::Failed), - status_reason: Some(reason), - ..Default::default() - }; - - self.transaction_repository() - .partial_update(tx.id.clone(), update_request) - .await - .map_err(|e| TransactionError::UnexpectedError(e.to_string())) + self.update_transaction_status_and_send_notification( + tx, + TransactionStatus::Failed, + None, + ) + .awaitsrc/models/relayer/mod.rs (1)
453-455: Option fields marked non-nullable in schema can mislead/OpenAPI mismatchThese are Option<...> but annotated #[schema(nullable = false)]. For responses this is fine (fields omitted when None), but for request/patch flows this signals “null not allowed,” which conflicts with common JSON Merge Patch semantics elsewhere. Suggest removing the attribute so OpenAPI reflects Option’s nullability.
Apply this diff:
@@ - #[schema(nullable = false)] pub fee_payment_strategy: Option<SolanaFeePaymentStrategy>, @@ - #[schema(nullable = false)] pub swap_config: Option<RelayerSolanaSwapConfig>,Based on learnings.
Also applies to: 464-466
src/domain/transaction/solana/mod.rs (1)
5-8: LGTM: utils/validation modules + re-exportSurface looks fine. If name collisions arise later, prefer targeted re-exports.
src/utils/mocks.rs (1)
162-162: created_at format diverges across mocksSolana mock uses RFC3339, EVM mock uses to_string(); consider standardizing to RFC3339 in both for consistency in tests/fixtures.
Apply this diff in EVM mock (optional):
- created_at: Utc::now().to_string(), + created_at: Utc::now().to_rfc3339(),src/services/provider/mod.rs (1)
46-88: Tighten retry classification (HTTP 408; JSON-RPC invalid params) to avoid noisy retriesCurrent defaults are sensible, but two tweaks reduce futile retries:
- Treat HTTP 408 (Request Timeout) as transient alongside 5xx.
- For JSON-RPC, avoid retrying classic client errors: Parse error (-32700), Invalid request (-32600), Method not found (-32601), Invalid params (-32602).
Suggested diffs:
@@ - ProviderError::RequestError { status_code, .. } => { - // 5xx errors are server-side and typically transient - // 4xx errors are client-side and typically permanent - *status_code >= 500 - } + ProviderError::RequestError { status_code, .. } => { + // 5xx are transient; also treat 408 as transient + *status_code >= 500 || *status_code == 408 + } @@ - ProviderError::RpcErrorCode { .. } => true, + ProviderError::RpcErrorCode { code, .. } => { + // Do not retry classic client-side JSON-RPC errors + !matches!(code, -32700 | -32600 | -32601 | -32602) + },Optional: consider not defaulting Other(_) to transient, or split Other into finer variants at source (e.g., builder/DNS/invalid URL) to avoid retry loops on permanent misconfig. I can help wire that through categorize_reqwest_error if desired.
Please confirm if 408s are observed from your RPCs and whether you want to map additional permanent JSON-RPC codes (e.g., application-specific invalid params).
src/services/signer/solana/vault_signer.rs (3)
95-131: De-duplicate concurrent loads with OnceCell to prevent double fetch/decryptTwo concurrent callers may both miss caches and load from Vault twice. Use
tokio::sync::OnceCell<Arc<LocalSigner>>(oronce_cell::sync::OnceCellwith blocking guard) to ensure a single load path.I can provide a small patch replacing the
Arc<Mutex<Option<Arc<LocalSigner>>>>withOnceCell.
176-182: Correct the key-type comment (Solana uses ed25519)Comment says “32 bytes = 64 hex chars for secp256k1”; for Solana this is ed25519. Suggest updating to avoid confusion.
269-275: Consider adding a positive sign() test for determinism on a fixed messageGreat switch to
pubkey(). Add a test asserting thatsign(b"msg")yields a stableSignaturefor the given mock key to cover thesignpath too.Also applies to: 285-287, 297-307, 316-327, 336-344, 355-362
docs/openapi.json (2)
7196-7223: Enforce mutual exclusivity betweeninstructionsandtransaction.Description notes exclusivity, but schema doesn’t enforce it. Add object-level oneOf to avoid ambiguous requests.
"SolanaTransactionRequest": { - "type": "object", + "type": "object", + "oneOf": [ + { "required": ["instructions"], + "properties": { "transaction": { "type": "null" } } + }, + { "required": ["transaction"], + "properties": { "instructions": { "type": "null" } } + } + ], "properties": {Also applies to: 7207-7216
6719-6741: LGTM: Instruction and account meta schemas for Solana.Consider adding “pattern” hints (base58/base64) later for generators; not blocking.
Also applies to: 6809-6834
src/domain/transaction/common.rs (1)
36-46: Harden time parsing and future timestamps.Preserve parse error details; clamp negative durations to zero to avoid surprises when clocks skew.
- let sent_time = DateTime::parse_from_rfc3339(sent_at_str) - .map_err(|_| TransactionError::UnexpectedError("Error parsing sent_at time".to_string()))? + let sent_time = DateTime::parse_from_rfc3339(sent_at_str) + .map_err(|e| TransactionError::UnexpectedError(format!("Error parsing sent_at time: {e}")))? .with_timezone(&Utc); - Ok(now.signed_duration_since(sent_time)) + let age = now.signed_duration_since(sent_time); + Ok(if age < Duration::zero() { Duration::zero() } else { age })src/models/transaction/solana/instruction.rs (2)
7-15: Deny unknown fields for safer deserialization.Prevents silent acceptance of extra keys from clients.
#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] +#[serde(deny_unknown_fields)] pub struct SolanaInstructionSpec {
18-26: Apply the same strictness toSolanaAccountMeta.#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] +#[serde(deny_unknown_fields)] pub struct SolanaAccountMeta {src/domain/relayer/solana/token.rs (1)
198-206: Unify error messages for token account unpack failures.Messages “Invalid token account1/2” are confusing. Use a single, clear message.
- let account = SplTokenAccount::unpack(&account.data) - .map_err(|e| TokenError::AccountError(format!("Invalid token account1: {}", e)))?; + let account = SplTokenAccount::unpack(&account.data) + .map_err(|e| TokenError::AccountError(format!("Invalid token account: {}", e)))?; @@ - .map_err(|e| TokenError::AccountError(format!("Invalid token account2: {}", e)))?; + .map_err(|e| TokenError::AccountError(format!("Invalid token account: {}", e)))?;Also applies to: 207-221
src/models/transaction/request/solana.rs (1)
62-79: Make RFC3339 comparison timezone‑explicit to avoid subtle bugs.Convert parsed time to UTC before comparing.
- if let Some(valid_until) = &self.valid_until { - match chrono::DateTime::parse_from_rfc3339(valid_until) { - Ok(valid_until_dt) => { - if valid_until_dt <= chrono::Utc::now() { + if let Some(valid_until) = &self.valid_until { + match chrono::DateTime::parse_from_rfc3339(valid_until) { + Ok(valid_until_dt) => { + let valid_until_utc = valid_until_dt.with_timezone(&chrono::Utc); + if valid_until_utc <= chrono::Utc::now() { return Err(ApiError::BadRequest( "valid_until cannot be in the past".to_string(), )); } }src/domain/relayer/solana/rpc/methods/utils.rs (2)
241-277: Typo: “lampart” -> “lamport” (public API and tests).Rename for clarity; avoid leaking misspelling into public surface and tests.
I can generate a repo‑wide rename diff if desired.
Also applies to: 1265-1307
31-34: Optional: deterministic UI formatting for amounts.
f64.to_string()can emit varying precision. Consider fixed trimming (e.g., strip trailing zeros with a max scale) for stable APIs.Also applies to: 360-366
src/constants/solana_transaction.rs (1)
13-13: Make timeouts strongly typed and reuse everywhere (avoid unit drift).You mix ms/seconds/minutes across constants; only the initial delay has a Duration helper. Provide Duration helpers for all timeouts and replace magic numbers at call sites (e.g., schedule_status_check_job uses 5s). This prevents unit mistakes and keeps behavior consistent.
Apply pattern like:
pub fn solana_pending_timeout() -> Duration { Duration::minutes(SOLANA_PENDING_TIMEOUT_MINUTES) } pub fn solana_sent_timeout() -> Duration { Duration::minutes(SOLANA_SENT_TIMEOUT_MINUTES) }and expose a const for seconds where needed.
Also applies to: 30-30, 40-47, 53-56
src/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rs (1)
197-199: Replace magic delay with the public constant.Avoid hardcoding 5. Use SOLANA_STATUS_CHECK_INITIAL_DELAY_SECONDS for consistency with constants.
+use crate::constants::SOLANA_STATUS_CHECK_INITIAL_DELAY_SECONDS; @@ - self.schedule_status_check_job(&tx_repo_model, Some(5)) + self.schedule_status_check_job(&tx_repo_model, Some(SOLANA_STATUS_CHECK_INITIAL_DELAY_SECONDS)) .await?;src/domain/relayer/mod.rs (1)
552-559: Align external response shapes for consistency.SignTransactionExternalResponse returns Evm(Vec) but Solana(SignTransactionResponseSolana). Consider using a typed EVM external response (or making both raw) for a symmetric API surface.
src/services/provider/solana/mod.rs (1)
442-448: Don’t derive Error for TokenMetadata.TokenMetadata is a data model, not an error type. Drop the Error derive to avoid misleading type semantics.
-#[derive(Error, Debug, PartialEq)] +#[derive(Debug, PartialEq)] pub struct TokenMetadata {src/domain/relayer/solana/rpc/methods/test_setup.rs (2)
22-43: Make the mock signature closure robust across multiple invocationsThe returning closure captures
signatureby move into anasync moveand assumes Copy semantics. To avoid accidental single-use/move issues ifSignature’s traits change, clone per call and avoid shrinking lifetimes.Suggested change:
- signer - .expect_sign() - .returning(move |_| Box::pin(async move { Ok(signature) })); + let sig = signature.clone(); + signer + .expect_sign() + .returning(move |_| { + let sig = sig.clone(); + Box::pin(async move { Ok(sig) }) + });
789-793: Strengthen the signature assertionYou already return the mock signature from
setup_signer_mocks. Assert equality with that value instead of just “non‑zero” to catch mismatches.Example:
-assert_ne!(final_tx.signatures[0].as_ref(), &[0u8; 64], ...); +let expected = setup_signer_mocks(&mut signer, relayer.address.clone()); +assert_eq!(final_tx.signatures[0], expected, "Relayer signature mismatch");src/models/transaction/repository.rs (1)
440-448: Enforce mutual exclusivity and consider serde skipsDocstrings mark
transactionandinstructionsas mutually exclusive, but no validation enforces it. Add a validator or constructor to prevent both being Some. Also consider#[serde(skip_serializing_if = "Option::is_none")]to reduce JSON noise.Example:
#[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct SolanaTransactionData { - pub transaction: Option<String>, - pub instructions: Option<Vec<SolanaInstructionSpec>>, + #[serde(skip_serializing_if = "Option::is_none")] + pub transaction: Option<String>, + #[serde(skip_serializing_if = "Option::is_none")] + pub instructions: Option<Vec<SolanaInstructionSpec>>, pub signature: Option<String>, }And a simple check when constructing/updating:
if data.transaction.is_some() && data.instructions.is_some() { return Err(TransactionError::ValidationError( "Provide either transaction or instructions, not both".into(), )); }src/services/signer/solana/mod.rs (1)
165-167: Avoid extra allocation when converting Address → PubkeyMinor: instead of
Pubkey::from_str(&signer_address.to_string()), match the enum and pass the inner string to avoid an extra allocation.- let signer_pubkey = Pubkey::from_str(&signer_address.to_string()) + let signer_pubkey = match signer_address { + Address::Solana(ref s) => Pubkey::from_str(s), + _ => Err(SignerError::KeyError("Non-Solana address".into()))?, + } .map_err(|e| SignerError::KeyError(format!("Invalid signer address: {}", e)))?;src/domain/transaction/solana/utils.rs (1)
26-28: Clarify “exceeded” vs “reached” attemptsIf the intent is “block at the limit,” use
>=instead of>; if “allow up to the limit, block only after,” keep>. Consider aligning with EVM semantics for consistency.-pub fn too_many_solana_attempts(tx: &TransactionRepoModel) -> bool { - tx.hashes.len() > MAXIMUM_SOLANA_TX_ATTEMPTS +pub fn too_many_solana_attempts(tx: &TransactionRepoModel) -> bool { + tx.hashes.len() >= MAXIMUM_SOLANA_TX_ATTEMPTS }src/domain/relayer/solana/solana_relayer.rs (7)
265-269: Fix log typo and tighten phrasingMinor typo “for for” and noisy message. Suggest this tweak for clarity.
- info!( - "Sending job request for for relayer {} swapping tokens due to relayer swap_min_balance_threshold: Balance: {}, swap_min_balance_threshold: {}", - self.relayer.id, balance, swap_min_balance_threshold - ); + info!( + "Sending token-swap job for relayer {} due to low balance. balance={}, threshold={}", + self.relayer.id, balance, swap_min_balance_threshold + );
386-403: Don’t silently swallow potential calculate_swap_amount errorsUsing unwrap_or(0) could hide future validation errors if calculate_swap_amount ever returns Err. Prefer explicit handling.
- let swap_amount = self - .calculate_swap_amount( + let swap_amount = match self.calculate_swap_amount( token_account.amount, token .swap_config .as_ref() .and_then(|config| config.min_amount), token .swap_config .as_ref() .and_then(|config| config.max_amount), token .swap_config .as_ref() .and_then(|config| config.retain_min_amount), - ) - .unwrap_or(0); + ) { + Ok(v) => v, + Err(e) => { + warn!(%relayer_id, mint=%token.mint, error=%e, "skipping token due to invalid swap config"); + 0 + } + };
633-699: Validate input before signingAdd an early guard to reject empty transactions; avoids invoking the signer with bad input.
- // Prepare transaction data for signing - let transaction_data = NetworkTransactionData::Solana(SolanaTransactionData { - transaction: Some(transaction_bytes.clone().into_inner()), + // Prepare transaction data for signing + let raw_tx = transaction_bytes.clone().into_inner(); + if raw_tx.is_empty() { + return Err(RelayerError::ValidationError("Empty transaction".to_string())); + } + let transaction_data = NetworkTransactionData::Solana(SolanaTransactionData { + transaction: Some(raw_tx), ..Default::default() });
725-815: Align JSON‑RPC codes for PREPARATION_ERRORMultiple variants map PREPARATION_ERROR to different codes (e.g., -32013 vs -32601). Consider standardizing to a single server‑error code (e.g., -32013) for consistency and easier client handling.
930-959: Run health checks concurrentlyvalidate_rpc and validate_min_balance are independent; running them in parallel reduces startup/rehydration latency.
- let validate_rpc_result = self.validate_rpc().await; - let validate_min_balance_result = self.validate_min_balance().await; + let (validate_rpc_result, validate_min_balance_result) = + futures::join!(self.validate_rpc(), self.validate_min_balance());
961-979: Log threshold alongside balanceFor better observability, include the effective threshold in logs.
- let balance = self + let balance = self .provider .get_balance(&self.relayer.address) .await .map_err(|e| RelayerError::ProviderError(e.to_string()))?; - - debug!(balance = %balance, "balance for relayer"); - - let policy = self.relayer.policies.get_solana_policy(); - - if balance < policy.min_balance.unwrap_or(DEFAULT_SOLANA_MIN_BALANCE) { + let policy = self.relayer.policies.get_solana_policy(); + let threshold = policy.min_balance.unwrap_or(DEFAULT_SOLANA_MIN_BALANCE); + debug!(balance = %balance, threshold = %threshold, "balance for relayer"); + if balance < threshold { return Err(RelayerError::InsufficientBalanceError( "Insufficient balance".to_string(), )); }
568-572: Remove unnecessary error re-wrapping; convert RepositoryError directly to RelayerErrorThe
.create()method already returnsResult<T, RepositoryError>. The current code unnecessarily wraps this error intoRepositoryError::TransactionFailurebefore converting toRelayerErrorviaFrom. SinceFrom<RepositoryError> for RelayerErrorexists, skip the intermediate wrapping and convert directly:self.transaction_repository .create(transaction.clone()) .await - .map_err(|e| RepositoryError::TransactionFailure(e.to_string()))?; + .map_err(RelayerError::from)?;
📜 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 (47)
.cursor/rules/rust_standards.mdc(1 hunks).cursorignore(1 hunks)Cargo.toml(2 hunks)docs/openapi.json(10 hunks)helpers/test_tx.rs(2 hunks)src/constants/mod.rs(1 hunks)src/constants/solana_transaction.rs(1 hunks)src/domain/mod.rs(1 hunks)src/domain/relayer/mod.rs(8 hunks)src/domain/relayer/solana/rpc/methods/fee_estimate.rs(4 hunks)src/domain/relayer/solana/rpc/methods/mod.rs(1 hunks)src/domain/relayer/solana/rpc/methods/prepare_transaction.rs(15 hunks)src/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rs(20 hunks)src/domain/relayer/solana/rpc/methods/sign_transaction.rs(18 hunks)src/domain/relayer/solana/rpc/methods/test_setup.rs(6 hunks)src/domain/relayer/solana/rpc/methods/transfer_transaction.rs(16 hunks)src/domain/relayer/solana/rpc/methods/utils.rs(27 hunks)src/domain/relayer/solana/solana_relayer.rs(16 hunks)src/domain/relayer/solana/token.rs(27 hunks)src/domain/transaction/common.rs(3 hunks)src/domain/transaction/evm/status.rs(1 hunks)src/domain/transaction/evm/utils.rs(0 hunks)src/domain/transaction/mod.rs(2 hunks)src/domain/transaction/solana/mod.rs(1 hunks)src/domain/transaction/solana/solana_transaction.rs(6 hunks)src/domain/transaction/solana/status.rs(15 hunks)src/domain/transaction/solana/utils.rs(1 hunks)src/domain/transaction/solana/validation.rs(13 hunks)src/models/error/transaction.rs(3 hunks)src/models/relayer/mod.rs(3 hunks)src/models/relayer/request.rs(1 hunks)src/models/transaction/repository.rs(8 hunks)src/models/transaction/request/mod.rs(1 hunks)src/models/transaction/request/solana.rs(1 hunks)src/models/transaction/response.rs(3 hunks)src/models/transaction/solana/instruction.rs(1 hunks)src/models/transaction/solana/mod.rs(1 hunks)src/services/provider/mod.rs(1 hunks)src/services/provider/solana/mod.rs(17 hunks)src/services/signer/solana/cdp_signer.rs(3 hunks)src/services/signer/solana/google_cloud_kms_signer.rs(2 hunks)src/services/signer/solana/local_signer.rs(1 hunks)src/services/signer/solana/mod.rs(4 hunks)src/services/signer/solana/turnkey_signer.rs(2 hunks)src/services/signer/solana/vault_signer.rs(6 hunks)src/services/signer/solana/vault_transit_signer.rs(0 hunks)src/utils/mocks.rs(1 hunks)
💤 Files with no reviewable changes (2)
- src/domain/transaction/evm/utils.rs
- src/services/signer/solana/vault_transit_signer.rs
🔇 Additional comments (54)
.cursor/rules/rust_standards.mdc (5)
1-6: YAML front matter is well-structured for Cursor integration.The globs pattern and alwaysApply flag are correctly configured to apply these standards across the Rust codebase.
8-13: Code style guidelines are solid; verify rustfmt configuration alignment.The guidelines reference
max_width = 100andedition = 2021, which are specific and actionable. Ensure these values match your committedrustfmt.tomlto avoid conflicts.Can you confirm the project's rustfmt.toml specifies
max_width = 100? If it differs, align this documentation with the actual configuration.
29-31: Async guidance is prescriptive; clarify flexibility around runtime choice.Line 30 states "Always use Tokio runtime for async code," which is appropriate for this project (aligned with learnings showing tokio 1.43.0). However, verify that there are no legacy async-std usages or scenario-specific requirements that would contradict this mandate.
Confirm that the codebase has standardized on Tokio exclusively, with no ongoing use of alternative async runtimes (async-std, smol, etc.).
33-37: Serialization and data standards align well with learnings.The emphasis on serde with derive macros, trait implementations, and Display trait is consistent with best practices and the retrieved learnings on serde 1.0.x.
48-51: Code organization guidance is pragmatic and well-balanced.The emphasis on clarity-first optimization and modularization is sound. The suggestion to use derive macros aligns with idiomatic Rust and reduces boilerplate effectively.
.cursorignore (1)
2-2: No issues found—Cargo.lock is correctly version-controlled.For an executable project like openzeppelin-relayer, Cargo.lock should be checked into version control, and the verification confirms this is already the case: Cargo.lock is not in .gitignore, meaning it remains tracked by git. Adding it to .cursorignore only affects editor visibility and has no impact on version control behavior.
Likely an incorrect or invalid review comment.
helpers/test_tx.rs (1)
11-11: Interface import looks correct.Switch to spl_token_interface aligns with the new interface-based stack.
src/services/signer/solana/turnkey_signer.rs (1)
99-99: LGTM on API switch.Test updated to pubkey() matches the new SolanaSignTrait.
src/domain/transaction/solana/validation.rs (3)
19-22: Imports update looks good.CommitmentConfig and system-interface program/SystemInstruction align with Solana v3 + interface crates.
25-25: Good: error type is now serializable.Deriving Serialize on SolanaTransactionValidationError enables API surfacing.
650-650: Token interface migration looks correct.Using spl_token_interface::instruction and state::Account with interface IDs is consistent with the crate switch.
Also applies to: 670-676, 713-728
src/models/error/transaction.rs (2)
19-21: New SolanaValidation variant: good addition.Enables seamless propagation of Solana validation errors.
98-98: API mapping is sensible.SolanaValidation -> BadRequest matches validation semantics.
Cargo.toml (1)
65-71: The review comment is incorrect—the version mix is intentional and required.mpl-token-metadata v5.1.1 requires solana-program ">=1.14, <3.0", which means upgrading solana-program to v3 would violate this constraint and break the build. The current configuration (solana-sdk/client/commitment-config v3 with solana-program v2) is correct and necessary.
The single usage of solana-program in the codebase (src/services/provider/solana/mod.rs:768) is an explicit type conversion for mpl-token-metadata and does not cause ABI conflicts. The Cargo.lock resolves without errors, and no version mismatch issues are evident in the codebase.
Likely an incorrect or invalid review comment.
src/domain/mod.rs (1)
9-13: Publicly exposingrelayerandtransactionlooks goodSurface matches new Solana flows; keep in mind this expands the public API.
Confirm CI/docs are updated for the broader public surface.
src/constants/mod.rs (1)
20-22: LGTM: re-exporting Solana tx constantsMakes Solana defaults accessible where needed.
src/models/transaction/request/mod.rs (1)
43-44: Explicit Solana validation path: goodDelegating to
request.validate(relayer)for Solana aligns with typed validation.src/models/relayer/mod.rs (3)
382-388: Good addition: explicit Solana fee payment strategy + docsEnum + Default align with described behavior. No issues spotted.
754-760: Jupiter strategy restricted to mainnet-beta — confirm intended environmentsIf devnet/testnet support is desired (for testing), consider allowing testnets via config flag rather than hardcoding mainnet-beta.
Would you like me to gate this on a “is_testnet” flag from SolanaNetworkConfig instead?
804-806: Priority level values — verify allowed setAllowed list is ["medium", "high", "veryHigh"]. Please confirm exact casing/values expected by Jupiter to avoid client surprises.
I can adjust and add tests if you confirm the authoritative list.
src/models/transaction/solana/mod.rs (1)
1-2: LGTM: re-export instruction modulePublic surface expansion is straightforward and consistent.
src/utils/mocks.rs (1)
167-169: LGTM: transaction now Option in mockInitialization with Some(...) matches the new Optional type. Ensure downstream assertions don’t expect a non-optional string literal.
src/domain/transaction/mod.rs (2)
486-487: LGTM: signer wired into Solana constructorPassing
signer_serviceintoSolanaRelayerTransaction::newmatches the new signer-aware design.
477-479: Remove the suggested refactoring; the type annotation contradicts the.into()conversionThe proposed binding in the review comment contains a type mismatch. The parameter
signerisSignerRepoModel, andsigner.into()converts toSigner(domain model) via theFrom<SignerRepoModel> for Signerimpl. Annotating it asSignerRepoModelwould fail to compile.The original code pattern
&signer.into()is already used consistently across the codebase (e.g., line 55 insrc/domain/relayer/solana/mod.rs). If you want to improve clarity by binding, the correct type would beSigner:- let signer_service = - Arc::new(SolanaSignerFactory::create_solana_signer(&signer.into())?); + let signer_domain: Signer = signer.into(); + let signer_service = + Arc::new(SolanaSignerFactory::create_solana_signer(&signer_domain)?);However, this refactoring is optional and not critical since the current pattern is already established elsewhere in the codebase and works correctly.
Likely an incorrect or invalid review comment.
src/domain/transaction/evm/status.rs (1)
12-15: LGTM: imports consolidated and pending-status helper usedThe added helpers (
get_age_of_sent_at,is_pending_transaction) make the flow clearer and avoid redundant checks.Also applies to: 20-22
src/domain/relayer/solana/rpc/methods/mod.rs (1)
26-29: Now let me check if Solana module uses any validations imports elsewhere:Verification confirms the re-export is correct; no lingering validations imports in Solana module
The scan found only one
validations::*reference in the entire codebase—in the EVM module (src/domain/relayer/evm/mod.rs). This is a separate, domain-specific validations module unrelated to Solana's re-export.Solana correctly re-exports from
crate::domain::transaction::solanawith no lingering references to the oldvalidations::*path. The code change aligns the surface properly.docs/openapi.json (4)
3837-3839: LGTM: Solana sign-transaction response added to union.
6381-6391: LGTM: NewSignTransactionRequestSolanamodel (base64 transaction).
6421-6435: LGTM: NewSignTransactionResponseSolana(transaction + signature).
7582-7584: LGTM: ClarifiedUpdateRelayerRequest.policiesdocs.src/domain/transaction/common.rs (1)
29-34: LGTM: Pending-state helper covers Pending/Sent/Submitted.src/models/transaction/response.rs (2)
241-244: LGTM: Tests updated for optionaltransactionin repo model.Also applies to: 389-392
132-132: Make responsetransactionfield optional or handle None as error condition.The code converts
solana_data.transaction: Option<String>toSolanaTransactionResponse.transaction: Stringviaunwrap_or_default(), silently replacingNonewith an empty string. However, elsewhere in the codebase (src/services/signer/solana/mod.rs),Noneis explicitly treated as an error state with the message "Transaction not yet built - only available after preparation," indicating this state can and does occur.This creates two problems:
- Data masking: Consumers receive an empty string and cannot distinguish whether the transaction is genuinely empty or was never built.
- Schema inconsistency: The response schema marks
transactionasnullable = false, but the field may be logically absent during the transaction lifecycle.Either make
SolanaTransactionResponse.transactionanOption<String>(matching the source type and updating OpenAPI schema), or replaceunwrap_or_default()with an error check to fail fast when a transaction should exist but doesn't.src/domain/relayer/solana/rpc/methods/sign_transaction.rs (1)
176-178: LGTM: Centralized signer mocks and interface-based SPL types in tests.Run the test suite to ensure mocks cover all paths after the move to interface crates.
Also applies to: 296-298, 402-404, 928-930, 1074-1076
src/domain/relayer/solana/rpc/methods/fee_estimate.rs (1)
24-26: LGTM on commitment import and fee estimation flow.Switch to
solana_commitment_config::CommitmentConfigand the payer-adjusting estimation path looks good.src/services/signer/solana/local_signer.rs (1)
50-60: LGTM: SolanaSignTrait impl is minimal and correct.pubkey()/sign() correctly wrap Keypair behavior and error handling.
src/services/signer/solana/google_cloud_kms_signer.rs (1)
68-93: LGTM: trait migration to SolanaSignTrait is sound.Clean error mapping, signature length validation via
Signature::try_from.src/models/transaction/request/solana.rs (1)
126-143: Confirm product intent: blocking any non‑relayer signer in instructions.The rule “Only the relayer can be marked as a signer” forbids typical user‑authority instructions. If the endpoint must support user‑authorized ops (with user signatures collected later), this will reject valid requests.
Would you like me to propose a mode that:
- allows additional signers but requires the final submitted transaction to include their signatures (validated elsewhere), or
- keeps current default but adds a feature flag to relax this check?
Also applies to: 187-214, 233-243
src/domain/relayer/solana/rpc/methods/sign_and_send_transaction.rs (1)
240-247: Validator concurrency looks good.Grouping sync validations and joining with blockhash/simulate/token/lamports checks is clean and efficient.
src/domain/relayer/mod.rs (1)
216-225: Enum dispatch additions look correct.Solana branches delegate to corresponding relayers; signatures align with trait methods.
Also applies to: 279-286, 287-294, 295-302, 311-323
src/domain/transaction/solana/solana_transaction.rs (3)
220-228: Guard against zero-required-signature edge case before writing signatures[0].Highly unlikely, but if message.header.num_required_signatures == 0, indexing signatures[0] would panic. Add a debug_assert! or early validation.
+ debug_assert!(transaction.message.header.num_required_signatures > 0, "tx must require at least 1 signature"); transaction.signatures[0] = signature;If you prefer explicit errors, return a ValidationError when required_signatures == 0.
668-686: Parallel validation is well structured.Nice separation of policy, blockhash/simulate, token checks, and fee/balance validation with try_join.
431-436: Resubmit flow handles blockhash refresh and error classes correctly.Refreshing recent_blockhash, re-signing, and handling AlreadyProcessed/transient vs permanent errors is spot on.
Also applies to: 437-445, 463-507
src/services/provider/solana/mod.rs (1)
136-167: Error classification taxonomy is solid.Nice mapping of ClientError kinds and JSON-RPC codes into transient/permanent categories.
Also applies to: 169-252, 254-314
src/domain/relayer/solana/rpc/methods/test_setup.rs (1)
147-156: Switch to interface crates looks correctUsing
spl_token_interface::instruction::transfer_checkedandspl_associated_token_account_interface::address::get_associated_token_addressaligns with the v3 interface crates. No issues spotted in params order or IDs.Also applies to: 263-272, 276-285, 403-412
src/domain/relayer/solana/rpc/methods/prepare_transaction.rs (1)
278-285: Good move to shared signer mock helperReplacing inlined expectations with
setup_signer_mocksreduces duplication and keeps tests consistent.Also applies to: 503-505, 637-639, 721-723
src/models/transaction/repository.rs (1)
856-863: Mapping from request looks correctCloning
transactionandinstructionsfrom the request into the repo model is straightforward and preservesvalid_until.src/services/signer/solana/mod.rs (1)
69-72: Address delegation via pubkey() is fineDelegating
address()topubkey()keeps one source of truth for the Solana address.src/domain/transaction/solana/utils.rs (1)
166-175: Transaction build helper looks goodBuilding from
Instructionspecs and settingrecent_blockhashis correct; payer is first signer as expected.src/domain/relayer/solana/solana_relayer.rs (5)
66-66: Signer bound addition looks goodRequiring S: SolanaSignTrait + Signer across impls prevents accidental use of non‑signing types and tightens API guarantees. No concerns.
Also applies to: 91-91, 323-323, 522-522
534-552: Fee strategy gating is a good safety checkDefaulting None→User and requiring fee_payment_strategy='relayer' for this endpoint is a solid guardrail. Please add unit tests covering:
- None/User → ValidationError
- Relayer → creates transaction, enqueues request and status jobs
580-593: Nice: enqueue status check with initial delayGood use of SOLANA_STATUS_CHECK_INITIAL_DELAY_SECONDS and scheduled timestamp.
825-857: Status aggregation looks correctCount/slice logic and last_confirmed_transaction_timestamp derivation are sound.
1314-1323: Token account packing via spl_token_interface is appropriatePacking Account into data with correct owner/program id is correct for tests targeting interface‑based SPL tokens.
Also applies to: 1478-1486, 1491-1491, 1511-1526
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #521 +/- ##
=======================================
Coverage 93.1% 93.1%
=======================================
Files 241 244 +3
Lines 87849 92936 +5087
=======================================
+ Hits 81833 86576 +4743
- Misses 6016 6360 +344
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:
|
Summary
Changes
Upgrade Dependencies:
Updated Solana-related packages to their latest stable versions.
New Endpoints:
Implemented the following Solana RPC endpoints:
Transaction Status Logic:
Enhanced the transaction status handling to:
Miscellaneous Improvements:
Various internal enhancements and cleanup for better maintainability and consistency.
OpenZeppelin/openzeppelin-relayer-sdk#221
Testing Process
Checklist
Summary by CodeRabbit
New Features
Updates