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.

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

Add Threshold fulfillments #119

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions src/transaction/makeCreateTransaction.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import makeFulfillment from './makeFulfillment'
import makeInputTemplate from './makeInputTemplate'
import makeTransaction from './makeTransaction'

Expand Down Expand Up @@ -25,7 +26,8 @@ export default function makeCreateTransaction(asset, metadata, outputs, ...issue
const assetDefinition = {
'data': asset || null,
}
const inputs = issuers.map((issuer) => makeInputTemplate([issuer]))
// Create transaction has 1 and just 1 input
const inputs = makeInputTemplate(issuers, null, makeFulfillment(issuers))

return makeTransaction('CREATE', assetDefinition, metadata, outputs, inputs)
return makeTransaction('CREATE', assetDefinition, metadata, outputs, [inputs])
}
21 changes: 21 additions & 0 deletions src/transaction/makeFulfillment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export default function makeFulfillment(issuers = []) {
if (issuers.length === 1) {
const fulfillment = {
type: 'ed25519-sha-256',
public_keys: issuers
}
return fulfillment
} else {
const subcdts = []
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

issuers.map((issuer) => (
subcdts.push({ type: 'ed25519-sha-256',
public_key: issuer
})
))
const fulfillment = {
type: 'threshold-sha-256',
subconditions: subcdts
}
return fulfillment
}
}
9 changes: 8 additions & 1 deletion src/transaction/makeTransaction.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import clone from 'clone'
import hashTransaction from './hashTransaction'


Expand All @@ -16,13 +17,19 @@ function makeTransactionTemplate() {

export default function makeTransaction(operation, asset, metadata = null, outputs = [], inputs = []) {
const tx = makeTransactionTemplate()

const realInputs = clone(inputs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest naming this to something else than real. E.g. inputsCopy etc.

tx.operation = operation
tx.asset = asset
tx.metadata = metadata
tx.inputs = inputs
tx.inputs = realInputs
tx.outputs = outputs

// Hashing must be done after, as the hash is of the Transaction (up to now)
tx.inputs.forEach((input) => {
input.fulfillment = null
})
tx.id = hashTransaction(tx)
tx.inputs = inputs
return tx
}
9 changes: 5 additions & 4 deletions src/transaction/makeTransferTransaction.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import makeFulfillment from './makeFulfillment'
import makeInputTemplate from './makeInputTemplate'
import makeTransaction from './makeTransaction'

Expand Down Expand Up @@ -36,14 +37,14 @@ export default function makeTransferTransaction(
'output_index': outputIndex,
'transaction_id': unspentTransaction.id,
}

return makeInputTemplate(fulfilledOutput.public_keys, transactionLink)
return makeInputTemplate(fulfilledOutput.public_keys, transactionLink,
makeFulfillment(fulfilledOutput.public_keys))
})

const assetLink = {
'id': unspentTransaction.operation === 'CREATE' ? unspentTransaction.id
: unspentTransaction.asset.id
}
const makeT = makeTransaction('TRANSFER', assetLink, metadata, outputs, inputs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? I suspect you don't need makeT after the return?


return makeTransaction('TRANSFER', assetLink, metadata, outputs, inputs)
return makeT
}
46 changes: 37 additions & 9 deletions src/transaction/signTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,43 @@ import serializeTransactionIntoCanonicalString from './serializeTransactionIntoC
*/
export default function signTransaction(transaction, ...privateKeys) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const signedTx = clone(transaction)
signedTx.inputs.forEach((input, index) => {
const privateKey = privateKeys[index]
const privateKeyBuffer = new Buffer(base58.decode(privateKey))
const serializedTransaction = serializeTransactionIntoCanonicalString(transaction)
const ed25519Fulfillment = new cc.Ed25519Sha256()
ed25519Fulfillment.sign(new Buffer(serializedTransaction), privateKeyBuffer)
const fulfillmentUri = ed25519Fulfillment.serializeUri()

input.fulfillment = fulfillmentUri
transaction.inputs.forEach((input) => {
input.fulfillment = null // OJOOO
Copy link
Contributor

Choose a reason for hiding this comment

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

What does OJOOO mean?

})
const serializedTransaction = serializeTransactionIntoCanonicalString(transaction)
signedTx.inputs.forEach((input) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In both the ed25519-sha-256 and threshold-sha-256 case, you're doing something along the lines of:

  1. cast private key into buffer
  2. create ed25519Fulfillment
  3. using the buffer and the serialized transaction, calling sign on ed25519fulfillment

I wonder if that could be it's own routine. DRY.

if (input.fulfillment.type === 'ed25519-sha-256') {
const privateKey = privateKeys[0]
privateKeys.splice(0, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

splice returns the removed element within an array. Not sure how useful that is, but you could combine the two lines.

const privateKeyBuffer = new Buffer(base58.decode(privateKey))
const ed25519Fulfillment = new cc.Ed25519Sha256()
ed25519Fulfillment.sign(new Buffer(serializedTransaction), privateKeyBuffer)
const fulfillmentUri = ed25519Fulfillment.serializeUri()
input.fulfillment = fulfillmentUri
} else if (input.fulfillment.type === 'threshold-sha-256') {
// Not valid for more than one input with m-of-n signatures where m < n.
const thresholdFulfillment = new cc.ThresholdSha256()
// m represents the number of signatures needed for this input
let m = 0
input.fulfillment.subconditions.forEach((subcdt) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const ed25519subFulfillment = new cc.Ed25519Sha256()
if (privateKeys.length > 0) {
m++
const privateKey = privateKeys[0]
privateKeys.splice(0, 1)
const privateKeyBuffer = new Buffer(base58.decode(privateKey))
ed25519subFulfillment.sign(new Buffer(serializedTransaction), privateKeyBuffer)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Codecov says, this condition is not covered by tests. Please add :)

// All conditions are needed as the "circuit definition" is needed.
const publicKeyBuffer = new Buffer(base58.decode(subcdt.public_key))
ed25519subFulfillment.setPublicKey(publicKeyBuffer)
}
thresholdFulfillment.addSubfulfillmentUri(ed25519subFulfillment)
})
thresholdFulfillment.setThreshold(m)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const fulfillmentUri = thresholdFulfillment.serializeUri()
input.fulfillment = fulfillmentUri
}
})

