Add ClientWithTransactionSigning interface - #1899
Conversation
🦋 Changeset detectedLatest commit: e9ff80a 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 |
|
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. |
BundleMonFiles updated (4)
Unchanged files (146)
Total files change -433B -0.08% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
This PR adds the ClientWithTransactionSigning interface to @solana/plugin-interfaces, installing signTransaction(s) methods that mirror sendTransaction(s) — same flexible inputs, same config — but return the …WithOptionalSignature result types from #1893, since the fee payer signature may legitimately be absent. Docs pages, README, and typetests are all updated.
The implementation is solid:
- The interface mirrors
ClientWithTransactionSendingfaithfully — input unions,Config, and docblock structure all match the house style. - The typetests follow the repo's
// [DESCRIBE]+ nested-block convention, and the assertion that aClientWithTransactionSending's methods structurally satisfy the signing methods is a nice way to pin down the required-signature-assignable-to-optional relationship. - README and both docs pages are updated consistently.
Two points before merge:
-
Missing changeset. This ships a new public API on the published
@solana/plugin-interfacespackage, so per the repo conventions it needs a changeset (minor, generated vianpx changeset add --empty). I don't see one in the changed files — if it's intentionally deferred to another PR in the stack, ignore this, but as-is this PR wouldn't trigger a release. -
context.transactionis optional on the results — see the inline comment. Not blocking, but worth a deliberate decision since handing back the transaction is the whole point of this interface.
For subsequent reviewers: the types this PR consumes are defined in #1893, so the main thing to verify here is the semantic contract (docblocks/README) rather than the type mechanics, which the typetests already cover.
| signTransaction: ( | ||
| input: InstructionPlanInput | SingleTransactionPlan | SingleTransactionPlan['message'], | ||
| config?: Config, | ||
| ) => Promise<SuccessfulSingleTransactionPlanResultWithOptionalSignature>; |
There was a problem hiding this comment.
Design question, non-blocking: SuccessfulSingleTransactionPlanResultWithOptionalSignature types context.transaction as optional (it comes from BaseTransactionPlanResultContext), yet retrieving the signed transaction is the entire purpose of this interface. As typed, every caller must handle result.context.transaction being undefined — including your own README example, which passes result.context.transaction straight to postToRelayer and would fail a strict type-check.
In practice successfulSingleTransactionPlanResultWithOptionalSignature always populates transaction, so the type is looser than the reality. Would it be worth tightening the return type here to guarantee transaction is present (e.g. intersecting with { context: { transaction: Transaction } }, or adding a dedicated result type in @solana/instruction-plans)? If the looseness is intentional — e.g. to allow executors that only report signatures — a sentence in the docblock explaining how callers should handle a missing transaction would help.
There was a problem hiding this comment.
Agreed, changed this to narrow the context on this executor so that it has transaction: Transaction on the successful leaves.
|
Documentation Preview: https://kit-docs-mxfri30jp-anza-tech.vercel.app |
34963e8 to
c18ec28
Compare
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Thanks for addressing both points from the last pass — this now looks ready to go. (Would approve, but I don't have repository access to submit an approving review, so leaving this as a comment.)
What changed since my last review:
- A
minorchangeset for@solana/plugin-interfaceswas added, resolving point 1. - The optional-
context.transactionconcern (point 2) is resolved properly: the newSignedSingleTransactionPlanResultnarrowsSuccessfulSingleTransactionPlanResultWithOptionalSignaturevia theTContextslot socontext.transactionis required, andSignedTransactionPlanResultthreads that through the result tree forsignTransactions. Neat use of the context generic — the intersection oftransaction?: Transactionwith{ transaction: Transaction }does exactly the right thing, and it stays within theTSinglebound (SingleTransactionPlanResultWithOptionalSignature) used by the tree types. - The typetests now cover all the new guarantees:
transactionalways present,signaturestill optional, the loose result not assignable to the signing result, and — nicely — the flipped assertion that aClientWithTransactionSending's methods no longer satisfy the signing methods, which documents the deliberate divergence introduced by the tightening. - README example and docblocks updated to match (
result.context.transactionis now safe unguarded), andpackage.json/lockfile pick up the two new type-only workspace deps.
One non-blocking note (see inline): the successfulSingleTransactionPlanResultWithOptionalSignature factory in @solana/instruction-plans always populates transaction but its return type leaves it optional, so implementers building on that helper won't structurally satisfy SignedSingleTransactionPlanResult without a cast. Worth tightening upstream in a follow-up.
For subsequent reviewers: the type mechanics are fully pinned by the typetests; the remaining thing to sanity-check is the semantic contract in the docblocks/README (relayer use case, fee-payer-signature-optional semantics), which reads accurately to me.
| export type SignedSingleTransactionPlanResult = SuccessfulSingleTransactionPlanResultWithOptionalSignature<{ | ||
| transaction: Transaction; | ||
| }>; |
There was a problem hiding this comment.
Non-blocking: the successfulSingleTransactionPlanResultWithOptionalSignature factory in @solana/instruction-plans always populates context.transaction, but its declared return type leaves it optional — so an executor implementation built on that helper won't structurally satisfy SignedSingleTransactionPlanResult without a cast or manual narrowing. Consider tightening the factory's return type upstream (in a follow-up PR in the stack, since that package isn't touched here) so implementations of this interface type-check out of the box.
There was a problem hiding this comment.
This should now be resolved downstack: #1893 (comment)
c18ec28 to
601fc78
Compare
a62ea89 to
58f44c2
Compare
601fc78 to
e85b097
Compare
58f44c2 to
47d43e2
Compare
47d43e2 to
e2e365c
Compare
e85b097 to
ebdc059
Compare
ebdc059 to
567e5e7
Compare
e2e365c to
0594be0
Compare
567e5e7 to
d5ce66b
Compare
0594be0 to
67cbc21
Compare
…result contexts `ClientWithTransactionSigning` provides `signTransaction` and `signTransactions`, which accept the same flexible inputs as their sending counterparts but hand back the signed transactions instead of submitting them. The interface is parameterised over the context attached to its results and makes no default guarantees about that context: what it contains is entirely decided by the plugin providing the capability. `ClientWithTransactionSending` gains the same `TContext` type parameter, but defaults it to `TransactionPlanResultContextWithSignature` for backward compatibility, so existing usage keeps the required `context.signature` on successful results.
d5ce66b to
e9ff80a
Compare
67cbc21 to
2187418
Compare
2 participants
Summary of Changes
This PR adds the
ClientWithTransactionSigninginterface, which addssignTransaction[s]functions to the client.These take the same inputs as
sendTransaction. The implementation should finalise and sign the transaction, for example adding a lifetime that may not have been part of the planner.As the transaction does not need to be sent, it is allowed to be partially signed. In particular, the fee payer does not necessarily need to have signed it, which is useful for eg. relayers. We return the
TransactionPlanResultWithOptionalSignaturetypes defined in #1893, to reflect that the fee payer may not have provided a signature, and therefore the transaction may not yet have one.