Stop writing to the execution context in createTransactionPlanExecutor - #1913
Open
mcintyre94 wants to merge 1 commit into
Open
Stop writing to the execution context in createTransactionPlanExecutor#1913mcintyre94 wants to merge 1 commit into
createTransactionPlanExecutor#1913mcintyre94 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 2187418 The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (4)
Unchanged files (146)
Total files change -433B -0.08% Final result: ✅ View report in BundleMon website ➡️ |
mcintyre94
force-pushed
the
tx-executor-context-only
branch
from
August 12, 2026 13:49
8176ffe to
a7e859c
Compare
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
mcintyre94
marked this pull request as ready for review
August 12, 2026 13:58
Member
Author
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Verdict: LGTM (submitting as a comment since I don't have approve permissions on this repo)
Summary
This PR completes the inversion started in #1910: createTransactionPlanExecutor no longer writes anything to the execution context. The executeTransactionMessage callback now returns a complete TContext, which becomes the successful result's context (merged over whatever was stored on the mutable context, with the returned value winning). The two signature derivations are gone — the success path no longer calls getSignatureFromTransaction on a returned transaction, and the catch handler no longer derives one from a stored transaction.
This fixes two real bugs for executors producing partially signed transactions (no fee payer signature):
- They can now succeed — previously
successfulSingleTransactionPlanResultFromTransactionwould throwSOLANA_ERROR__TRANSACTION__FEE_PAYER_SIGNATURE_MISSING. - A failure no longer has its original error replaced by the signature-derivation throw in the catch handler.
Both are covered by new tests using the new createPartiallySignedTransaction helper, and the removal of the as unknown as TContext assertion on the happy path is a genuine type-safety win — the callback now proves the context is populated instead of the executor taking it on faith.
Things to watch
- Return-type inference is parameter-sensitivity dependent. A zero-parameter callback gets
TContextinferred from its return value, while any callback that declares parameters falls back to the defaultTransactionPlanResultContextWithSignature. The typetests pin this behavior explicitly (nice), but it's a subtle inference quirk that depends on TypeScript's context-sensitivity rules — worth keeping an eye on across TS version bumps, since a change in inference order would silently alter which executors compile. Promise<void>vsPromise<TContext>trade-off. The PR description's reasoning holds up:voidwould be a gentler migration but loses the one guarantee that makesTContexton successful results honest. The typetest// Mutating the context does not discharge the obligation to return itcaptures exactly the failure mode the return type prevents.- Docs are consistent. The TSDoc on
createTransactionPlanExecutor, theTransactionPlanResultContextWithSignatureexample, the token-airdrop example, and the changeset all tell the same story, including the "return an object built from the values you have; the mutable context itself doesn't satisfyTContext" caveat. The changeset's migration diff matches the actual new API. - Changeset bump.
majorfor@solana/instruction-plansmatches the scale of the break (callback return type change); consistent with the majors on the preceding PRs in this series.
Notes for subsequent reviewers
- The merge in
traverseSingle({ ...context, ...returnedContext }) is the one spot with non-obvious runtime semantics — see the inline note about explicitly-undefinedreturned properties. successfulSingleTransactionPlanResultFromTransactionstill derivessignature/transactionand still throws on missing fee payer signatures — that's intentional and called out in the changeset as the opt-in helper for the fully-signed case.- The
return context as Context; // Never reachedcasts in the failure-path tests are unreachable by construction (the callback always throws before them) and only exist to satisfy the new return type; they don't weaken what the tests assert.
Member
Author
|
Note that if we decide this is the correct approach, I will open a PR that makes it possible to return |
This was referenced Aug 12, 2026
lorisleiva
approved these changes
Aug 13, 2026
mcintyre94
force-pushed
the
tx-result-context-types
branch
from
August 14, 2026 13:03
943d7bb to
50426f2
Compare
mcintyre94
force-pushed
the
tx-executor-context-only
branch
from
August 14, 2026 13:03
a7e859c to
0594be0
Compare
This was referenced Aug 14, 2026
mcintyre94
force-pushed
the
tx-executor-context-only
branch
from
August 14, 2026 14:18
0594be0 to
67cbc21
Compare
The `executeTransactionMessage` callback returned a `Signature` or a `Transaction`, and the executor wrote that return value into the context — overwriting anything the callback had already stored under those keys. It also derived `context.signature` by calling `getSignatureFromTransaction` on a returned transaction, and again on any transaction found on the context while handling a failure. That call throws when the fee payer slot is empty, so an executor that deliberately produces partially signed transactions could not succeed, and one that stored such a transaction before failing had its real error replaced by `SOLANA_ERROR__TRANSACTION__FEE_PAYER_SIGNATURE_MISSING` thrown from inside the catch block. The callback now returns `void` and the context is its only output. Neither derivation survives, which is what makes the partially signed case work: no code path asks a transaction for a signature any more, so an executor declares a `TContext` that does not require one, stores just the transaction, and gets results that honestly report no signature. The previous commit made the types admit that shape; this one makes the runtime match. `BaseTransactionPlanResultContext` goes with those derivations — it described fields the executor wrote on the caller's behalf, and nothing writes them now. The cost is that a signature is no longer supplied on the caller's behalf. An executor whose context requires one — including the default `TransactionPlanResultContextWithSignature` — must now assign it, and failed results carry only what was stored before the throw. `successfulSingleTransactionPlanResultFromTransaction`, the last helper that derived both on its own, goes too; construct results with `successfulSingleTransactionPlanResult` and pass the context explicitly. This supersedes #1906, which wrapped the failed-path derivation in a catch-all rather than removing it. That branch's regression test and its partially-signed-transaction fixture are carried over here.
mcintyre94
force-pushed
the
tx-executor-context-only
branch
from
August 14, 2026 15:56
67cbc21 to
2187418
Compare
mcintyre94
force-pushed
the
tx-result-context-types
branch
from
August 14, 2026 15:56
50426f2 to
1ec4b6f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR follows from #1910
With this PR we no longer write anything to the context in
createTransactionPlanExecutor. Just like the new underlying types, the caller is now in complete control of the context.executeTransactionMessageis nowPromise<TContext>. Previously it returned aTransaction, which we added (along with a derived context) tosignature. As with the previous PR, now we allow the executor to completely control whatTContextis and to return it. We no longer make any changes to what the executor returns.Note that an alternative design considered was to return
Promise<void>, but this loses type safety. Our success case is typed withTContext, but nothing in the type system forces an executor to write what it promises to, to context. The mutablecontextis only for failed/cancelled cases, which are typedPartial<TContext>. This would be a less dramatic breaking change/migration, but I think the type safety is more valuable.This refactor means that we no longer derive the signature from a returned transaction, and we also drop the PR that refactored that to improve safety. If a
signatureis not onTContextthen it won't be available in context, and if it is onTContextthen the executor must return it itself. Similarly, the catch handler no longer derives a signature from the transaction in context.Supersedes: #1894
Supersedes: #1906
Note that if we decide this is the correct approach, I will open a PR that makes it possible to return
TContextbut still allows signature and transaction (ideally deprecated). This will make migration easier, and in particular I'll update our plugins to return context before we move to v8.