solana-labs / solana-web3.js

Solana JavaScript SDK
https://solana-labs.github.io/solana-web3.js
MIT License
2.12k stars 847 forks source link

[experimental] Decide what to do about single sending signer enforcement #2818

Closed steveluscher closed 3 months ago

steveluscher commented 3 months ago

This valid, compilable message is not acceptable to signAndSendTransactionMessageWithSigners.

https://codesandbox.io/p/sandbox/signandsendtransactionmessagewithsigners-input-type-problem-szwfmy?file=%2Fsrc%2Findex.ts%3A39%2C9-39%2C49

The full error:

Argument of type 'TransactionMessageWithBlockhashLifetime & ITransactionMessageWithFeePayerSigner<"123", TransactionSigner<"123">> & Readonly<...>' is not assignable to parameter of type 'CompilableTransactionMessageWithSigners & Pick<Readonly<{ instructions: readonly (IInstruction<string, readonly (IAccountLookupMeta<string, string> | IAccountMeta<...>)[]> & IInstructionWithSigners<...>)[]; version: TransactionVersion; }>, "instructions"> & { ...; } & { ...; }'.
  Type 'TransactionMessageWithBlockhashLifetime & ITransactionMessageWithFeePayerSigner<"123", TransactionSigner<"123">> & Readonly<...>' is not assignable to type 'Readonly<{ instructions: readonly IInstruction<string, readonly (IAccountLookupMeta<string, string> | IAccountMeta<string>)[]>[]; version: TransactionVersion; }> & ... 4 more ... & { ...; }'.
    Type 'TransactionMessageWithBlockhashLifetime & ITransactionMessageWithFeePayerSigner<"123", TransactionSigner<"123">> & Readonly<...>' is not assignable to type 'TransactionMessageWithDurableNonceLifetime<string, string, string>'.
      Types of property 'instructions' are incompatible.
        Type 'readonly IInstruction<string, readonly (IAccountLookupMeta<string, string> | IAccountMeta<string>)[]>[]' is not assignable to type 'readonly [AdvanceNonceAccountInstruction<string, string>, ...IInstruction<string, readonly (IAccountLookupMeta<string, string> | IAccountMeta<string>)[]>[]]'.
          Source provides no match for required element at position 0 in target.typescript(2345)

Discussion

Right now we have an ITransactionMessageWithSingleSendingSigner type. The input of signAndSendTransactionMessageWithSigners() takes this as input. It's designed to ensure that every message passed to it has exactly one sending signer, because by definition that's all a message can handle.

Because our upstream utilities don't do anything to produce this type, it's always incumbent on the dev to assert this fact:

const message = pipe(
  createTransactionMessage({ version: 0}),
  m => setTransactionMessageFeePayerSigner(signer, m),
  /* ... */
);
assertIsTransactionMessageWithSingleSendingSigner(message); // <= required
await signAndSendTransactionMessageWithSigners(message);

If you forget that assertion, the TypeScript error that results is unintelligible and even less actionable. If you haven't read the docs, you're hosed.

Options

  1. Delete ITransactionMessageWithSingleSendingSigner and all associated utilities. Do runtime enforcement of a message having exactly one sending signer in the send function
  2. Make every operation over a transaction message aware that if the input message already has a sending-signer for address X and the operation would add a new sending-signer for address Y, to remove the ITransactionMessageWithSingleSendingSigner type. Otherwise, add it with the introduction of a new sending-signer (discussion link).
steveluscher commented 3 months ago

I bet this is because the IInstructionWithSigners type nukes the specificity of the durable nonce instructions.

lorisleiva commented 3 months ago

I'm not sure why TypeScript decides to throw this error but the root cause is actually different and valid.

In order to use the signAndSendTransactionMessageWithSigners function, you need to ensure the provided message contains exactly one sending signer. Therefore, the assertIsTransactionMessageWithSingleSendingSigner function must be used beforehand.

I've added a typetest in PR https://github.com/solana-labs/solana-web3.js/pull/2835 to illustrate that.

