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

Updated data models for 1.0 #35

Merged
merged 10 commits into from
Jun 20, 2017
Merged

Updated data models for 1.0 #35

merged 10 commits into from
Jun 20, 2017

Conversation

TimDaub
Copy link
Contributor

@TimDaub TimDaub commented Jun 16, 2017

So far:

  • change fulfills.txid to `transaction_id
  • stringify outputs amount
  • wrote tests for makeOutput

TODO:

  • test makeTransferTransaction

@vrde: Should we mimick the folder structure of src in test?
For file structure, I think everything in src/transaction should be combined into one class as we did with src/connection, hence only one test file called test_transaction.js.

@@ -8,7 +8,7 @@
*/
export default function makeOutput(condition, amount = 1) {
return {
'amount': amount.toString(),
'amount': amount,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of stringifying, strings have to be passed directly now.

@vrde
Copy link
Contributor

vrde commented Jun 16, 2017

@vrde: Should we mimick the folder structure of src in test?

Yep.

For file structure, I think everything in src/transaction should be combined into one class as we did with src/connection, hence only one test file called test_transaction.js.

Agree.

For now I've just added a simple integration test that pushes a transaction to BigchainDB, do you want to extend the test suite in this PR? Maybe we can add more "simple" integration tests for now, and then work on the unit test, what do you think?

@TimDaub TimDaub changed the title [WIP] 1.0 rc compatiblity [WIP] Updated data models for 1.0 Jun 19, 2017
@@ -2,6 +2,32 @@ import makeInputTemplate from './makeInputTemplate'
import makeTransaction from './makeTransaction'


// TODO: Can we remove `export` here somehow, but still be able to import the
// function for tests?
export function _makeTransferTransaction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that I should maybe not do this whole private function thing.
I did it initially for testing, however I should use mocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone knows how to mock a export default function, let me know and I can change this.

@TimDaub TimDaub mentioned this pull request Jun 19, 2017
2 tasks
@TimDaub TimDaub force-pushed the 1.0-rc-compatiblity branch from 46d8e61 to 3389726 Compare June 20, 2017 09:05
@TimDaub TimDaub changed the title [WIP] Updated data models for 1.0 Updated data models for 1.0 Jun 20, 2017
@TimDaub TimDaub requested review from vrde and ssadler June 20, 2017 09:58
@@ -18,7 +18,7 @@ before_install:
-e BIGCHAINDB_KEYPAIR_PRIVATE=5C5Cknco7YxBRP9AgB1cbUVTL4FAcooxErLygw1DeG2D
-e BIGCHAINDB_DATABASE_BACKEND=mongodb
-e BIGCHAINDB_DATABASE_HOST=172.17.0.1
bigchaindb/bigchaindb:0.10.2
bigchaindb/bigchaindb:master
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we want to change this to 1.0.0rc1 at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup

@@ -21,6 +21,8 @@ import makeTransaction from './makeTransaction'
* @returns {object} Unsigned transaction -- make sure to call signTransaction() on it before
* sending it off!
*/
// TODO: `outputs` should throw or include output in array if no array was
// passed
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean specifically? L25 or the L24-25 comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove TODO

})

const assetLink = {
'id': unspentTransaction.operation === 'CREATE' ? unspentTransaction.id
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have a function getAssetId(transaction) somewhere.


return makeTransaction('TRANSFER', assetLink, metadata, outputs, inputs)
// TODO:
Copy link
Contributor

Choose a reason for hiding this comment

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

TODOs in master?

public_keys: []
}
const res = Transaction.makeOutput(condition)
return t.deepEqual(res, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure the return is needed here

public_keys: ['abc']
}
const res = Transaction.makeOutput(condition)
return t.deepEqual(res, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure return is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is. Thx. I'll fix, also here

@@ -2,6 +2,32 @@ import makeInputTemplate from './makeInputTemplate'
import makeTransaction from './makeTransaction'


// TODO: Can we remove `export` here somehow, but still be able to import the
// function for tests?
export function _makeTransferTransaction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@libscott and I created: #44

unspentTransaction,
metadata,
outputs,
...fulfilledOutputs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to: outputIndexes



// TODO: Find out if ava has something like conftest, if so put this there.
const alice = new Ed25519Keypair()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use output of respective functions as strings/dicts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue: #45

@TimDaub TimDaub merged commit 0d284c7 into master Jun 20, 2017
@kremalicious kremalicious deleted the 1.0-rc-compatiblity branch June 20, 2017 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants