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.

Sign up for GitHub

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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0960840
enhancement: Enhance ClientOptions to support Keypair for transaction…
od-hunter Dec 20, 2024
a751ba6
fix: signTransaction to signFunction
od-hunter Dec 20, 2024
810e8cf
Merge branch 'stellar:master' into enhance_client_options
od-hunter Dec 27, 2024
a7abadd
test(e2e): keypair
od-hunter Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/contract/assembled_transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
Address,
BASE_FEE,
Contract,
Keypair,
Operation,
SorobanDataBuilder,
TransactionBuilder,
Expand All @@ -15,6 +16,7 @@ import type {
AssembledTransactionOptions,
ClientOptions,
MethodOptions,
SignTransaction,
Tx,
WalletError,
XDR_BASE64,
Expand All @@ -32,6 +34,7 @@ import {
import { DEFAULT_TIMEOUT } from "./types";
import { SentTransaction } from "./sent_transaction";
import { Spec } from "./spec";
import { basicNodeSigner } from './basic_node_signer';

/** @module contract */

Expand Down Expand Up @@ -682,7 +685,17 @@ export class AssembledTransaction<T> {
);
}

if (!signTransaction) {
// 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) {
Comment on lines +688 to +698
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new AssembledTransaction.Errors.NoSigner(
"You must provide a signTransaction function, either when calling " +
"`signAndSend` or when initializing your Client"
Expand All @@ -708,15 +721,18 @@ export class AssembledTransaction<T> {
.setTimeout(timeoutInSeconds)
.build();

const signOpts: Parameters<NonNullable<ClientOptions['signTransaction']>>[1] = {
// const signOpts: Parameters<NonNullable<ClientOptions['signTransaction']>>[1] = {
// networkPassphrase: this.options.networkPassphrase,
// };
const signOpts: Parameters<SignTransaction>[1] = {
Comment on lines +724 to +727
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

networkPassphrase: this.options.networkPassphrase,
};

if (this.options.address) signOpts.address = this.options.address;
if (this.options.submit !== undefined) signOpts.submit = this.options.submit;
if (this.options.submitUrl) signOpts.submitUrl = this.options.submitUrl;

const { signedTxXdr: signature, error } = await signTransaction(
const { signedTxXdr: signature, error } = await signFunction(
this.built.toXDR(),
signOpts,
);
Expand Down
3 changes: 2 additions & 1 deletion src/contract/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/* eslint-disable @typescript-eslint/naming-convention */
import { Memo, MemoType, Operation, Transaction, xdr } from "@stellar/stellar-base";
import type { Client } from "./client";
import { Keypair } from "@stellar/stellar-base";

export type XDR_BASE64 = string;
/**
Expand Down Expand Up @@ -131,7 +132,7 @@ export type ClientOptions = {
*
* Matches signature of `signTransaction` from Freighter.
*/
signTransaction?: SignTransaction;
signTransaction?: SignTransaction | Keypair;
Copy link
Contributor

@silence48 silence48 Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* A function to sign a specific auth entry for a transaction, using the
* private key corresponding to the provided `publicKey`. This is only needed
Expand Down
18 changes: 18 additions & 0 deletions test/e2e/src/test-non-invoker-signing-by-contracts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,24 @@ describe("cross-contract auth", function () {
async () => await this.context.tx.sign({ force: true }),
).to.not.throw();
});

it("signs transaction using Keypair", async function () {
const result = await this.context.tx.sign({
signTransaction: this.context.keypair,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signTransaction should be the function not a keypair.

force: true,
});
expect(result).to.be.undefined;
});


it("signs transaction using basicNodeSigner", async function () {
const signer = contract.basicNodeSigner(this.context.keypair, "Standalone Network ; February 2017");
const result = await this.context.tx.sign({
signTransaction: signer.signTransaction,
force: true,
});
expect(result).to.be.undefined;
});
});

describe("signAuthEntries with custom authorizeEntry", function () {
Expand Down
8 changes: 5 additions & 3 deletions test/e2e/src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ module.exports.generateFundedKeypair = generateFundedKeypair;
*/
async function clientFor(name, { keypair, contractId } = {}) {
const internalKeypair = keypair ?? (await generateFundedKeypair());
const signer = contract.basicNodeSigner(internalKeypair, networkPassphrase);

// Pass the Keypair directly instead of using basicNodeSigner
const signer = internalKeypair; // Use Keypair directly

if (contractId) {
return {
Expand All @@ -99,7 +101,7 @@ async function clientFor(name, { keypair, contractId } = {}) {
rpcUrl,
allowHttp: true,
publicKey: internalKeypair.publicKey(),
...signer,
signTransaction: signer, // Pass Keypair here
}),
contractId,
keypair,
Expand All @@ -118,7 +120,7 @@ async function clientFor(name, { keypair, contractId } = {}) {
allowHttp: true,
wasmHash: wasmHash,
publicKey: internalKeypair.publicKey(),
...signer,
signTransaction: signer, // Pass Keypair here
},
);
const { result: client } = await deploy.signAndSend();
Expand Down