steveluscher commented 3 months ago

Oof. That will catch everyone who uses this new wallet adapter API. Do you think there's any way to change the types of setTransactionMessageFeePayerSigner et al such that the first time it sees a TransactionSendingSigner<TAddress> it outputs ITransactionMessageWithSingleSendingSigner, the second time it sees TransactionSendingSigner<TAddress> it still outputs ITransactionMessageWithSingleSendingSigner, and any time it sees TransactionSendingSigner<TSomeOtherAddress> it removes ITransactionMessageWithSingleSendingSigner?

steveluscher commented 3 months ago

I guess at minimum this would involve making ITransactionMessageWithSingleSendingSigner take TAddress as a type parameter, some overloads, and some complicated TAddress extends TOtherAddress ? ... : ... conditional types.

lorisleiva commented 3 months ago

I don't think that adding/removing a TS flag will be doable since you can technically have multiple sending signers as long as they are all the signer. For instance that sending signer could be used as the transaction payer and also as the authority on some instruction, yet it's still the same signer.

I think the easiest path to success here is to move the assertion call inside the sign and send helper. TS won't complain but JS will and I think that's good enough in this case since using multiple sending signers is more of an edge case than a common gotcha. Wdyt?

steveluscher commented 3 months ago

…you can technically have multiple sending signers as long as they are all the signer.

Yeah totally. I meant the overloads would be like:

// Overrides
function foo(thing: ThingWithSendingSigner<TAddressOfSendingSigner>, tx: SingleSendingSignerTx<TAddressOfSendingSigner>): SingleSendingSignerTx<TAddressOfSendingSigner>;
function foo(thing: ThingWithSendingSigner<TOtherAddress>, tx: SingleSendingSignerTx<TAddressOfSendingSigner>): NoLongerASingleSendingSignerTx;
function foo(thing: ThingWithSendingSigner<TAddressOfSendingSigner>, tx: TransactionWithNoSendingSigner): SingleSendingSignerTx<TAddressOfSendingSigner>;
function foo(thing: Thing, tx: Transaction) {
  // Implementation
}

Anyway, the types for that would be off the rails.

steveluscher commented 3 months ago

I think the easiest path to success here is to move the assertion call inside the sign and send helper.

Yeah, but then we'd have to delete all of the ITransactionMessageWithSingleSendingSigner types and helpers, because they're intended exactly to avoid having to make that runtime assertion 🤣

steveluscher commented 3 months ago

I wonder if it would be possible at all to create a utility type that walked the transaction that returned true if a transaction message had exactly one sending signer.

TypeScript playground.

Maybe then we could use that to constrain the argument type on signAndSendTransactionMessageWithSigners().

In other words, instead of creating a type that represents a transaction with a single sending signer because we said so, create a utility type that determines that a value has exactly one sending signer.

Again though, this is probably hair-brained and impossible to actually achieve reliably in practice.

I'm leaning toward your suggestion.

lorisleiva commented 3 months ago

I wonder if it would be possible at all to create a utility type that walked the transaction that returned true if a transaction message had exactly one sending signer.

It would be ideal but the main issue here is that people will get signers with non-static addresses from packages like the wallet adapter — i.e. TransactionSendingSigner<string> instead of TransactionSendingSigner<"1111">. Meaning, in practice, it'll be impossible for that recursive type to properly deduplicate unique sending signers.

I'm also worried about how TS is going to fail using a complex recursive type considering it's not even able to properly fail using a simple flag type.


Now IMO we have two viable solutions going forward:

Personally, I'm leaning towards option B as it will make the end-user API simpler to use without much of a tradeoff.

Lmk what you think and I can open a PR for it.

steveluscher commented 3 months ago

Yeah, I'm with you on option B. This one is just too hard to TypeScript, for now.

github-actions[bot] commented 3 months ago

Because there has been no activity on this issue for 7 days since it was closed, it has been automatically locked. Please open a new issue if it requires a follow up.