Change signature of ClientOption.signTransaction to be able to pass KeyPair #1079
Conversation
src/contract/basic_node_signer.ts
Outdated
| t.sign(keypair); | ||
| return t.toXDR(); | ||
| signTransaction: async (tx: string, signer?: () | KeyPair, opts?: { | ||
| network?: string; |
There was a problem hiding this comment.
network doesn't appear to be used here
src/contract/basic_node_signer.ts
Outdated
| signTransaction: async (tx: string, signer?: () | KeyPair, opts?: { | ||
| network?: string; | ||
| networkPassphrase?: string; | ||
| accountToSign?: string;) => { |
src/contract/basic_node_signer.ts
Outdated
| network?: string; | ||
| networkPassphrase?: string; | ||
| accountToSign?: string;) => { | ||
| if(signer instanceof KeyPair){ |
There was a problem hiding this comment.
formatting nit:
| if(signer instanceof KeyPair){ | |
| if (signer instanceof KeyPair){ |
There was a problem hiding this comment.
We've had issues in the past with using instanceof to check for types due to dependency trees creating multiple instances of a particular object. This is okay for now but please consider a better way to determine whether or not a signer is a Keypair
|
@Shaptic , please review |
|
@Shaptic please review |
There was a problem hiding this comment.
Let's hold off until the upstream discussion is finalized (see below). But also, the code as written will not work, I don't think, and we will need at least one test to verify one way or the other (an alternative would be to update most of the e2e tests, and just keep one test that verifies the old functionality).
src/contract/basic_node_signer.ts
Outdated
| const t = TransactionBuilder.fromXDR(tx, networkPassphrase); | ||
| t.sign(keypair); | ||
| return t.toXDR(); | ||
| signTransaction: async (tx: string, signer?, opts?: { |
There was a problem hiding this comment.
signer?, is not valid TypeScript; does this code work?
I think this whole approach will cause typing problems, though! Sorry I didn't catch this before you started working. See comment here:
There was a problem hiding this comment.
Ok, conversation resolved, this is the desired way to do it. Please make sure your code works by testing it and we can get this merged!
| signTransaction: async (tx: string, opts?: { | ||
| networkPassphrase?: string;}, signer?: Keypair): Promise<string> => { | ||
| if (signer instanceof Keypair){ | ||
| const basicSigner = basicNodeSigner(signer, opts?.networkPassphrase!); | ||
| return basicSigner.signTransaction(tx); | ||
| } else{ | ||
| const t = TransactionBuilder.fromXDR(tx, networkPassphrase); | ||
| t.sign(keypair); | ||
| return t.toXDR(); | ||
| } |
There was a problem hiding this comment.
We do not need any change to basicNodeSigner
| @@ -84,6 +84,7 @@ export type ClientOptions = { | |||
| networkPassphrase?: string; | |||
| accountToSign?: string; | |||
| }, | |||
| signer?: Keypair, | |||
There was a problem hiding this comment.
We don't want to add a signer as an option to signTransaction. We want to use the existing signature, which comes from Freighter / SEP-43, OR we want to allow passing a keypair. Like this:
signTransaction?: Keypair | (
tx: XDR_BASE64,
opts?: {
network?: string;
networkPassphrase?: string;
accountToSign?: string;
},
) => Promise<XDR_BASE64>;
resolves #1063