Skip to content

Commit

Permalink
fix: downshift preflightCommitment to processed when bypassing pr…
Browse files Browse the repository at this point in the history
…eflight checks (#2415)

# Summary

There's a bug in the `sendTransaction` RPC method where, when bypassing preflight, we nevertheless materially use the value of `preflightCommitment` to determine how to behave when sending the transaction.

If you supply _nothing_ – as you might think appropriate when _skipping_ preflight – then the default of `finalized` will be used. Far from irrelevant, such a value can actually affect the retry behaviour of the send-transaction-service (STS). Read anza-xyz/agave#483 for more detail.

In this PR, we try to get ahead of anza-xyz/agave#483 by setting this value to `processed` in the client. Until the server PR is deployed widely, this should cover those who choose to upgrade

Addresses anza-xyz/agave#479

# Test plan

```
pnpm turbo test:unit:node test:unit:browser
```
  • Loading branch information
steveluscher authored Apr 3, 2024
1 parent 0be6bed commit c801637
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 7 deletions.
5 changes: 5 additions & 0 deletions .changeset/tricky-fishes-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@solana/rpc-transformers': patch
---

Improve transaction sending reliability for those who skip preflight (simulation) when calling `sendTransaction`
4 changes: 3 additions & 1 deletion packages/library-legacy/src/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5895,7 +5895,9 @@ export class Connection {
const config: any = {encoding: 'base64'};
const skipPreflight = options && options.skipPreflight;
const preflightCommitment =
(options && options.preflightCommitment) || this.commitment;
skipPreflight === true
? 'processed' // FIXME Remove when https://github.com/anza-xyz/agave/pull/483 is deployed.
: (options && options.preflightCommitment) || this.commitment;

if (options && options.maxRetries != null) {
config.maxRetries = options.maxRetries;
Expand Down
23 changes: 23 additions & 0 deletions packages/library-legacy/test/connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4968,6 +4968,29 @@ describe('Connection', function () {
});
}

// FIXME Remove when https://github.com/anza-xyz/agave/pull/483 is deployed.
(
[undefined, 'processed', 'confirmed', 'finalized'] as (
| Commitment
| undefined
)[]
).forEach(explicitPreflightCommitment => {
it(`sets \`preflightCommitment\` to \`processed\` when \`skipPreflight\` is \`true\`, no matter that \`preflightCommitment\` was set to \`${explicitPreflightCommitment}\``, () => {
const connection = new Connection(url);
const rpcRequestMethod = spy(connection, '_rpcRequest');
connection.sendEncodedTransaction('ENCODEDTRANSACTION', {
...(explicitPreflightCommitment
? {preflightCommitment: explicitPreflightCommitment}
: null),
skipPreflight: true,
});
expect(rpcRequestMethod).to.have.been.calledWithExactly(
'sendTransaction',
[match.any, match.has('preflightCommitment', 'processed')],
);
});
});

it('get largest accounts', async () => {
await mockRpcResponse({
method: 'getLargestAccounts',
Expand Down
1 change: 1 addition & 0 deletions packages/rpc-transformers/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"maintained node versions"
],
"dependencies": {
"@solana/functional": "workspace:*",
"@solana/rpc-types": "workspace:*",
"@solana/rpc-spec": "workspace:*",
"@solana/rpc-subscriptions-spec": "workspace:*"
Expand Down
22 changes: 22 additions & 0 deletions packages/rpc-transformers/src/__tests__/params-transformer-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,28 @@ describe('getDefaultParamsTransformerForSolanaRpc', () => {
},
);
});
// FIXME Remove when https://github.com/anza-xyz/agave/pull/483 is deployed.
it.each([undefined, 'processed', 'confirmed', 'finalized'])(
'sets the `preflightCommitment` to `processed` when `skipPreflight` is `true` and `preflightCommitment` is `%s` on calls to `sendTransaction`',
explicitPreflightCommitment => {
const patcher = getDefaultParamsTransformerForSolanaRpc();
expect(
patcher(
[
null,
{
// eslint-disable-next-line jest/no-conditional-in-test
...(explicitPreflightCommitment
? { preflightCommitment: explicitPreflightCommitment }
: null),
skipPreflight: true,
},
],
'sendTransaction',
),
).toContainEqual(expect.objectContaining({ preflightCommitment: 'processed' }));
},
);
describe('given an integer overflow handler', () => {
let onIntegerOverflow: jest.Mock;
let paramsTransformer: (value: unknown) => unknown;
Expand Down
30 changes: 24 additions & 6 deletions packages/rpc-transformers/src/params-transformer.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { pipe } from '@solana/functional';
import { Commitment } from '@solana/rpc-types';

import { applyDefaultCommitment } from './default-commitment';
Expand Down Expand Up @@ -32,11 +33,28 @@ export function getDefaultParamsTransformerForSolanaRpc(config?: ParamsTransform
if (optionsObjectPositionInParams == null) {
return patchedParams;
}
return applyDefaultCommitment({
commitmentPropertyName: methodName === 'sendTransaction' ? 'preflightCommitment' : 'commitment',
optionsObjectPositionInParams,
overrideCommitment: defaultCommitment,
params: patchedParams,
});
return pipe(
patchedParams,
params =>
applyDefaultCommitment({
commitmentPropertyName: methodName === 'sendTransaction' ? 'preflightCommitment' : 'commitment',
optionsObjectPositionInParams,
overrideCommitment: defaultCommitment,
params,
}),
// FIXME Remove when https://github.com/anza-xyz/agave/pull/483 is deployed.
params =>
methodName === 'sendTransaction'
? applyFixForIssue479(params as [unknown, { skipPreflight?: boolean } | undefined])
: params,
);
};
}

// See https://github.com/anza-xyz/agave/issues/479
function applyFixForIssue479(params: [unknown, { skipPreflight?: boolean } | undefined]) {
if (params[1]?.skipPreflight !== true) {
return params;
}
return [params[0], { ...params[1], preflightCommitment: 'processed' }, ...params.slice(2)];
}
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit c801637

Please sign in to comment.