-
Notifications
You must be signed in to change notification settings - Fork 307
[AMM-164] remove rust and ts tx-sender libraries #1211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| { | ||
| href: "https://orca-so.github.io/tx-sender/rust/orca_tx_sender", | ||
| label: "Rust TX Sender", | ||
| target: "_blank", | ||
| }, | ||
| { | ||
| href: "https://orca-so.github.io/tx-sender/ts", | ||
| label: "TS TX Sender", | ||
| target: "_blank", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these links already work 🙂
site published to gh pages here https://orca-so.github.io/tx-sender
calintje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
Aside from the inline comment I left, one small docs note that might be worth updating:
In docs/whirlpool/docs/03-SDKs/06-Send Transaction.mdx, the install snippet still shows older versions: orca_tx_sender ^0.1.0 and @orca-so/tx-sender ^1.0.0
| }, | ||
| "dependencies": { | ||
| "@orca-so/tx-sender": "*", | ||
| "@orca-so/tx-sender": "^3.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orca-so/tx-sender imports @solana/rpc at runtime, but @solana/rpc is a peer of tx-sender. If a consumer pulls in @orca-so/whirlpools (which depends on + re-exports tx-sender config setRpc) without explicitly providing the peer chain, package managers that strictly enforce peer deps can fail fast.
I think this was mostly masked in the monorepo by workspace hoisting/shared deps.
Test
Create a fresh directory with a minimal package.json:
{
"name": "whirlpools-peer-test",
"private": true,
"type": "module",
"packageManager": "yarn@4.6.0",
"dependencies": {
"@orca-so/whirlpools": "^6.0.0",
"@solana/kit": "^5.0.0"
}
}Create index.mjs:
import { setRpc } from "@orca-so/whirlpools";
await setRpc("https://api.devnet.solana.com");
console.log("setRpc ok");Install and run:
yarn install
yarn node index.mjsYou should see an error similar to:
@orca-so/tx-sender tried to access @solana/rpc (a peer dependency) but it isn't provided by its ancestors
In the stack trace I saw, the peer chain was reported as breaking at @orca-so/whirlpools.
After installing @solana/rpc, I hit another peer gap: @solana/rpc-subscriptions-channel-websocket expects ws.
yarn add @solana/rpc@^5.0.0
yarn node index.mjsError looked like:
@solana/rpc-subscriptions-channel-websocket tried to access ws (a peer dependency) but it isn't provided by its ancestors
Installing ws made it run:
yarn add ws@^8
yarn node index.mjsSuggestion
It may be worth making this less “peer-chasey” for consumers by treating the runtime-required pieces as regular dependencies at the layer that imports them, rather than peers that apps must discover:
- either make
wsand@solana/rpcregular dependencies where they are used - or, if we keep them as peers, have
@orca-so/whirlpoolsalso declare/pass-through those peer requirements so consumers are prompted to install them
It would also help if the Next.js example declared these dependencies explicitly, so they behave like real-world consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great catch — i ran into this same issue when porting over to the new repository. for the purpose of tests, i just added to dev deps, but i think you make a good point that we should improve the general ergonomics.
i saw that @orca-so/whirlpools exposes @solana/kit as a peer dependency (source), so maybe it makes sense to add @solana/rpc there as well?
i'm not sure where we should draw the line though. i see there are other solana packages as normal dependencies. i could see an argument for exposing some of those as peer deps (e.g. token program packages), but i'm not sure what the threshold is for moving from normal deps to peer deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m also not sure where the threshold should be. An alternative solution for this issue specifically might be to remove the dependency on @solana/rpc entirely. As far as I can tell, this is the only place it’s currently used:
whirlpools/ts-sdk/tx-sender/src/compatibility.ts
Lines 1 to 25 in 6ee54a9
| import { createSolanaRpcApi, createRpc } from "@solana/rpc"; | |
| import type { Address, Rpc, SolanaRpcApi } from "@solana/kit"; | |
| import { createDefaultRpcTransport, address } from "@solana/kit"; | |
| /** | |
| * Creates an RPC client instance for interacting with the SVM blockchains using the provided RPC URL. | |
| * | |
| * @param {string} url - The RPC endpoint URL. | |
| * @returns {Rpc<SolanaRpcApi>} An RPC client configured with the Solana RPC API. | |
| * | |
| * @example | |
| * ```ts | |
| * const rpc = rpcFromUrl("https://api.mainnet-beta.solana.com"); | |
| * const slot = await rpc.getSlot().send(); | |
| * console.log(slot); | |
| * ``` | |
| */ | |
| export function rpcFromUrl(url: string): Rpc<SolanaRpcApi> { | |
| const api = createSolanaRpcApi({ | |
| defaultCommitment: "confirmed", | |
| }); | |
| const transport = createDefaultRpcTransport({ url }); | |
| const rpc = createRpc({ api, transport }); | |
| return rpc; | |
| } |
Maybe inside rpcFromUrl we could use createSolanaRpc from @solana/kit instead? I’m not certain whether there was a reason not to use that originally, so this suggestion may be missing some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ope, good catch. i will update this! 🎣 |
calintje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()


Description
Remove tx-sender from rust-sdk and ts-sdk in favor of a dedicated repository: orca-so/tx-sender#1
Testing