support partially signed serialised transactions for solana #77
Conversation
WalkthroughRefactors Solana transaction handling to accept two input types (instructions or pre-serialized), introducing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as execution_router
participant Worker as solana_executor
participant TxnLayer as solana_core::transaction
Client->>Router: Send transaction request
Router->>Router: Build SolanaTransactionOptions (flattened `input`)
Router->>Worker: execute_transaction(options)
alt input == Instructions
Worker->>Worker: Extract instructions
Worker->>Worker: get_compute_unit_price(priority_fee, instructions)
Worker->>TxnLayer: build_from_instructions -> VersionedTransaction
else input == Serialized
Worker->>TxnLayer: provide base64 transaction
TxnLayer->>TxnLayer: deserialize + verify fee payer
end
TxnLayer->>Worker: return VersionedTransaction
Worker->>Worker: append memo (uniqueness)
Worker->>Client: return signed/ready transaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
solana-core/src/transaction.rs (2)
12-24: Untagged enum design: Consider validation for ambiguous inputs.The
#[serde(untagged)]attribute means serde will try to deserialize into the first matching variant. With the current design:
- If JSON contains
instructionsfield →Instructionsvariant- If JSON contains
transactionfield →Serializedvariant- If JSON contains both fields →
Instructionsvariant (first match wins), andtransactionis silently ignoredThis could lead to user confusion if both fields are accidentally provided.
Consider adding a validation layer that rejects inputs with both fields, or use a tagged enum approach instead:
#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)] #[serde(rename_all = "camelCase", tag = "type")] pub enum SolanaTransactionInput { #[serde(rename = "instructions")] Instructions { instructions: Vec<SolanaInstructionData>, }, #[serde(rename = "serialized")] Serialized { transaction: String, }, }This would require clients to specify
"type": "instructions"or"type": "serialized", making the intent explicit.
198-228: Good fee payer verification, but consider additional validation.The
deserialize_transactionmethod correctly verifies that the fee payer matches the expected payer, which is important for security. The note about compute budget fields being ignored is also well-documented.However, consider these additional validations:
- Signature state: If the transaction already has valid signatures (not default), you may want to detect this and handle it appropriately
- Blockhash validation: The deserialized transaction contains a blockhash that may be stale
- Runtime check for compute budget: If
compute_unit_limitorcompute_unit_priceare set when using a serialized transaction, consider returning a validation error rather than silently ignoring themConsider adding validation:
fn deserialize_transaction( &self, tx_base64: &str, expected_payer: Pubkey, ) -> Result<VersionedTransaction, SolanaTransactionError> { + // Validate that compute budget fields aren't set + if self.compute_unit_limit.is_some() || self.compute_unit_price.is_some() { + return Err(SolanaTransactionError::InvalidConfiguration { + error: "compute_unit_limit and compute_unit_price cannot be used with serialized transactions".to_string(), + }); + } + let tx_bytes = Base64Engine.decode(tx_base64) .map_err(|e| SolanaTransactionError::DeserializationFailed { error: format!("Invalid base64: {}", e), })?; // ... rest of method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- 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 (8)
.gitignore(1 hunks)Cargo.toml(1 hunks)core/src/execution_options/solana.rs(2 hunks)executors/src/solana_executor/rpc_cache.rs(1 hunks)executors/src/solana_executor/worker.rs(3 hunks)server/src/execution_router/mod.rs(1 hunks)solana-core/Cargo.toml(1 hunks)solana-core/src/transaction.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
PR: thirdweb-dev/engine-core#48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.
Applied to files:
executors/src/solana_executor/rpc_cache.rsexecutors/src/solana_executor/worker.rs
🧬 Code graph analysis (1)
executors/src/solana_executor/worker.rs (1)
core/src/execution_options/mod.rs (2)
chain_id(180-182)transaction_id(184-186)
🔇 Additional comments (13)
.gitignore (1)
5-5: LGTM!Adding
.env.testto.gitignoreis a best practice for isolating test environment configuration from version control.Note: The PR objectives mention substantial Solana transaction serialization refactoring (new
SolanaTransactionInputenum, executor logic, transaction layer changes), but only.gitignorewas provided for review. If you'd like me to review the core Solana transaction handling changes, please provide those files (e.g., solana-core, executors, transaction layer files).solana-core/Cargo.toml (1)
17-17: LGTM! Dependency addition is appropriate.The bincode dependency is correctly added to support the new serialized transaction handling introduced in this PR.
executors/src/solana_executor/rpc_cache.rs (1)
4-4: LGTM! Unused import cleanup.Removing the unused
std::time::Durationimport is good housekeeping.Cargo.toml (1)
44-44: LGTM! Workspace dependency properly configured.The bincode dependency with serde feature support is correctly configured for the serialization needs of this PR.
server/src/execution_router/mod.rs (1)
516-519: LGTM! Field rename aligns with new API structure.The change from
instructionstoinputcorrectly reflects the newSolanaTransactionInputenum introduced in this PR, which supports both instruction-based and serialized transaction inputs.core/src/execution_options/solana.rs (2)
150-152: Same flattening pattern applied consistently.The same
#[serde(flatten)]pattern is applied here, maintaining consistency withSolanaTransactionOptions. The same serialization considerations apply.
134-136: Review the deserialization behavior to ensure ambiguous input is handled acceptably.The concern is valid: when an untagged enum with
#[serde(flatten)]receives JSON with bothinstructionsandtransactionfields, serde will deserialize to the first matching variant (Instructions) and silently ignore thetransactionfield.The API documentation states "either instructions or serialized transaction" but does not enforce this mutual exclusivity at deserialization time. While this is standard untagged enum behavior and likely not a practical issue for typical API users, there are no tests covering this edge case. Verify that this behavior aligns with your API contract expectations—consider whether validation should reject inputs containing both fields or whether documentation should be clarified.
solana-core/src/transaction.rs (3)
138-152: LGTM! Clean dispatcher pattern for dual input paths.The refactored
to_versioned_transactionmethod cleanly dispatches to the appropriate builder based on the input variant. This is a good separation of concerns.
154-196: LGTM! Transaction building from instructions is well-structured.The
build_from_instructionsmethod correctly:
- Adds compute budget instructions first (if specified)
- Converts user instructions to Solana SDK instructions
- Compiles the versioned message with proper error handling
248-253: LGTM! Error variants appropriately added.The new error variants
DeserializationFailedandFeePayerMismatchprovide clear error messages for the new deserialization path.executors/src/solana_executor/worker.rs (3)
436-480: Helper function refactoring looks good.The signature change for
get_writable_accountsto accept&[SolanaInstructionData]and the newget_compute_unit_priceasync function are well-structured and correctly implemented. The logic cleanly handles all priority fee variants.
717-762: Instructions variant implementation is solid.The logic correctly handles priority fee calculation, adds a memo instruction to ensure unique signatures on resubmission, and properly constructs the versioned transaction. Error handling with contextual logging is appropriate.
763-785: Review comment is incorrect—blockhash is preserved for serialized transactions.The
to_versioned_transactionmethod receivesrecent_blockhashbut does not pass it todeserialize_transactionfor the Serialized variant (line 149). Thedeserialize_transactionfunction deserializes the base64-encoded transaction bytes using bincode and returns the VersionedTransaction as-is, preserving the original blockhash embedded in the serialized transaction. This correctly supports partially signed transactions—the original signatures, which were computed over the original blockhash, remain valid.Likely an incorrect or invalid review comment.
- Added `bincode` as a dependency in the Cargo.toml for the executors. - Updated the SolanaExecutorJobHandler to handle serialized transactions with existing signatures more robustly, preventing retries that would invalidate signatures when the blockhash expires.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- 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 (2)
executors/Cargo.toml(1 hunks)executors/src/solana_executor/worker.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
PR: thirdweb-dev/engine-core#48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.
Applied to files:
executors/src/solana_executor/worker.rs
🔇 Additional comments (5)
executors/Cargo.toml (1)
9-9: LGTM - Dependency addition supports new serialization path.The
bincodedependency withserdefeature is correctly added to support deserializing base64-encoded Solana transactions as part of the newSolanaTransactionInput::Serializedvariant.executors/src/solana_executor/worker.rs (4)
14-14: LGTM - Import additions support new functionality.The
Pubkeyandbase64::Engineimports are correctly added to support the refactored writable accounts extraction and transaction serialization/deserialization logic.Also applies to: 20-20
436-446: LGTM - Well-focused refactor.The function signature change from accepting
&SolanaTransactionOptionsto&[SolanaInstructionData]is a good improvement that makes the function more focused and reusable. The logic correctly extracts writable accounts from the provided instructions.
448-480: LGTM - Clean extraction of compute unit price logic.The new
get_compute_unit_pricefunction appropriately extracts priority fee calculation logic. It correctly handles all threeSolanaPriorityFeevariants (Auto, Manual, Percentile) with proper error handling using NACK for retryable network operations.
749-817: Document behavioral differences between Instructions and Serialized variants.The transaction building logic correctly handles the two input variants with appropriate behavior:
- Instructions: Applies execution options (priority fees, compute limits) and adds memo for unique signatures
- Serialized: Ignores execution options to avoid invalidating existing signatures (lines 796-801)
However, this behavioral difference should be clearly documented in user-facing documentation or API specifications, as users might not expect their
execution_optionsto be ignored when providing a serialized transaction.Consider:
- Adding API documentation explaining that
execution_options.priority_feeandexecution_options.compute_unit_limitare ignored for serialized transactions- Documenting that memo instructions are only added for instruction-based transactions
- Explaining the tradeoff: serialized transactions with existing signatures cannot be modified without invalidating those signatures
| // Blockhash expired | ||
|
|
||
| // For serialized transactions with existing signatures, we cannot retry with a new blockhash | ||
| // because the signatures will become invalid. Check if there are any non-default signatures. | ||
| if let engine_solana_core::transaction::SolanaTransactionInput::Serialized { transaction } = &job_data.transaction.input { | ||
| // Deserialize the base64 transaction to check for signatures | ||
| if let Ok(tx_bytes) = base64::engine::general_purpose::STANDARD.decode(transaction) | ||
| && let Ok((versioned_tx, _)) = bincode::serde::decode_from_slice::<solana_sdk::transaction::VersionedTransaction, _>( | ||
| &tx_bytes, | ||
| bincode::config::standard() | ||
| ) { | ||
| // Check if any signatures are non-default (not all zeros) | ||
| let has_signatures = versioned_tx.signatures.iter().any(|sig| { | ||
| sig.as_ref() != [0u8; 64] | ||
| }); | ||
|
|
||
| if has_signatures { | ||
| error!( | ||
| transaction_id = %transaction_id, | ||
| signature = %signature, | ||
| "Blockhash expired for serialized transaction with existing signatures - cannot retry without invalidating them" | ||
| ); | ||
| let _ = self.storage.delete_attempt(transaction_id).await; | ||
| return Err(SolanaExecutorError::TransactionFailed { | ||
| reason: "Blockhash expired for serialized transaction with existing signatures. Retrying with a new blockhash would invalidate them.".to_string(), | ||
| } | ||
| .fail()); | ||
| } | ||
| // If no signatures, we can retry - will be signed during execution | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Handle deserialization failures explicitly for serialized transactions.
The blockhash expiry logic for serialized transactions with existing signatures is sound — preventing retry when signatures would be invalidated. However, if the base64 decode or bincode deserialize fails (lines 606-610), the error is silently ignored and the transaction falls through to retry logic. This could mask malformed serialized transactions.
Consider handling deserialization failures explicitly:
- if let engine_solana_core::transaction::SolanaTransactionInput::Serialized { transaction } = &job_data.transaction.input {
- // Deserialize the base64 transaction to check for signatures
- if let Ok(tx_bytes) = base64::engine::general_purpose::STANDARD.decode(transaction)
- && let Ok((versioned_tx, _)) = bincode::serde::decode_from_slice::<solana_sdk::transaction::VersionedTransaction, _>(
- &tx_bytes,
- bincode::config::standard()
- ) {
- // Check if any signatures are non-default (not all zeros)
- let has_signatures = versioned_tx.signatures.iter().any(|sig| {
- sig.as_ref() != [0u8; 64]
- });
-
- if has_signatures {
- error!(
- transaction_id = %transaction_id,
- signature = %signature,
- "Blockhash expired for serialized transaction with existing signatures - cannot retry without invalidating them"
- );
- let _ = self.storage.delete_attempt(transaction_id).await;
- return Err(SolanaExecutorError::TransactionFailed {
- reason: "Blockhash expired for serialized transaction with existing signatures. Retrying with a new blockhash would invalidate them.".to_string(),
- }
- .fail());
- }
- // If no signatures, we can retry - will be signed during execution
- }
- }
+ if let engine_solana_core::transaction::SolanaTransactionInput::Serialized { transaction } = &job_data.transaction.input {
+ // Deserialize the base64 transaction to check for signatures
+ let tx_bytes = base64::engine::general_purpose::STANDARD.decode(transaction)
+ .map_err(|e| {
+ error!(
+ transaction_id = %transaction_id,
+ error = %e,
+ "Failed to decode serialized transaction"
+ );
+ SolanaExecutorError::TransactionBuildFailed {
+ inner_error: format!("Invalid base64 in serialized transaction: {}", e),
+ }
+ .fail()
+ })?;
+
+ let (versioned_tx, _) = bincode::serde::decode_from_slice::<solana_sdk::transaction::VersionedTransaction, _>(
+ &tx_bytes,
+ bincode::config::standard()
+ )
+ .map_err(|e| {
+ error!(
+ transaction_id = %transaction_id,
+ error = %e,
+ "Failed to deserialize transaction"
+ );
+ SolanaExecutorError::TransactionBuildFailed {
+ inner_error: format!("Invalid serialized transaction: {}", e),
+ }
+ .fail()
+ })?;
+
+ // Check if any signatures are non-default (not all zeros)
+ let has_signatures = versioned_tx.signatures.iter().any(|sig| {
+ sig.as_ref() != [0u8; 64]
+ });
+
+ if has_signatures {
+ error!(
+ transaction_id = %transaction_id,
+ signature = %signature,
+ "Blockhash expired for serialized transaction with existing signatures - cannot retry without invalidating them"
+ );
+ let _ = self.storage.delete_attempt(transaction_id).await;
+ return Err(SolanaExecutorError::TransactionFailed {
+ reason: "Blockhash expired for serialized transaction with existing signatures. Retrying with a new blockhash would invalidate them.".to_string(),
+ }
+ .fail());
+ }
+ // If no signatures, we can retry - will be signed during execution
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In executors/src/solana_executor/worker.rs around lines 600 to 630, the base64
decoding and bincode deserialization errors for serialized transactions are
currently ignored, allowing malformed serialized transactions to fall through
and be retried; update the code to explicitly handle failures by distinguishing
decode vs deserialize errors, log the specific error (including transaction_id
and signature), delete the attempt from storage, and return a
SolanaExecutorError::TransactionFailed with a clear reason containing the
decode/deserialize error so the job is not retried.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
executors/src/solana_executor/worker.rs (1)
600-630: Deserialization failures for serialized transactions are still silently ignored.Lines 606-610 use
if let Okpatterns that silently ignore deserialization errors for serialized transactions. This allows malformed base64 or invalid bincode to fall through to the retry logic, potentially causing indefinite retries without informing the user that their serialized transaction is invalid.A previous review comment on this code segment suggested handling these errors explicitly. The current implementation still has the same issue. Consider applying the suggested fix from the previous review, or alternatively, add explicit error logging at minimum:
if let engine_solana_core::transaction::SolanaTransactionInput::Serialized (t) = &job_data.transaction.input { // Deserialize the base64 transaction to check for signatures - if let Ok(tx_bytes) = base64::engine::general_purpose::STANDARD.decode(&t.transaction) - && let Ok((versioned_tx, _)) = bincode::serde::decode_from_slice::<solana_sdk::transaction::VersionedTransaction, _>( - &tx_bytes, - bincode::config::standard() - ) { + let tx_bytes = match base64::engine::general_purpose::STANDARD.decode(&t.transaction) { + Ok(bytes) => bytes, + Err(e) => { + error!( + transaction_id = %transaction_id, + error = %e, + "Failed to decode serialized transaction - invalid base64" + ); + let _ = self.storage.delete_attempt(transaction_id).await; + return Err(SolanaExecutorError::TransactionBuildFailed { + inner_error: format!("Invalid base64 in serialized transaction: {}", e), + }.fail()); + } + }; + + let (versioned_tx, _) = match bincode::serde::decode_from_slice::<solana_sdk::transaction::VersionedTransaction, _>( + &tx_bytes, + bincode::config::standard() + ) { + Ok(result) => result, + Err(e) => { + error!( + transaction_id = %transaction_id, + error = %e, + "Failed to deserialize transaction - invalid bincode" + ); + let _ = self.storage.delete_attempt(transaction_id).await; + return Err(SolanaExecutorError::TransactionBuildFailed { + inner_error: format!("Invalid serialized transaction: {}", e), + }.fail()); + } + }; + - // Check if any signatures are non-default (not all zeros) - let has_signatures = versioned_tx.signatures.iter().any(|sig| { - sig.as_ref() != [0u8; 64] - }); - - if has_signatures { - error!( - transaction_id = %transaction_id, - signature = %signature, - "Blockhash expired for serialized transaction with existing signatures - cannot retry without invalidating them" - ); - let _ = self.storage.delete_attempt(transaction_id).await; - return Err(SolanaExecutorError::TransactionFailed { - reason: "Blockhash expired for serialized transaction with existing signatures. Retrying with a new blockhash would invalidate them.".to_string(), - } - .fail()); - } - // If no signatures, we can retry - will be signed during execution - } + // Check if any signatures are non-default (not all zeros) + let has_signatures = versioned_tx.signatures.iter().any(|sig| { + sig.as_ref() != [0u8; 64] + }); + + if has_signatures { + error!( + transaction_id = %transaction_id, + signature = %signature, + "Blockhash expired for serialized transaction with existing signatures - cannot retry without invalidating them" + ); + let _ = self.storage.delete_attempt(transaction_id).await; + return Err(SolanaExecutorError::TransactionFailed { + reason: "Blockhash expired for serialized transaction with existing signatures. Retrying with a new blockhash would invalidate them.".to_string(), + } + .fail()); + } + // If no signatures, we can retry - will be signed during execution }
🧹 Nitpick comments (2)
solana-core/src/transaction.rs (1)
98-113: Consider validating compute budget fields for serialized transactions.Lines 107-112 allow
compute_unit_limitandcompute_unit_priceto be set even for serialized transactions. However, line 212-213 indeserialize_transactiondocuments that these fields are ignored for serialized transactions. This could confuse users who set these fields expecting them to take effect.Consider adding a validation method that warns or returns an error when compute budget fields are set for serialized transactions:
impl SolanaTransaction { + /// Validate that compute budget fields are not set for serialized transactions + pub fn validate(&self) -> Result<(), SolanaTransactionError> { + if matches!(&self.input, SolanaTransactionInput::Serialized(_)) { + if self.compute_unit_limit.is_some() || self.compute_unit_price.is_some() { + return Err(SolanaTransactionError::InvalidData { + error: "Compute budget fields are ignored for serialized transactions".to_string(), + }); + } + } + Ok(()) + } + /// Build or deserialize a VersionedTransaction pub fn to_versioned_transaction(core/src/execution_options/solana.rs (1)
141-162: Document why struct-level rename_all is disabled.Line 143 has the struct-level
#[serde(rename_all = "camelCase")]commented out, with individual field renames applied instead (lines 147, 155, 160). This is likely necessary because the#[serde(flatten)]on line 151 combined with the#[serde(untagged)]enum inSolanaTransactionInputcould cause the flattened fields (instructionsortransaction) to be incorrectly renamed.Consider adding a comment explaining this:
/// Request to send a Solana transaction #[derive(Serialize, Deserialize, Clone, Debug, utoipa::ToSchema)] -// #[serde(rename_all = "camelCase")] +// Note: rename_all is not used here because it conflicts with the flattened untagged input field. +// Individual fields are explicitly renamed to camelCase instead. pub struct SendSolanaTransactionRequest {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Cargo.toml(1 hunks)core/src/execution_options/solana.rs(1 hunks)executors/src/solana_executor/worker.rs(4 hunks)solana-core/src/transaction.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.
Applied to files:
executors/src/solana_executor/worker.rs
🧬 Code graph analysis (2)
solana-core/src/transaction.rs (1)
executors/src/solana_executor/worker.rs (1)
bincode(607-607)
executors/src/solana_executor/worker.rs (1)
solana-core/src/transaction.rs (1)
new_with_instructions(23-25)
🔇 Additional comments (8)
solana-core/src/transaction.rs (4)
22-30: LGTM!The helper constructors are clear and straightforward, providing a convenient API for creating transaction inputs.
32-40: LGTM!The input structs are simple and correctly structured for the untagged enum pattern.
154-168: LGTM!The dispatch logic cleanly separates the two input paths and delegates to appropriate helper methods.
170-209: LGTM!The instruction-based transaction building logic is correct and well-structured. Compute budget instructions are properly added first, followed by user instructions, and the message is compiled correctly.
core/src/execution_options/solana.rs (1)
129-139: LGTM!The
SolanaTransactionOptionscorrectly uses the flattened input field pattern and maintains camelCase serialization for consistency.executors/src/solana_executor/worker.rs (3)
436-446: LGTM!The signature change from
&SolanaTransactionOptionsto&[SolanaInstructionData]improves separation of concerns and makes the function more reusable.
448-480: LGTM!The new
get_compute_unit_pricemethod cleanly extracts priority fee computation logic. The default of 75th percentile for Auto mode is reasonable, and the implementation correctly handles all three priority fee variants.
749-815: Transaction building logic correctly handles both input types.The implementation properly distinguishes between instruction-based and serialized transactions:
Instructions path (lines 750-792):
- Computes priority fees based on execution options (lines 753-757)
- Crucially, adds a memo instruction with the transaction ID (lines 760-771), ensuring unique signatures even with the same blockhash during retries
- Builds transaction with compute budget options applied (lines 773-777)
Serialized path (lines 793-814):
- Correctly sets compute budget to
Noneto avoid invalidating existing signatures (lines 797-798)- Does not add memo instruction (correct, would invalidate signatures)
The memo instruction addition on line 760-771 is particularly important for preventing signature collisions during resubmissions.
| /// Input for Solana transaction - either build from instructions or use pre-built | ||
| #[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)] | ||
| #[serde(untagged)] | ||
| pub enum SolanaTransactionInput { | ||
| /// Build transaction from instructions | ||
| Instructions(SolanaTransactionInputInstructions), | ||
| /// Use pre-built serialized VersionedTransaction (base64) | ||
| Serialized(SolanaTransactionInputSerialized), | ||
| } | ||
|
|
||
| impl SolanaTransactionInput { | ||
| pub fn new_with_instructions(instructions: Vec<SolanaInstructionData>) -> Self { | ||
| Self::Instructions(SolanaTransactionInputInstructions { instructions }) | ||
| } | ||
|
|
||
| pub fn new_with_serialized(transaction: String) -> Self { | ||
| Self::Serialized(SolanaTransactionInputSerialized { transaction }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify untagged serde behavior with overlapping fields.
The #[serde(untagged)] attribute on line 14 means serde will try to deserialize into Instructions first, then Serialized. If a JSON payload contains both instructions and transaction fields, it will deserialize as Instructions and silently ignore the transaction field. This could lead to unexpected behavior if callers mistakenly provide both fields.
Run the following test to verify the deserialization behavior:
🏁 Script executed:
#!/bin/bash
# Search for any validation or tests that check for mutually exclusive fields
rg -n -A5 -B5 'SolanaTransactionInput' --type rust -g '!target/**'Length of output: 10451
Based on the web search results, with untagged enums, serde will try to match the data against each variant in order and the first one that deserializes successfully is the one returned. However, the concern in the review comment needs deeper investigation. Let me check the actual code structure for validation:
Address deserialization ambiguity with overlapping field payloads
With #[serde(untagged)], serde will try to match the data against each variant in order and the first one that deserializes successfully is the one returned. The SolanaTransactionInput enum has distinct field names (instructions vs transaction), but by default, unknown fields are ignored for self-describing formats like JSON. If a caller provides a JSON payload containing both instructions and transaction fields, it will deserialize as the Instructions variant and silently ignore the transaction field.
To prevent this ambiguity, consider either:
- Adding
#[serde(deny_unknown_fields)]toSolanaTransactionInputInstructionsandSolanaTransactionInputSerializedstructs to reject payloads with unexpected fields - Switching to an internally tagged enum representation to provide explicit variant discrimination
- Adding validation logic at the API layer to ensure callers provide exactly one field
🤖 Prompt for AI Agents
In solana-core/src/transaction.rs around lines 12 to 30, the untagged enum
SolanaTransactionInput can silently accept payloads that contain both
"instructions" and "transaction" fields because unknown fields are ignored; to
fix this, add #[serde(deny_unknown_fields)] to the
SolanaTransactionInputInstructions and SolanaTransactionInputSerialized struct
definitions so deserialization will reject payloads containing unexpected
fields, or alternatively switch the enum to an explicitly tagged representation
(e.g., internally tagged) or add explicit validation in the API layer to ensure
exactly one of the two fields is present before accepting the input.
| /// Deserialize pre-built transaction and verify fee payer | ||
| /// Note: compute_unit_limit and compute_unit_price are ignored for serialized transactions | ||
| /// as they would require adding new instructions which would invalidate existing signatures | ||
| fn deserialize_transaction( | ||
| &self, | ||
| tx_base64: &str, | ||
| expected_payer: Pubkey, | ||
| ) -> Result<VersionedTransaction, SolanaTransactionError> { | ||
| let tx_bytes = Base64Engine.decode(tx_base64).map_err(|e| { | ||
| SolanaTransactionError::DeserializationFailed { | ||
| error: format!("Invalid base64: {}", e), | ||
| } | ||
| })?; | ||
|
|
||
| // Deserialize from binary wire format using bincode | ||
| let (transaction, _): (VersionedTransaction, _) = | ||
| bincode::serde::decode_from_slice(&tx_bytes, bincode::config::standard()).map_err( | ||
| |e| SolanaTransactionError::DeserializationFailed { | ||
| error: format!("Failed to deserialize VersionedTransaction: {}", e), | ||
| }, | ||
| )?; | ||
|
|
||
| // Verify fee payer | ||
| let fee_payer = transaction.message.static_account_keys()[0]; | ||
| if fee_payer != expected_payer { | ||
| return Err(SolanaTransactionError::FeePayerMismatch { | ||
| expected: expected_payer.to_string(), | ||
| got: fee_payer.to_string(), | ||
| }); | ||
| } | ||
|
|
||
| Ok(transaction) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fee payer verification is correctly implemented.
The deserialization logic correctly:
- Decodes the base64 transaction (lines 219-223)
- Deserializes using bincode (lines 226-231)
- Verifies the fee payer matches the expected signer (lines 233-240)
The fee payer check on line 234 correctly uses the first account in static_account_keys(), which is always the fee payer in Solana's transaction model. This is an essential security check.
One consideration: The method ignores the recent_blockhash parameter for serialized transactions. Should we verify that the serialized transaction's blockhash matches the expected blockhash? Run this search to check if blockhash verification is handled elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for blockhash verification logic for serialized transactions
rg -n -C5 'Serialized.*blockhash|blockhash.*Serialized' --type rustLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search for blockhash handling in transaction code
rg -n -C3 'blockhash|recent_blockhash' --type rust -g 'transaction.rs'
Length of output: 2382
🏁 Script executed:
#!/bin/bash
# Search for where deserialize_transaction is called to see if blockhash is validated before/after
rg -n -C5 'deserialize_transaction' --type rustLength of output: 1655
🏁 Script executed:
#!/bin/bash
# Check if VersionedTransaction struct contains blockhash field
rg -n 'VersionedTransaction' --type rust -A2 -B2Length of output: 4489
🏁 Script executed:
#!/bin/bash
# Search for any transaction validation logic
rg -n 'verify|validate.*transaction|transaction.*verify' --type rust -iLength of output: 4830
🏁 Script executed:
#!/bin/bash
# Check caller context: how is to_versioned_transaction used?
rg -n 'to_versioned_transaction' --type rust -B2 -A2Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or tests about blockhash handling for serialized transactions
rg -n 'Serialized.*blockhash|blockhash.*stale|blockhash.*expired' --type rustLength of output: 124
🏁 Script executed:
#!/bin/bash
# Search for InvalidBlockhash error usage
rg -n 'InvalidBlockhash' --type rustLength of output: 74
Add blockhash validation for deserialized transactions to match Instructions path.
The deserialize_transaction method lacks validation that the deserialized transaction's blockhash matches the expected recent_blockhash, creating an inconsistency:
- Instructions path (lines 162, 195): blockhash explicitly validated during message compilation
- Serialized path (line 165):
recent_blockhashparameter is ignored, deserialized blockhash is not validated
This allows pre-built transactions with stale or mismatched blockhashes to be accepted. The unused InvalidBlockhash error variant (line 262) suggests this validation was planned but not implemented.
Required changes:
- Accept
recent_blockhashparameter indeserialize_transaction(line 214) - Compare deserialized transaction's blockhash against expected value (after line 231)
- Return
InvalidBlockhasherror if they don't match
🤖 Prompt for AI Agents
In solana-core/src/transaction.rs around lines 211 to 243,
deserialize_transaction currently only takes tx_base64 and expected_payer and
does not validate the deserialized transaction's blockhash; update the function
signature to also accept recent_blockhash (same type used elsewhere), after
deserializing and verifying fee_payer compare
transaction.message.recent_blockhash (or equivalent getter) to recent_blockhash,
and if they differ return SolanaTransactionError::InvalidBlockhash with
appropriate expected/got strings; keep the existing fee payer check and error
handling otherwise.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores