DX-272: Rust tx-sender crate #820

parhim · 2025-03-12T21:04:09Z

https://linear.app/orca-so/issue/DX-272/build-and-test-crate-with-same-specs-as-ts-package

this still needs some thoughtful review but at least it works with and without fees
jito
exact + dynamic

not as clean as typescript but thats a rust limitation
tests are doing small transfers between orca wallets (can use whatever on devnet)
but you need a locally stored mainnet-test-keypair.json (ping me on slack if you need the lightly funded orca pk)

wjthieme · 2025-03-17T03:46:48Z

Cargo.toml

@@ -1,7 +1,8 @@
 [workspace]
 resolver = "2"
 members = [
-    "programs/*"
+    "programs/*",
+    "rust-sdk/tx-sender",


We only added programs/* to the workspace to avoid version conflicts. Each SDK has their own cargo.lock file and can thus have an independent version of shared dependencies (e.g. solana-program). Can you remove?

wjthieme · 2025-03-17T03:47:07Z

rust-sdk/Cargo.toml

@@ -0,0 +1,5 @@
+[workspace]


Same goes for this one ^

wjthieme · 2025-03-17T03:47:51Z

rust-sdk/tx-sender/Cargo.toml

+edition = "2021"
+description = "Solana Transaction Sender for building and sending transactions with priority fees"
+repository = "https://github.com/orca-so/whirlpools"
+license = "Orca"


Think this should match what is in the other cargo.toml files. I think we use license-path instead

wjthieme · 2025-03-17T03:48:13Z

rust-sdk/tx-sender/Cargo.toml

+description = "Solana Transaction Sender for building and sending transactions with priority fees"
+repository = "https://github.com/orca-so/whirlpools"
+license = "Orca"
+rust-version = "1.70"


Unless this is a limitation I wouldn't add this one

wjthieme · 2025-03-17T03:50:14Z

rust-sdk/tx-sender/Cargo.toml

+rust-version = "1.70"
+
+[dependencies]
+# Use consistent versions to avoid conflicts


Cargo is kinda annoying when it comes to dependencies. Specifying "1.17.22" here will most likely pull in v1.18.x.

On the other hand we want this lib to work with as many versions of solana-program as possible. Specifying "1.17.22" in cargo means >= 1.17.22 and <2.0.0. We might want this lib to also work with >2.0 like the other libs in the repo.

wjthieme · 2025-03-17T16:28:52Z

rust-sdk/tx-sender/src/rpc_config.rs

+
+/// Chain identifier based on genesis hash
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ChainId(Hash);


Would it make sense to have a enum instead and implement From<Hash> for it?

wjthieme · 2025-03-17T16:29:16Z

rust-sdk/tx-sender/src/rpc_config.rs

+}
+
+/// RPC configuration for connecting to Solana nodes
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]


Does this need Serialize and Deserialize?

wjthieme · 2025-03-17T16:30:54Z

rust-sdk/tx-sender/src/rpc_config.rs

+    }
+
+    /// Async constructor with chain ID detection
+    pub async fn with_chain_detection(url: impl Into<String>) -> Result<Self> {


I would just only do the async constructor. Is there a scenario where we would want this without a chain_id?

wjthieme · 2025-03-17T16:31:27Z

rust-sdk/tx-sender/src/tests.rs

@@ -0,0 +1,70 @@
+#[cfg(test)]
+mod tests {


Why is this a separate file and not in-file?

wjthieme · 2025-03-17T16:31:50Z

rust-sdk/tx-sender/src/tx_config.rs

+use std::time::Duration;
+
+/// Transaction configuration for sending transactions
+#[derive(Debug, Clone, Serialize, Deserialize)]


Does this need Serialize and Deserialize?

wjthieme · 2025-03-21T18:59:00Z

rust-sdk/tx-sender/README.md

+    };
+
+    // Create sender instance
+    let sender = TransactionSender::new(rpc_config, fee_config);


Is this still accurate?

wjthieme · 2025-03-21T19:07:10Z

rust-sdk/tx-sender/src/compute_budget.rs

+
+    let transaction = Transaction::new_unsigned(message);
+    // Simulate transaction
+    let simulation_result = rpc_client.simulate_transaction(&transaction)


Does this work with an unsigned tx? I thought you had to add a param to disable sig_verify and replace_blockhash. Also can we use VersionedTransaction here?

wjthieme · 2025-03-21T19:08:40Z

rust-sdk/tx-sender/src/compute_budget.rs

+
+
+/// Calculate and return compute budget instructions for a transaction
+pub async fn add_compute_budget_instructions(


nit: maybe get_compute_budget_instruction is more appropriate here

wjthieme · 2025-03-21T19:19:05Z

rust-sdk/tx-sender/src/config.rs

+/// - `Option<RpcConfig>` - Must be explicitly set with `set_rpc()` before sending transactions
+/// - `Option<Arc<RpcClient>>` - Created when RPC is set, for reuse across transactions
+/// - `FeeConfig` - Configured with defaults but can be customized
+static GLOBAL_CONFIG: OnceLock<RwLock<GlobalConfig>> = OnceLock::new();


Do we need a OnceLock here?

wjthieme · 2025-03-21T19:21:43Z

rust-sdk/tx-sender/src/lib.rs

+    );
+
+    // Return unsigned transaction (signing happens at the higher level)
+    Ok(Transaction::new_unsigned(message))


Can we do a versioned TX?

wjthieme · 2025-03-21T19:25:53Z

rust-sdk/tx-sender/src/lib.rs

+    let expiry_time = Instant::now() + Duration::from_millis(options.timeout_ms);
+    let mut retries = 0;
+
+    while Instant::now() < expiry_time && retries <= options.max_retries {


I am still kinda concerned about the max_retries.

I think a default of 3 is too low

After the last retry there is still a fairly decent chance that the transaction lands (but we won't pick picked up potentially because of the timeout_ms).

I think it would be safest to add the same logic here as in the TS sdk.

loop until expiry_time: send TX see if TX is confirmed Wait 1s

wjthieme · 2025-03-21T19:26:20Z

rust-sdk/tx-sender/src/lib.rs

+
+    while Instant::now() < expiry_time && retries <= options.max_retries {
+        let send_config = RpcSendTransactionConfig {
+            skip_preflight: options.skip_preflight,


With client retries skip_preflight needs to be always true. Don't think it makes sense to make this configurable by the user.

wjthieme · 2025-03-21T19:28:02Z

rust-sdk/tx-sender/src/rpc_config.rs

+impl From<Hash> for ChainId {
+    fn from(hash: Hash) -> Self {
+        // Mainnet genesis hash
+        let mainnet_hash = Hash::from_str(MAINNET_HASH).unwrap_or_default();


do we need to do the from_str here every time? Can we instead do hash.to_string()?

wjthieme

One comment about timeout and retries but other than that lgtm 🚀

wjthieme · 2025-03-25T13:21:01Z

rust-sdk/tx-sender/src/config.rs

+pub const DEFAULT_TRANSACTION_TIMEOUT_MS: u64 = 30_000;
+
+/// Default number of retries
+pub const DEFAULT_RETRIES: usize = 3;


Are these still being used?

wjthieme · 2025-03-25T13:23:56Z

rust-sdk/tx-sender/src/lib.rs

+        return Err(format!("Transaction simulation failed: {}", err));
+    }
+
+    let expiry_time = Instant::now() + Duration::from_millis(options.timeout_ms);


Think the default for this should be 90 seconds instead of 30 (or just make it not configurable like on TS). The issue with lowering the timeout is that the transaction is valid for the full 90 seconds. This means that if it ends up landing on the final send this function never picks up on it. If you set the timeout to 30 seconds and it 'times-out' you can't definitively say that the transaction did not land

…bust sending

parhim added 4 commits March 6, 2025 10:39

first pass

7b4f71a

some changes

07751f8

tests working

5fe84e6

readme update

f796c3c

parhim requested review from yugure-orca, wjthieme and odcheung as code owners March 12, 2025 21:04

parhim added 2 commits March 14, 2025 15:31

downgraded to solana 1.17.22

a3b6027

Merge branch 'main' into sam/DX-272

8955c95

wjthieme reviewed Mar 17, 2025

View reviewed changes

parhim added 12 commits March 18, 2025 17:01

reduce dependenices and simplify errors

d9a0589

passes reference to rpc client instead of creating one every time

816da6a

uses strict struct instead of serializable for fee config

893b69e

struct for jito tip response

f1c18d9

simpler struct for rpc config

d80b375

rpc config always with chain detection

33a61b3

transaction config strict struct

e800674

updated architechture with global config

c87455b

updated tests

5aaa607

Merge branch 'main' into sam/DX-272

3f4dfdc

rm network name

6c0b1f5

Merge branch 'main' into sam/DX-272

a795174

wjthieme reviewed Mar 21, 2025

View reviewed changes

parhim added 3 commits March 24, 2025 14:29

some comments

7b3d398

addresses all comments (keeps oncelock)

f172d7b

Merge branch 'main' into sam/DX-272

61237f0

wjthieme approved these changes Mar 25, 2025

View reviewed changes

parhim added 2 commits March 25, 2025 09:09

updated to only accept commitment level (no more timeout) and more ro…

5844b87

…bust sending

Merge branch 'main' into sam/DX-272

a9eb313

parhim merged commit 9ced62d into main Mar 25, 2025
6 checks passed

parhim deleted the sam/DX-272 branch March 25, 2025 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DX-272: Rust tx-sender crate #820

DX-272: Rust tx-sender crate #820

parhim commented Mar 12, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 17, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme Mar 21, 2025

wjthieme left a comment

wjthieme Mar 25, 2025

wjthieme Mar 25, 2025



		/// Calculate and return compute budget instructions for a transaction
		pub async fn add_compute_budget_instructions(

DX-272: Rust tx-sender crate #820

DX-272: Rust tx-sender crate #820

Conversation

parhim commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjthieme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment