enhancement: Change signature of ClientOption.signTransaction to be able to pass KeyPair #1063

[od-hunter]

Change signature of ClientOption.signTransaction to be able to pass KeyPair #1063

Closes #1063

Pull Request Description

Summary

This pull request modifies the ClientOptions type to allow the signTransaction property to accept a Keypair in addition to a function. This change simplifies the process of signing transactions, especially for testing and simple applications, by allowing users to directly use a Keypair without needing to implement a custom signing function.

Changes Made

1. Updated ClientOptions Type:

• Modified the signTransaction property to accept either a SignTransaction function or a Keypair.

export type ClientOptions = {
  // ... other properties
  signTransaction?: SignTransaction | Keypair;
  // ... other properties
};

2. Modified AssembledTransaction Class:

• Updated the sign method to handle the case where signTransaction is a Keypair.
• If a Keypair is provided, it uses the basicNodeSigner to create a signing function.

sign = async ({
  force = false,
  signTransaction = this.options.signTransaction,
}: {
  force?: boolean;
  signTransaction?: ClientOptions["signTransaction"];
} = {}): Promise<void> => {
  // ... existing logic

  // Check if signTransaction is a Keypair
  let signFunction: SignTransaction | undefined;

  if (signTransaction instanceof Keypair) {
    const keypair = signTransaction;
    signFunction = basicNodeSigner(keypair, this.options.networkPassphrase).signTransaction;
  } else if (typeof signTransaction === 'function') {
    signFunction = signTransaction;
  }
  // ... existing logic
    const signOpts: Parameters<SignTransaction>[1] = {
      networkPassphrase: this.options.networkPassphrase,
    };
};

3. Type Safety:

• Ensured that TypeScript correctly infers types when preparing signing options.

[od-hunter]

Hi @chadoh , @Shaptic please review.

[od-hunter]

Happy holidays @Shaptic , @chadoh please review 🥲

[chadoh]

This looks pretty good! Please update at least some of the e2e tests to show that it works. If it's easiest, you can change just test/e2e/src/util.ts to pass the keypair instead of using basicNodeSigner, which will update all the tests. But then we will need one new test using basicNodeSigner to ensure that both approaches continue to work.

[od-hunter]

@chadoh changes implemented.

[od-hunter]

Hi @chadoh , please review, change has been implemented.

[od-hunter]

Hi @chadoh , please review

[silence48]

I would not like to see the signTransaction get modified to be a signTransaction or a keypair... rather add keypair as an optional parameter to clientoptions, then update the sign and sign and send functions in the assembled_transaction file. I tried to leave comments and suggestions on how to fix. you'll need to also solve the tests tho.

[silence48]

This could be less verbose, and arguably safer...
signFunction doesn't need to be a let it can be a const

Suggested change

	// Check if signTransaction is a Keypair
	let signFunction: SignTransaction | undefined;
	if (signTransaction instanceof Keypair) {
	const keypair = signTransaction;
	signFunction = basicNodeSigner(keypair, this.options.networkPassphrase).signTransaction;
	} else if (typeof signTransaction === 'function') {
	signFunction = signTransaction;
	}
	if (!signFunction) {
	if (!signTransaction && !keyPair) {
	throw new AssembledTransaction.Errors.NoSigner(
	"You must provide a signTransaction function, or a keypair either when calling " +
	"`signAndSend` or when initializing your Client"
	);
	};
	const signFunction = signTransaction ? signTransaction : basicNodeSigner(keypair, this.options.networkPassphrase).signTransaction

However you'll also need to remove lines 699-703 also.

Further, you shouldn't modify the type SignTransaction you should just add keypair to clientoptions as an optional and update the sign function here around line 664... you also need to look at the signandsend function for consistency and update that correctly.

[silence48]

you might be wondering what !keyPair is yet later in this review i suggest modifying the sign function to accept either the function or the keypair, then narrow it. There's a bunch of ways to do this but there's no easy way for me to show exactly how i would do it without making a whole new pr which i am not gonna do. Anyways if you have questions about my review feel free to reach out. thanks.

[silence48]

I think there's a better way to do this whole thing than modifying the signTransaction type which is supposed to match the function signature from freighter. Instead, modify the sign function to just either take a signTransaction OR a Keypair, then check if signTransaction is falsey you should build it using that simple node signer...

Suggested change

	// const signOpts: Parameters<NonNullable<ClientOptions['signTransaction']>>[1] = {
	// networkPassphrase: this.options.networkPassphrase,
	// };
	const signOpts: Parameters<SignTransaction>[1] = {
	const signOpts: Parameters<SignTransaction>[1] = {

it's fine to remove NonNullable here since it no longer is, but basically it needs to include EITHER signTransaction as a SignTransaction type or a keyPair as a KeyPair type...

[silence48]

Suggested change

	signTransaction?: SignTransaction | Keypair;
	signTransaction?: SignTransaction;
	keyPair?: Keypair;

please, let's avoid overloading signTransaction with a Keypair. Keep Freighter’s SignTransaction type intact and handle a Keypair as a separate field. Then in sign, pick whichever one is provided—if we have a Keypair, wrap it with basicNodeSigner; if not, use signTransaction. That way, the code stays clean and you don’t break the original type. For example:

sign = async ({
  force = false,
  signTransaction = this.options.signTransaction,
  keypair = this.options.keypair,
}: {
  force?: boolean;
  signTransaction?: SignTransaction;
  keypair?: Keypair;
} = {}): Promise<void> => {
  if (!this.built) throw new Error("Transaction has not been simulated yet.");
  if (!force && this.isReadCall) {
    throw new AssembledTransaction.Errors.NoSignatureNeeded("Read call; no signature needed unless `force: true`.");
  }

  let fn = signTransaction;
  if (keypair) {
    fn = basicNodeSigner(keypair, this.options.networkPassphrase).signTransaction;
  }
  if (!fn) {
    throw new AssembledTransaction.Errors.NoSigner("Provide either a `signTransaction` function or a `keypair`.");
  }

  // Prepare signing options
  const signOpts: Parameters<SignTransaction>[1] = {
    networkPassphrase: this.options.networkPassphrase,
    address: this.options.address,
    submit: this.options.submit,
    submitUrl: this.options.submitUrl,
  };

  const { signedTxXdr, error } = await fn(this.built.toXDR(), signOpts);
  this.handleWalletError(error);

  this.signed = TransactionBuilder.fromXDR(signedTxXdr, this.options.networkPassphrase);
};

That’s it. You either provide a regular signTransaction method or a Keypair, never both in the same field, and you don’t tangle up the types.

[silence48]

signTransaction should be the function not a keypair.