stacks-archive / stacks-transactions-js

The JavaScript library for generating Stacks 2.0 transactions
19 stars 17 forks source link

feat: add ability to create unsigned txs #115

Closed kyranjamie closed 4 years ago

kyranjamie commented 4 years ago

Related https://github.com/blockstack/stacks-transactions-js/issues/112

This PR introduces additional options to makeSTXTokenTransfer() allowing you to pass public key rather than private, so an unsigned transaction can be created.

Wanted to type this in a way the compiler can detect either senderKey or publicKey, as below, but this approach doesn't work as the compiler throws an error when checking if the prop of the other exists. For now, I've added a runtime exception if both keys are passed.

export interface BaseTokenTransferOptions {
  recipient: string | PrincipalCV;
  amount: BigNum;
  fee?: BigNum;
  nonce?: BigNum;
  network?: StacksNetwork;
  anchorMode?: AnchorMode;
  memo?: string;
  postConditionMode?: PostConditionMode;
  postConditions?: PostCondition[];
  sponsored?: boolean;
}
export interface SignedTokenTransferOptions extends BaseTokenTransferOptions {
  senderKey: string | Buffer;
}

export interface UnsignedTokenTransferOptions extends BaseTokenTransferOptions {
  publicKey: string | Buffer;
}

export type TokenTransferOptions = SignedTokenTransferOptions | UnsignedTokenTransferOptions;

This error follows an if (options.senderKey) {}

Property 'senderKey' does not exist on type 'Required<SignedTokenTransferOptions> | Required<UnsignedTokenTransferOptions>'.
  Property 'senderKey' does not exist on type 'Required<UnsignedTokenTransferOptions>'.ts(2339)
codecov[bot] commented 4 years ago

Codecov Report

Merging #115 into master will increase coverage by 0.02%. The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   82.76%   82.79%   +0.02%     
==========================================
  Files          28       28              
  Lines        2031     2040       +9     
  Branches      438      444       +6     
==========================================
+ Hits         1681     1689       +8     
  Misses        346      346              
- Partials        4        5       +1     
Impacted Files Coverage Δ
src/builders.ts 77.53% <91.66%> (+0.29%) :arrow_up:
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 29f23dc...f1c6821. Read the comment docs.

kyranjamie commented 4 years ago

@yknl, I did just this in my test version by copying the code. In this PR I changed it to doing it within the one function.

If you reckon it's a much better API, we can do this, but it won't be as simple change as for makeSTXTokenTransferUnsigned() to invoke makeSTXTokenTransfer(). To follow your recommendation, makeSTXTokenTransfer needs to be able to accept a publicKey, which means unsigned transactions could be made by both functions anyway, if I'm not mistaken.

yknl commented 4 years ago

Yes, if we do this the base function makeSTXTokenTransfer() will support creating unsigned txs as well. Since the multi-sig functions are also going to need to create unsigned txs, I think this makes sense.

zone117x commented 4 years ago

@kyranjamie

Wanted to type this in a way the compiler can detect either senderKey or publicKey, as below, but this approach doesn't work as the compiler throws an error when checking if the prop of the other exists

Your example code would work, you just need to type cast before the check. E.g.

if ((options as SignedTokenTransferOptions).senderKey) {}
reedrosenbluth commented 4 years ago

I've got a version of this working in the multi-sig PR: https://github.com/blockstack/stacks-transactions-js/blob/feat/multi-sig-gen/src/builders.ts#L249

@kyranjamie could you take a look?

kyranjamie commented 4 years ago

Closing out this PR in favour of @reedrosenbluth's