Add Threshold fulfillments #119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 82.2% 83.64% +1.44%
==========================================
Files 21 22 +1
Lines 281 318 +37
==========================================
+ Hits 231 266 +35
- Misses 50 52 +2
Continue to review full report at Codecov.
|
| } | ||
| return fulfillment | ||
| } else { | ||
| const subcdts = [] |
There was a problem hiding this comment.
subcdts is not an appreviation we commonly use in the code base.
I suggest to rename to a word we also use in other implementations.
| @@ -16,13 +17,19 @@ function makeTransactionTemplate() { | |||
|
|
|||
| export default function makeTransaction(operation, asset, metadata = null, outputs = [], inputs = []) { | |||
| const tx = makeTransactionTemplate() | |||
|
|
|||
| const realInputs = clone(inputs) | |||
There was a problem hiding this comment.
Suggest naming this to something else than real. E.g. inputsCopy etc.
| const assetLink = { | ||
| 'id': unspentTransaction.operation === 'CREATE' ? unspentTransaction.id | ||
| : unspentTransaction.asset.id | ||
| } | ||
| const makeT = makeTransaction('TRANSFER', assetLink, metadata, outputs, inputs) |
There was a problem hiding this comment.
Why this change? I suspect you don't need makeT after the return?
| privateKeys.splice(0, 1) | ||
| const privateKeyBuffer = new Buffer(base58.decode(privateKey)) | ||
| ed25519subFulfillment.sign(new Buffer(serializedTransaction), privateKeyBuffer) | ||
| } else { |
There was a problem hiding this comment.
Codecov says, this condition is not covered by tests. Please add :)
| signedTx.inputs.forEach((input) => { | ||
| if (input.fulfillment.type === 'ed25519-sha-256') { | ||
| const privateKey = privateKeys[0] | ||
| privateKeys.splice(0, 1) |
There was a problem hiding this comment.
splice returns the removed element within an array. Not sure how useful that is, but you could combine the two lines.
| @@ -20,15 +20,43 @@ import serializeTransactionIntoCanonicalString from './serializeTransactionIntoC | |||
| */ | |||
| export default function signTransaction(transaction, ...privateKeys) { | |||
There was a problem hiding this comment.
What I'd like to see (maybe as tests):
What happens
- if the wrong private keys get submitted to this method
- if too little or too much keys get submitted to this method
See also my comment on having a friend sign with you
|
|
||
| input.fulfillment = fulfillmentUri | ||
| transaction.inputs.forEach((input) => { | ||
| input.fulfillment = null // OJOOO |
| const thresholdFulfillment = new cc.ThresholdSha256() | ||
| // m represents the number of signatures needed for this input | ||
| let m = 0 | ||
| input.fulfillment.subconditions.forEach((subcdt) => { |
There was a problem hiding this comment.
subcdts is not an appreviation we commonly use in the code base.
I suggest to rename to a word we also use in other implementations.
| } | ||
| thresholdFulfillment.addSubfulfillmentUri(ed25519subFulfillment) | ||
| }) | ||
| thresholdFulfillment.setThreshold(m) |
There was a problem hiding this comment.
Instead of determining m ourselves, it may make sense to let the user of this method decide by themselves.
They may not be aware that this method is adjusting their fulfillment's threshold.
One suggestion would be: add threshold to parameters of sign method?
Maybe there's better ideas.
| @@ -42,6 +42,24 @@ test('Valid CREATE transaction', t => { | |||
| }) | |||
|
|
|||
|
|
|||
| test('Valid CREATE transaction with Threshold input', t => { | |||
There was a problem hiding this comment.
This test may not be that useful. The CREATE tx fulfillment isn't reliant on any output.
I'd instead test the following:
- CREATE tx with std ed25519 input and threshold output
and
- CREATE tx with std ed25519 input and threshold output
- TRANSFER tx spending the threshold output into an ed25519 output
I'd also test:
- m-of-n
- multi signature transactions
- serialization of these transactions during signing.
For last point, think about a real-life scenario. You and a friend want to spend a transaction together, so you start signing with your key. They you send the half-signed tx to your friend. They sign as well and send it off to the network.
Implement a method to create threshold fulfillments for an input. For this purpose is needed the fulfillment in the makeCreateTransaction and makeTransferTransaction methods that was null until now.