From c801637bcf94930be832c57817166da2d2fdb1a4 Mon Sep 17 00:00:00 2001 From: Steven Luscher Date: Wed, 3 Apr 2024 14:22:52 -0700 Subject: [PATCH] fix: downshift `preflightCommitment` to `processed` when bypassing preflight checks (#2415) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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 https://github.com/anza-xyz/agave/pull/483 for more detail. In this PR, we try to get ahead of https://github.com/anza-xyz/agave/pull/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 https://github.com/anza-xyz/agave/issues/479 # Test plan ``` pnpm turbo test:unit:node test:unit:browser ``` --- .changeset/tricky-fishes-pull.md | 5 ++++ packages/library-legacy/src/connection.ts | 4 ++- .../library-legacy/test/connection.test.ts | 23 ++++++++++++++ packages/rpc-transformers/package.json | 1 + .../src/__tests__/params-transformer-test.ts | 22 ++++++++++++++ .../src/params-transformer.ts | 30 +++++++++++++++---- pnpm-lock.yaml | 3 ++ 7 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 .changeset/tricky-fishes-pull.md diff --git a/.changeset/tricky-fishes-pull.md b/.changeset/tricky-fishes-pull.md new file mode 100644 index 000000000000..c59d499d2eee --- /dev/null +++ b/.changeset/tricky-fishes-pull.md @@ -0,0 +1,5 @@ +--- +'@solana/rpc-transformers': patch +--- + +Improve transaction sending reliability for those who skip preflight (simulation) when calling `sendTransaction` diff --git a/packages/library-legacy/src/connection.ts b/packages/library-legacy/src/connection.ts index 3826119f3dab..dedf5b7964d2 100644 --- a/packages/library-legacy/src/connection.ts +++ b/packages/library-legacy/src/connection.ts @@ -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; diff --git a/packages/library-legacy/test/connection.test.ts b/packages/library-legacy/test/connection.test.ts index 7c240257d149..ecd3fea02aa8 100644 --- a/packages/library-legacy/test/connection.test.ts +++ b/packages/library-legacy/test/connection.test.ts @@ -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', diff --git a/packages/rpc-transformers/package.json b/packages/rpc-transformers/package.json index 2074aa5c4565..54467a06d0e9 100644 --- a/packages/rpc-transformers/package.json +++ b/packages/rpc-transformers/package.json @@ -63,6 +63,7 @@ "maintained node versions" ], "dependencies": { + "@solana/functional": "workspace:*", "@solana/rpc-types": "workspace:*", "@solana/rpc-spec": "workspace:*", "@solana/rpc-subscriptions-spec": "workspace:*" diff --git a/packages/rpc-transformers/src/__tests__/params-transformer-test.ts b/packages/rpc-transformers/src/__tests__/params-transformer-test.ts index 04af6898185f..525a7d58b8de 100644 --- a/packages/rpc-transformers/src/__tests__/params-transformer-test.ts +++ b/packages/rpc-transformers/src/__tests__/params-transformer-test.ts @@ -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; diff --git a/packages/rpc-transformers/src/params-transformer.ts b/packages/rpc-transformers/src/params-transformer.ts index 4520de8f367f..faf99159424a 100644 --- a/packages/rpc-transformers/src/params-transformer.ts +++ b/packages/rpc-transformers/src/params-transformer.ts @@ -1,3 +1,4 @@ +import { pipe } from '@solana/functional'; import { Commitment } from '@solana/rpc-types'; import { applyDefaultCommitment } from './default-commitment'; @@ -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)]; +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c759672bbb41..7628ac586992 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -811,6 +811,9 @@ importers: packages/rpc-transformers: dependencies: + '@solana/functional': + specifier: workspace:* + version: link:../functional '@solana/rpc-spec': specifier: workspace:* version: link:../rpc-spec