feat: add pay sign command and MCP transaction signing#368
feat: add pay sign command and MCP transaction signing#368berkayoztunc wants to merge 2 commits into
Conversation
|
@berkayoztunc is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds
Confidence Score: 4/5The signing flow is functionally correct and well-tested; the main rough edges are design-level rather than user-visible bugs. The core logic is sound: signer-slot lookup, signature preservation, and pre-submission validation (bad base64, bad bytes, wrong signer) all work correctly and are covered by tests. The three findings are design concerns: the TransactionSubmitter::submit trait being sync while called from an async fn (safe today via block_on, fragile if the call site changes), the confusing (requested, enforced) parameter order in resolve_network/resolve_account (enforced silently wins despite being second), and the absence of a pre-submission check for still-zeroed co-signer slots (which produces an opaque RPC error instead of a clear local message). None of these break current behaviour. rust/crates/core/src/client/sign.rs and rust/crates/mcp/src/tools/sign_transaction.rs warrant a second look for the async/blocking design and parameter ordering. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as pay sign CLI
participant MCP as MCP sign_transaction
participant Core as pay_core::sign
participant KS as Keystore / Signer
participant RPC as Solana RPC
User->>CLI: pay sign BASE64_TX
CLI->>Core: sign_and_submit_base64_transaction(b64, network, account)
Core->>KS: load_signer_for_network_with_intent()
KS-->>Core: SolanaSigner
Core->>Core: decode_base64_transaction(b64)
Core->>Core: sign_transaction(tx, signer)
Note over Core: sanitize() → find signer slot → sign_message()
Core->>RPC: send_and_confirm_transaction(tx)
RPC-->>Core: Signature
Core-->>CLI: signature string
CLI-->>User: prints signature
User->>MCP: "sign_transaction({transaction, network, account})"
MCP->>MCP: resolve env vars (PAY_NETWORK_ENFORCED, PAY_ACTIVE_ACCOUNT)
MCP->>Core: spawn_blocking → sign_and_submit_base64_transaction(...)
Core->>KS: load_signer_for_network_with_intent()
KS-->>Core: SolanaSigner
Core->>RPC: send_and_confirm_transaction(tx)
RPC-->>Core: Signature
Core-->>MCP: signature string
MCP-->>User: CallToolResult::success(signature)
Reviews (1): Last reviewed commit: "fix signer" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn resolve_network(requested: Option<&str>, enforced: Option<&str>) -> String { | ||
| normalize(enforced) | ||
| .or_else(|| normalize(requested)) | ||
| .unwrap_or_else(|| pay_core::accounts::MAINNET_NETWORK.to_string()) | ||
| } | ||
|
|
||
| fn resolve_account(requested: Option<&str>, enforced: Option<&str>) -> Option<String> { | ||
| normalize(enforced).or_else(|| normalize(requested)) | ||
| } |
There was a problem hiding this comment.
enforced silently wins despite being second
resolve_network and resolve_account both list requested first and enforced second, but the implementation silently prioritises the second argument. A caller who passes (enforced_value, requested_value) by mistake (or a future maintainer who reads the signature without reading the body) gets the wrong priority with no compile-time or runtime warning. Putting the higher-priority argument first, or renaming them (caller_hint, env_override), would remove the footgun.
| async fn sign_and_submit_with_submitter( | ||
| transaction_base64: &str, | ||
| signer: &dyn SolanaSigner, | ||
| submitter: &dyn TransactionSubmitter, | ||
| ) -> Result<String> { | ||
| let transaction = decode_base64_transaction(transaction_base64)?; | ||
| let transaction = sign_transaction(transaction, signer).await?; | ||
| submitter.submit(&transaction) | ||
| } |
There was a problem hiding this comment.
submit() called directly inside an async fn
sign_and_submit_with_submitter is async, but submitter.submit() is a synchronous blocking call (RpcClient::send_and_confirm_transaction is blocking). In the current code path this is safe because the function is always driven via rt.block_on(...) on a spawn_blocking thread, so there is no async executor to starve. However, the TransactionSubmitter trait's sync surface makes the design fragile — if sign_and_submit_with_submitter is ever called from a regular async context (e.g. directly awaited from the MCP handler), the blocking RPC call would hold an executor thread. Making submit async on the trait, or wrapping the call in tokio::task::spawn_blocking, would make the design self-consistent.
| async fn sign_transaction( | ||
| mut transaction: VersionedTransaction, | ||
| signer: &dyn SolanaSigner, | ||
| ) -> Result<VersionedTransaction> { | ||
| transaction | ||
| .sanitize() | ||
| .map_err(|e| Error::Config(format!("Invalid Solana transaction: {e}")))?; | ||
|
|
||
| let signer_pubkey = signer.pubkey(); | ||
| let required_signatures = usize::from(transaction.message.header().num_required_signatures); | ||
| let required_signer_keys = transaction | ||
| .message | ||
| .static_account_keys() | ||
| .get(..required_signatures) | ||
| .ok_or_else(|| { | ||
| Error::Config( | ||
| "Invalid Solana transaction: required signer keys are missing".to_string(), | ||
| ) | ||
| })?; | ||
| let signature_index = required_signer_keys | ||
| .iter() | ||
| .position(|pubkey| pubkey == &signer_pubkey) | ||
| .ok_or_else(|| { | ||
| Error::Config(format!( | ||
| "Selected pay account `{signer_pubkey}` is not a required signer for this transaction" | ||
| )) | ||
| })?; | ||
|
|
||
| let signature_bytes = signer | ||
| .sign_message(&transaction.message.serialize()) | ||
| .await | ||
| .map_err(|e| Error::Mpp(format!("transaction signing failed: {e}")))?; | ||
| transaction.signatures[signature_index] = Signature::from(<[u8; 64]>::from(signature_bytes)); | ||
|
|
||
| Ok(transaction) |
There was a problem hiding this comment.
sign_transaction fills in only the Pay account's signature slot and leaves every other slot as Signature::default() (all-zeroes). If the transaction requires additional co-signers that have not yet signed, the function submits the transaction to RPC and gets back a validator error about missing signatures rather than a clear, local error. A pre-submission guard that verifies no required slot remains at Signature::default() after the Pay account signs would give users a much clearer message (e.g. "transaction still needs signatures from [pubkeys]") instead of an opaque RPC rejection.
1 participant
Summary
Add Solana transaction signing and submission support to Pay through both the CLI and MCP.
Changes
add CLI command:
add MCP tool:
support base64-encoded legacy and v0 Solana transactions
sign with the selected Pay account and submit the transaction through RPC
preserve existing signatures while updating the Pay signer slot
print or return the confirmed Solana transaction signature on success
update Pay MCP instructions and Codex/Claude MCP tool allowlists
add tests for CLI parsing, transaction decode/signing, signer validation, RPC submission behavior, and MCP parameter routing
make MCP curl binary-response tempfile creation collision-safe under parallel tests
Why
Pay already manages user accounts and wallet-backed authorization. This adds a direct way for users and agents to sign externally prepared Solana transactions with a Pay account and submit them without exposing private keys.
CLI
The command:
MCP
The new
sign_transactiontool accepts:{ "transaction": "<BASE64_TX>", "network": "mainnet", "account": "optional-account-name" }The MCP flow reuses the same core signing and submission logic as the CLI.
It also honors existing MCP routing through:
PAY_NETWORK_ENFORCEDPAY_ACTIVE_ACCOUNTPAY_RPC_URLSafety and errors
The signing flow rejects before RPC submission when:
Out of scope
This PR does not add:
Validation
cargo test -p pay-mcp --libcargo test -p pay codexcargo test -p pay allowed_toolsjust rs fmtjust rs lintjust rs unit-testjust ci