Skip to content
This repository has been archived by the owner on Apr 15, 2021. It is now read-only.

feat: add ability to create unsigned txs #115

Closed
wants to merge 2 commits into from

Conversation

kyranjamie
Copy link
Contributor

Related #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)

@kyranjamie kyranjamie requested a review from yknl August 14, 2020 11:04
@kyranjamie kyranjamie force-pushed the feat/allow-unsigned-stx-token-transfers branch from d6a48b1 to d6509c8 Compare August 14, 2020 11:05
@kyranjamie kyranjamie force-pushed the feat/allow-unsigned-stx-token-transfers branch from d6509c8 to 746a4ec Compare August 14, 2020 11:41
@codecov
Copy link

codecov bot commented Aug 14, 2020

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%) ⬆️
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.

Copy link
Collaborator

@yknl yknl left a comment

Choose a reason for hiding this comment

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

Have you considered creating a different builder function for unsigned transactions? i.e. makeSTXTokenTransferUnsigned()

IMO that makes it clearer. For the implementation, it can call makeSTXTokenTransfer() with a public key.

@kyranjamie
Copy link
Contributor Author

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

yknl commented Aug 17, 2020

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
Copy link
Contributor

@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
Copy link
Contributor

reedrosenbluth commented Aug 17, 2020

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

Closing out this PR in favour of @reedrosenbluth's

@kyranjamie kyranjamie closed this Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants