Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
steveluscher opened this issue Jun 14, 2024 · 11 comments · Fixed by #2852
Closed

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

steveluscher opened this issue Jun 14, 2024 · 11 comments · Fixed by #2852
Assignees
Labels
enhancement New feature or request

Comments

@steveluscher
Copy link
Collaborator

steveluscher commented Jun 14, 2024

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 steveluscher added the bug Something isn't working label Jun 14, 2024
@steveluscher steveluscher changed the title [experimental] Possible problem with [experimental] Possible problem with signAndSendTransactionMessageWithSigners Jun 14, 2024
@steveluscher steveluscher changed the title [experimental] Possible problem with signAndSendTransactionMessageWithSigners [experimental] Possible typing problem with signAndSendTransactionMessageWithSigners Jun 14, 2024
@steveluscher
Copy link
Collaborator Author

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

@lorisleiva
Copy link
Collaborator

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 #2835 to illustrate that.

@steveluscher
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@steveluscher steveluscher changed the title [experimental] Possible typing problem with signAndSendTransactionMessageWithSigners [experimental] Is it possible to produce a message with ITransactionMessageWithSingleSendingSigner on the basis of a single signer having been used in its construction? Jun 20, 2024
@steveluscher steveluscher added enhancement New feature or request and removed bug Something isn't working labels Jun 20, 2024
@lorisleiva
Copy link
Collaborator

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
Copy link
Collaborator Author

steveluscher commented Jun 21, 2024

…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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

steveluscher commented Jun 21, 2024

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.

@steveluscher steveluscher changed the title [experimental] Is it possible to produce a message with ITransactionMessageWithSingleSendingSigner on the basis of a single signer having been used in its construction? [experimental] Decide what to do about single sending signer enforcement Jun 24, 2024
@lorisleiva
Copy link
Collaborator

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:

  • Option A: Keep things as-is, meaning the user as to use the assert function in order to use signAndSendTransactionMessageWithSigners as it was initially intended. The part that sucks here is TS' failure to provide a helpful error message but with some documentation and the regression test added in refactor(experimental): add a typetest for single sending signer requirement #2835, we can probably make this work. Who knows, we may even figure out how to tell TS to fail properly in the future.
  • Option B: Internalise the assert function and call it directly inside the signAndSendTransactionMessageWithSigners function, therefore deferring the error from TS land to JS land. Whilst this may seem like a bad move (as we're not failing as soon as we can), in practice, this doesn't actually change the current behaviour since the assert function would also only fail in JS land anyway.

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
Copy link
Collaborator Author

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

lorisleiva added a commit that referenced this issue Jun 26, 2024
…nd function (#2852)

Fixes #2818

This PR calls the `assertIsTransactionMessageWithSingleSendingSigner` inside the `signAndSendTransactionMessageWithSigners` so the end-user doesn't need to do it explicitly.

This is partly to improve the developer experience and partly because TS doesn't fail properly when the transaction message hasn't been asserted on prior to calling the sign and send function.

Note that I did not remove the `assertIsTransactionMessageWithSingleSendingSigner` and the `isTransactionMessageWithSingleSendingSigner` from the package's exports since they may still be useful for custom use-cases such as the one described in the documentation:

```ts
let transactionSignature: SignatureBytes;
if (isTransactionMessageWithSingleSendingSigner(transaction)) {
    transactionSignature = await signAndSendTransactionMessageWithSigners(transaction);
} else {
    const signedTransaction = await signTransactionMessageWithSigners(transaction);
    const encodedTransaction = getBase64EncodedWireTransaction(signedTransaction);
    transactionSignature = await rpc.sendTransaction(encodedTransaction).send();
}
```
Copy link
Contributor

github-actions bot commented Jul 4, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.