[ FIX ] Fix bugs for #1837 and enable feePayer again #5
Conversation
…er and WhileValidTxSender to ensure correct fee payer assignment and transaction handling This commit introduces a new test script that validates the functionality of the RetryTxSender and WhileValidTxSender classes. It verifies that transactions are sent correctly with the appropriate fee payer, both when using a separate fee payer and when the authority is also the fee payer. fix(driftClient.ts): update payer assignment to use wallet.payer if available for better flexibility fix(txHandler.ts): update feePayer assignment to use wallet.payer if available for improved compatibility feat(whileValidTxSender.ts): add optional timeout parameter to WhileValidTxSender for enhanced control over transaction sending feat(wallet.ts): refactor Wallet class to allow optional payer and improve signing logic style(helpers.ts): format empty async methods for consistency in MockUserMap class
WalkthroughIntroduces a separate optional payer concept (distinct from authority) across the SDK, updates transaction preparation and signing to prefer an explicit payer fallback, removes direct wallet.payer signing in versioned paths, adds timeout support to WhileValidTxSender, and adds a new devnet test script exercising RetryTxSender and WhileValidTxSender. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Test / Caller
participant DriftClient as DriftClient / Builders
participant TxHandler as TxHandler
participant Wallet as Wallet
participant RPC as Solana RPC
Note over App,DriftClient: New flow uses explicit payer fallback
App->>DriftClient: build transaction (maybe overrides.payer)
DriftClient->>TxHandler: prepareTx(tx, wallet)
Note right of TxHandler: feePayer = overrides?.payer<br/>?? wallet.payer?.publicKey<br/>?? wallet.publicKey
TxHandler->>Wallet: signVersionedTransaction(tx) or signTransaction(tx)
alt wallet.payer exists && differs from authority
Wallet->>Wallet: include [payer, authority] signatures
else
Wallet->>Wallet: sign with [authority] only
end
Wallet->>RPC: sendRawTransaction(signed tx)
RPC-->>App: tx signature / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multiple interdependent files changed (wallet, tx handlers, instruction builders, senders, and new test), with moderate logic density and cross-cutting signer/payer semantics that require consistency checks. Poem
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: 3
🧹 Nitpick comments (6)
sdk/src/driftClient.ts (3)
1436-1452: Centralize payer resolution to avoid duplicationYou repeat the same payer fallback across methods. Extract a helper to reduce drift and future mistakes.
+// near other private helpers +private resolvePayer(overridesPayer?: PublicKey): PublicKey { + return overridesPayer ?? this.wallet.payer?.publicKey ?? this.wallet.publicKey; +}Then here:
-const payer = - overrides?.payer ?? this.wallet.payer?.publicKey ?? this.wallet.publicKey; +const payer = this.resolvePayer(overrides?.payer);Apply similarly to other ixs that set payer.
7018-7026: Sanity check: signMessage default keypair (not new but related)If wallet.payer is undefined (e.g., adapter wallets), default param
keypair: Keypair = this.wallet.payerwill be undefined at runtime. Consider throwing a clear error or supporting a wallet message-signing path.
5900-5914: Optional: align ATA creation payer with resolvePayerWhen creating ATAs (idempotent), you pass provider.wallet.publicKey as payer. Consider using resolvePayer() for consistency with separate fee payer flows.
Also applies to: 5929-5938
sdk/scripts/tx-sender-test.ts (3)
24-63: Consider adding retry logic for transaction verification.The function throws immediately if
getParsedTransactionreturns null (line 38-40). On devnet, transactions might take longer to become available. Consider adding a retry loop with exponential backoff.Apply this diff to add retry logic:
async function verifyTransactionFeePayer( connection: Connection, txSig: string, expectedFeePayer: PublicKey ): Promise<void> { console.log('🔍 Verifying transaction details...'); - await new Promise((resolve) => setTimeout(resolve, 2000)); - - const tx = await connection.getParsedTransaction(txSig, { - commitment: 'confirmed', - maxSupportedTransactionVersion: 0, - }); - - if (!tx) { - throw new Error(`Transaction ${txSig} not found`); - } + let tx = null; + const maxRetries = 5; + for (let i = 0; i < maxRetries; i++) { + await new Promise((resolve) => setTimeout(resolve, 2000)); + tx = await connection.getParsedTransaction(txSig, { + commitment: 'confirmed', + maxSupportedTransactionVersion: 0, + }); + if (tx) break; + console.log(` Retry ${i + 1}/${maxRetries}...`); + } + + if (!tx) { + throw new Error(`Transaction ${txSig} not found after ${maxRetries} retries`); + }
79-79: Document or add defensive check for wallet.payer assumption.Line 79 uses the non-null assertion operator (
wallet.payer!.publicKey). While safe in the current test context, consider adding a defensive check or a comment documenting that this test requires a wallet with a payer.Apply this diff to add a defensive check:
async function testRetryTxSender(wallet: Wallet, connection: Connection) { console.log('\n========================================'); console.log('Testing RetryTxSender'); console.log('========================================\n'); + if (!wallet.payer) { + throw new Error('Test requires wallet with payer'); + } + const retryTxSender = new RetryTxSender({ connection, wallet, }); - const expectedFeePayer = wallet.payer!.publicKey; + const expectedFeePayer = wallet.payer.publicKey;
164-164: Document or add defensive check for wallet.payer assumption.Similar to
testRetryTxSender, line 164 uses the non-null assertion operator. Consider adding the same defensive check as suggested for the previous function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
sdk/scripts/tx-sender-test.ts(1 hunks)sdk/src/driftClient.ts(14 hunks)sdk/src/tx/baseTxSender.ts(0 hunks)sdk/src/tx/txHandler.ts(6 hunks)sdk/src/tx/whileValidTxSender.ts(4 hunks)sdk/src/wallet.ts(2 hunks)sdk/tests/dlob/helpers.ts(2 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/tx/baseTxSender.ts
🧰 Additional context used
🧬 Code graph analysis (2)
sdk/tests/dlob/helpers.ts (1)
sdk/src/types.ts (1)
OrderRecord(609-613)
sdk/scripts/tx-sender-test.ts (4)
sdk/src/wallet.ts (1)
Wallet(10-79)sdk/src/tx/retryTxSender.ts (1)
RetryTxSender(15-135)sdk/src/tx/whileValidTxSender.ts (1)
WhileValidTxSender(22-262)sdk/src/keypair.ts (1)
loadKeypair(5-24)
🔇 Additional comments (24)
sdk/tests/dlob/helpers.ts (1)
675-675: LGTM! Formatting improvements for consistency.The empty method bodies now use consistent formatting (
{}without internal spaces), improving code style uniformity across the MockUserMap test helper class.Also applies to: 677-677, 736-736
sdk/src/driftClient.ts (13)
1116-1126: Payer fallback for initializeUserStats looks goodUsing wallet.payer?.publicKey with fallback preserves backward compatibility. Please verify on-chain doesn’t require payer==authority for this ix.
1156-1165: SignedMsg user orders init payer fallbackLGTM; consistent with the new payer resolution.
1197-1205: Resize SignedMsg orders payer fallbackLooks correct and consistent.
1271-1282: initializeRevenueShare: explicit payer override + fallbackNice addition of overrides?.payer with sane defaults.
Consider adding a brief JSDoc noting the override precedence.
1310-1326: initializeRevenueShareEscrow: payer override + fallbackConsistent with the pattern; good.
1350-1357: migrateReferrer: payer fallbackLGTM.
1383-1389: resizeRevenueShareEscrowOrders: payer fallbackLooks good.
1545-1551: initializeFuelOverflow: payer fallbackConsistent; no issues spotted.
1628-1636: initializeUser: payer fallbackLGTM. Same caveat: ensure program doesn’t enforce payer==authority.
1679-1686: initializeReferrerName: payer fallbackLooks correct.
9461-9472: initializeInsuranceFundStake: payer fallbackGood update; consistent with other init flows.
10552-10559: Fee payer behavior verified as uniformConfirmed. The txHandler already sets fee payer to
wallet.payer?.publicKey ?? wallet.publicKeyacross all transaction send paths (lines 194, 656, 696-697), matching the updated behavior in driftClient.ts line 10555. No inconsistencies found.
1120-1124: Verification complete: all payer sites correctly use the fallback patternThe repo-wide check confirms that all
payer:account entries insdk/src/driftClient.tsproperly implement the fallback chain. All 12 actual payer assignments follow one of these correct patterns:
payer: this.wallet.payer?.publicKey ?? this.wallet.publicKey,payer: overrides?.payer ?? this.wallet.payer?.publicKey ?? this.wallet.publicKey,The inverse match pattern (command #2) returned no results, confirming no non-compliant entries exist. The only other occurrence at line 2829 is a function parameter declaration (
createAssociatedTokenAccountIdempotentInstruction), not an account assignment, so it's appropriately excluded.No missed spots or regressions found.
sdk/src/tx/txHandler.ts (4)
194-194: LGTM! Fee payer resolution correctly handles optional payer.The fallback pattern
wallet.payer?.publicKey ?? wallet.publicKeyproperly supports both separate fee payer and authority-as-payer scenarios.
403-403: LGTM! Payer key resolution is consistent.Both
generateLegacyVersionedTransactionandgenerateVersionedTransactioncorrectly use the fallback pattern forpayerKeywhen constructingTransactionMessage.Also applies to: 425-425
651-656: LGTM! Wallet resolution and fee payer assignment are correct.The addition of
[wallet] = this.getProps(wallet)at line 651 ensures wallet is properly resolved before setting fee payers in the loop. The fee payer assignment at line 656 correctly uses the fallback pattern.
696-697: LGTM! Fee payer assignment for legacy transactions is correct.The code correctly sets
feePayeronly for legacy transactions using the fallback pattern. Versioned transactions handle payer during construction, so this differentiation is appropriate.sdk/src/wallet.ts (6)
11-18: LGTM! Wallet constructor properly implements payer fallback.The constructor correctly accepts an optional payer and defaults to authority when not provided. The
payerfield is marked optional but always has a value, which aligns with the fallback pattern used throughout the codebase.
20-30: LGTM! Transaction signing correctly handles payer/authority scenarios.The conditional signing logic properly signs with both payer and authority when they differ, or just authority when they're the same. The public key comparison using
toBase58()is appropriate.
32-44: LGTM! Versioned transaction signing is consistent with legacy signing.The signing logic mirrors
signTransactionbut correctly uses the VersionedTransaction API (tx.signwith array of signers). Consistent and correct.
46-58: LGTM! Batch transaction signing maintains consistent logic.The batch signing correctly applies the same conditional payer/authority logic to each transaction. Implementation is sound.
60-74: LGTM! New batch versioned transaction signing method completes the API.The new
signAllVersionedTransactionsmethod provides consistent batch signing for versioned transactions, filling a necessary gap in the wallet API. The logic aligns with other signing methods.
76-78: LGTM! Public key and WalletV2 updates align with authority-centric model.The
publicKeygetter now returnsauthority.publicKey, establishing authority as the primary wallet identity. WalletV2 correctly delegates to the base class and uses authority for message signing. All changes are logically consistent.Also applies to: 82-88
| const signedTx = (await wallet.signVersionedTransaction( | ||
| tx | ||
| )) as VersionedTransaction; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify IWallet interface includes signVersionedTransaction.
The call to wallet.signVersionedTransaction(tx) is correct given the wallet refactor, but the @ts-ignore suggests the IWallet interface may not include this method. Please confirm the interface is updated.
🏁 Script executed:
#!/bin/bash
# Check if IWallet interface includes signVersionedTransaction method
rg -n "interface IWallet" -A 20 sdk/src/types.tsLength of output: 1299
I need to see the full context of the signVersionedTx method to understand the wallet parameter type.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the signVersionedTx method and show its full signature and context
rg -n "signVersionedTx" sdk/src/tx/txHandler.ts -B 5 -A 10
Length of output: 417
---
🏁 Script executed:
```shell
#!/bin/bash
# Check getProps method to see what it returns
rg -n "getProps" sdk/src/tx/txHandler.ts -B 2 -A 8
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Find callers of signVersionedTx to see what wallet type is passed
rg -n "signVersionedTx\(" sdk/src -B 2 -A 2Length of output: 849
Change wallet parameter type from IWallet to IVersionedWallet.
The method signature declares wallet?: IWallet but calls wallet.signVersionedTransaction(), which only exists on IVersionedWallet. The @ts-ignore is masking a type safety issue. Change the parameter type to IVersionedWallet to match the actual implementation requirements.
🤖 Prompt for AI Agents
In sdk/src/tx/txHandler.ts around lines 299 to 301, the function parameter
currently typed as wallet?: IWallet is incorrect because
signVersionedTransaction is only on IVersionedWallet; change the parameter type
to IVersionedWallet (and keep optionality only if callers can legitimately pass
undefined), remove the @ts-ignore, update any import to bring in
IVersionedWallet, and adjust/update any call sites that pass an IWallet to
instead pass an IVersionedWallet or handle conversion/null case so the call to
wallet.signVersionedTransaction remains type-safe.
| connection, | ||
| wallet, | ||
| opts = { ...DEFAULT_CONFIRMATION_OPTS, maxRetries: 0 }, | ||
| timeout, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify timeout defaults to prevent undefined in arithmetic operations.
The timeout parameter is now optional with no default value. At line 108, this.timeout = timeout could set it to undefined. Line 220 uses this.timeout in an arithmetic comparison (this.getTimestamp() - startTime < this.timeout). If timeout is undefined, this comparison will use NaN and may break the retry logic.
Run the following script to check if BaseTxSender provides a default timeout:
Also applies to: 78-78, 94-94, 108-108
🏁 Script executed:
#!/bin/bash
# Check if BaseTxSender constructor provides default timeout value
rg -n "class BaseTxSender" -A 50 sdk/src/tx/baseTxSender.ts | rg -A 20 "constructor"Length of output: 676
---
🏁 Script executed:
```shell
#!/bin/bash
# View WhileValidTxSender class structure, constructor, and line 108 context
rg -n "class WhileValidTxSender" -A 100 sdk/src/tx/whileValidTxSender.ts | head -120
Length of output: 2830
🏁 Script executed:
#!/bin/bash
# Check line 108 and surrounding context more specifically
sed -n '100,130p' sdk/src/tx/whileValidTxSender.tsLength of output: 748
🏁 Script executed:
#!/bin/bash
# Check if line 220 uses this.timeout and what timeout value it gets
sed -n '215,225p' sdk/src/tx/whileValidTxSender.tsLength of output: 276
🏁 Script executed:
#!/bin/bash
# Search for DEFAULT_TIMEOUT definition
rg -n "DEFAULT_TIMEOUT" sdk/src/tx/Length of output: 506
🏁 Script executed:
#!/bin/bash
# Check what this.timeout refers to - own property or inherited
rg -n "this\.timeout" sdk/src/tx/whileValidTxSender.tsLength of output: 102
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if DEFAULT_TIMEOUT is imported or exported in whileValidTxSender.ts
head -30 sdk/src/tx/whileValidTxSender.ts
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Verify the exact line 220 condition to confirm the arithmetic issue
sed -n '218,222p' sdk/src/tx/whileValidTxSender.tsLength of output: 143
Add default timeout value to prevent undefined arithmetic at line 220.
The timeout parameter lacks a default value. At line 108, this.timeout = timeout assigns undefined if the parameter isn't provided. This causes the while loop condition at line 220 (this.getTimestamp() - startTime < this.timeout) to evaluate with NaN, breaking the retry logic.
Although BaseTxSender provides a DEFAULT_TIMEOUT, WhileValidTxSender declares its own timeout property (line 26) and directly assigns to it without fallback.
Fix: Add a default value at line 63 or provide a fallback at line 108. For consistency with other senders, import and use DEFAULT_TIMEOUT from baseTxSender.ts or define locally.
🤖 Prompt for AI Agents
In sdk/src/tx/whileValidTxSender.ts around line 63, the constructor parameter
`timeout` is missing a default and `this.timeout = timeout` (line 108) can
become undefined, causing NaN during arithmetic at the retry loop; fix by either
setting a default value on the parameter at line 63 (e.g. timeout =
DEFAULT_TIMEOUT) or, if you prefer not to change the signature, import
DEFAULT_TIMEOUT from baseTxSender.ts and change the assignment at line 108 to
`this.timeout = timeout ?? DEFAULT_TIMEOUT` so `this.timeout` always has a
numeric fallback.
posaune0423
changed the title
posaune0423
changed the title
posaune0423
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/driftClient.ts (1)
1241-1246: Authority param ignored during WS delegates initYou accept an authority: PublicKey param but set accounts.authority to this.wallet.publicKey. That prevents initializing for a passed-in authority and is inconsistent with other builders.
- authority: this.wallet.publicKey, + authority,Please confirm the program expects the specified authority to sign, and adjust calling sites as needed.
🧹 Nitpick comments (2)
sdk/src/driftClient.ts (2)
5906-5914: Also route ATA creation costs to the payer fallbackThese createAssociatedTokenAccountIdempotentInstruction calls still use provider.wallet.publicKey as the payer, which will charge the authority rather than the delegated payer. Align with the new payer fallback to avoid unexpected lamport spend from authority.
- this.createAssociatedTokenAccountIdempotentInstruction( - outAssociatedTokenAccount, - this.provider.wallet.publicKey, - this.provider.wallet.publicKey, - outMarket.mint, - tokenProgram - ) + this.createAssociatedTokenAccountIdempotentInstruction( + outAssociatedTokenAccount, + this.wallet.payer?.publicKey ?? this.provider.wallet.publicKey, + this.provider.wallet.publicKey, + outMarket.mint, + tokenProgram + )- this.createAssociatedTokenAccountIdempotentInstruction( - inAssociatedTokenAccount, - this.provider.wallet.publicKey, - this.provider.wallet.publicKey, - inMarket.mint, - tokenProgram - ) + this.createAssociatedTokenAccountIdempotentInstruction( + inAssociatedTokenAccount, + this.wallet.payer?.publicKey ?? this.provider.wallet.publicKey, + this.provider.wallet.publicKey, + inMarket.mint, + tokenProgram + )And similarly in getJupiterLiquidateSpotWithSwapIxV6:
- this.createAssociatedTokenAccountIdempotentInstruction( - liabilityTokenAccount, - this.provider.wallet.publicKey, - this.provider.wallet.publicKey, - liabilityMarket.mint, - tokenProgram - ) + this.createAssociatedTokenAccountIdempotentInstruction( + liabilityTokenAccount, + this.wallet.payer?.publicKey ?? this.provider.wallet.publicKey, + this.provider.wallet.publicKey, + liabilityMarket.mint, + tokenProgram + )- this.createAssociatedTokenAccountIdempotentInstruction( - assetTokenAccount, - this.provider.wallet.publicKey, - this.provider.wallet.publicKey, - assetMarket.mint, - tokenProgram - ) + this.createAssociatedTokenAccountIdempotentInstruction( + assetTokenAccount, + this.wallet.payer?.publicKey ?? this.provider.wallet.publicKey, + this.provider.wallet.publicKey, + assetMarket.mint, + tokenProgram + )Also applies to: 5930-5937, 8613-8619, 8633-8639
1121-1121: Verification complete: payer/fee-payer wiring is correct; update stale commentThe signing path already covers both authority and payer: wallet.ts (lines 21–43) conditionally adds the payer's signature when payer ≠ authority, and txHandler correctly sets feePayer before invoking signing methods. The payer fallback pattern at lines 1121, 1161, 1202, etc., is consistent and sound.
Apply this optional comment update to reflect the now-separated authority and payer roles:
- userStats: getUserStatsAccountPublicKey( - this.program.programId, - this.wallet.publicKey // only allow payer to initialize own user stats account - ), + userStats: getUserStatsAccountPublicKey( + this.program.programId, + this.wallet.publicKey // authority that owns the stats; payer may be delegated + ),
Summary
This PR is fixing the signing error with drift-labs#1837 make fee payer function available. Please look at the original PR for the changes about fee-payer.
Additional changes
driftClientworks correctly.Walletinterface keeping backward compatibilitySummary by CodeRabbit
New Features
Improvements