return signedTx
Expand Down
52 changes: 51 additions & 1 deletion test/integration/test_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,24 @@ test('Valid CREATE transaction', t => {
})


test('Valid CREATE transaction with Threshold input', t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test may not be that useful. The CREATE tx fulfillment isn't reliant on any output.

I'd instead test the following:

  1. CREATE tx with std ed25519 input and threshold output

and

  1. CREATE tx with std ed25519 input and threshold output
  2. 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.

const conn = new Connection(API_PATH)

const tx = Transaction.makeCreateTransaction(
asset(),
metaData,
[aliceOutput],
alice.publicKey,
bob.publicKey
)
const txSigned = Transaction.signTransaction(tx, alice.privateKey, bob.privateKey)

return conn.postTransaction(txSigned)
.then(({ id }) => conn.pollStatusAndFetchTransaction(id))
.then(resTx => t.truthy(resTx))
})


test('Valid TRANSFER transaction with single Ed25519 input', t => {
const conn = new Connection(API_PATH)
const createTx = Transaction.makeCreateTransaction(
Expand Down Expand Up @@ -109,6 +127,39 @@ test('Valid TRANSFER transaction with multiple Ed25519 inputs', t => {
})
})

test('Valid TRANSFER transaction with Threshold input', t => {
const conn = new Connection(API_PATH)
const createTx = Transaction.makeCreateTransaction(
asset(),
metaData,
[aliceOutput],
alice.publicKey,
bob.publicKey
)
const createTxSigned = Transaction.signTransaction(
createTx,
alice.privateKey,
bob.privateKey
)

return conn.postTransaction(createTxSigned)
.then(({ id }) => conn.pollStatusAndFetchTransaction(id))
.then(() => {
const transferTx = Transaction.makeTransferTransaction(
createTxSigned,
metaData,
[aliceOutput],
0
)
const transferTxSigned = Transaction.signTransaction(
transferTx,
alice.privateKey
)
return conn.postTransaction(transferTxSigned)
.then(({ id }) => conn.pollStatusAndFetchTransaction(id))
.then(resTx => t.truthy(resTx))
})
})

test('Search for spent and unspent outputs of a given public key', t => {
const conn = new Connection(API_PATH)
Expand Down Expand Up @@ -214,7 +265,6 @@ test('Search for spent outputs for a given public key', t => {
)
const createTxSigned = Transaction.signTransaction(
createTx,
carol.privateKey,
carol.privateKey
)

Expand Down
3 changes: 2 additions & 1 deletion test/transaction/test_cryptoconditions.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ test('Fulfillment correctly formed', t => {
[Transaction.makeOutput(Transaction.makeEd25519Condition(alice.publicKey))],
[0]
)
const msg = Transaction.serializeTransactionIntoCanonicalString(txTransfer)
const txSigned = Transaction.signTransaction(txTransfer, alice.privateKey)

const msg = Transaction.serializeTransactionIntoCanonicalString(txTransfer)
t.truthy(cc.validateFulfillment(txSigned.inputs[0].fulfillment,
txCreate.outputs[0].condition.uri,
new Buffer(msg)))
Expand Down
7 changes: 5 additions & 2 deletions test/transaction/test_transaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import sinon from 'sinon'
import { Transaction } from '../../src'
import * as makeTransaction from '../../src/transaction/makeTransaction' // eslint-disable-line
import makeInputTemplate from '../../src/transaction/makeInputTemplate'
import makeFulfillment from '../../src/transaction/makeFulfillment'

import {
alice,
Expand Down Expand Up @@ -85,7 +86,8 @@ test('Create TRANSFER transaction based on CREATE transaction', t => {
[aliceOutput],
[makeInputTemplate(
[alice.publicKey],
{ output_index: 0, transaction_id: createTx.id }
{ output_index: 0, transaction_id: createTx.id },
makeFulfillment([alice.publicKey])
)]
]

Expand Down Expand Up @@ -113,7 +115,8 @@ test('Create TRANSFER transaction based on TRANSFER transaction', t => {
[aliceOutput],
[makeInputTemplate(
[alice.publicKey],
{ output_index: 0, transaction_id: transferTx.id }
{ output_index: 0, transaction_id: transferTx.id },
makeFulfillment([alice.publicKey])
)]
]

Expand Down