Initial interface design #1

TimDaub · 2016-10-10T14:59:06Z

This was part of a first effort to make it possible for users of BigchainDB to create and sign transactions independently from the programming language they'd like to use.

The motivation of existence behind this tool was outlined by me in the following two comments:

TL;DR:

The idea is to have a command line tool that allows to write simple wrappers in the user's programming language of choice. I was inspired by bitpay's bitcoin-rpc lib: https://github.com/bitpay/bitcoind-rpc/blob/master/lib/index.js

With this PR, I've been mainly following the API that is provided by BDB-C. Specifically the following methods are of interest:

What has been done:

Private and public keys can be generated using $ bdb generate keys
A CREATE transaction can be generated using $ bdb generate create
Any Ed25519 or ThresholdSha256 Condition can be fulfilled (signed) using $ bdb sign
Any transactions' conditions can be transformed to "inputs" (for using them in a new transaction) using $ bdb spend

What is left to be done:

(https://docs.bigchaindb.com/projects/server/en/latest/drivers-clients/python-server-api-examples.html) and here

Tests for everything (We should aim for 100% code coverage, CI using Travis, ...)
Resolve all outstanding TODOs and FIXMEs in the code
Refactor the $ bdb generate create command such that it takes an arbitrary amount of JSON conditions instead of the current owner_after argument
Implement the command $ bdb generate transfer that takes inputs taken from bdb spend, as well as an arbitrary amount of JSON conditions (Note: that the command requires two unlimited variadic arguments which might be tricky to achieve)
Review cli method names (create/generate_tx, sign, etc)
Review cli api for security (should private keys be in argv really?)

Make tickets for:

Documentation by "real-life" examples, similar to [here]
Integration tests (@sbellem should probably be consulted before doing that)
Write a guideline how developers can use this to easily implement wrappers for the programming language of their choice
Support all cryptoconditions BigchainDB currently supports
Use pyinstaller to automatically create an executable (and if possible automatically deploy that to OS-package-manager-repositories)

Obviously all the TODOs mentioned here do not have to be handled in this PR, but instead tickets can be created to enable iterative development.

TimDaub · 2016-10-10T15:10:03Z

bdb_transaction_cli/cli.py

+        click.echo('{} {}'.format(private_key, public_key))
+
+
+# TODO:


The two TODOs here might be ignored initially.

TimDaub · 2016-10-10T15:10:29Z

bdb_transaction_cli/cli.py

+@spend_group.command()
+@click.argument('transaction', type=DictParamType())
+@click.argument('condition_id', required=False, type=click.INT, nargs=-1)
+# TODO: option to output JSON list


Optional requirement.

…pypi

…t it's breaking #breakstuff

ssadler · 2016-10-13T08:03:40Z

s/bdb-transactions-cli/bdb-transaction-cli/g

* rename (back) to * create utils module for helpers like json_param

sbellem · 2016-10-19T14:52:52Z

setup.py

 ]

 test_requirements = [
-    # TODO: put package test requirements here
 ]


Why not put the test requirements?

I'll make an issue about this!

sohkai · 2016-10-19T14:54:45Z

bdb_transaction_cli/cli.py

+    Generates a Ed25119 condition from a OWNER_AFTER or a ThresholdSha256
+    Condition from more than one OWNER_AFTER.
+    """
+    condition = Condition.generate(list(owner_after))


Why not [] instead of list()?

[] and list() don't do the same thing, the first wraps, the second converts.

The conversion is because the owners_after argument is variadic and click is providing a tuple, and in transactions.py you have the following, so it breaks:

if isinstance(owners_after, tuple): owners_after, threshold = owners_after else: threshold = len(owners_after)

sohkai · 2016-10-19T14:59:17Z

bdb_transaction_cli/utils.py

+    """ Decorator for a JSON command line argument """
+    kwargs['type'] = JsonParamType()
+    if 'help' in kwargs:
+        del kwargs['help']


Is there a reason we remove the help? Why can't we just do something like @click.argument(..., type=JSON_TYPE), where JSON_TYPE is an exported instance of JsonParamType?

Ohhhhh... Actually I had forgotten about that.

Yes, I created that function to reduce the boilerplate, and deleting that key was a temporary workaround for a strange bug. Now I'll revisit it, and I might well just remove the function as you say.

sohkai · 2016-10-19T14:59:35Z

bdb_transaction_cli/cli.py

+@json_argument('transaction')
+def get_asset(transaction):
+    """
+    Return the asset from a transaction for the purpose of providing as an


providing it as

Changed. No shorthand, generally?

Maybe it's my Canadian English, but it read awkwardly without it 😉

sohkai · 2016-10-19T14:59:57Z

bdb_transaction_cli/cli.py

+    Return the asset from a transaction for the purpose of providing as an
+    input to `transfer`.
+    """
+    click.echo(json.dumps({"id": transaction['transaction']['asset']['id']}))


From the docstrings, it sounds like we want the full asset, not just the id?

Actually, I want to make another method, generate_asset for this purpose. AFAIK, in a TRANSFER tx you just want the ID (tests pass anyhow), but in a CREATE tx you want a full asset.

sohkai · 2016-10-19T15:00:14Z

bdb_transaction_cli/cli.py

+    Outputs a signed transaction.
+    """
+    transaction = Transaction.from_dict(transaction)
+    transaction = transaction.sign(list([private_key]))


Unnecessary list().

Yep... yep, yep.

sohkai · 2016-10-19T15:07:23Z

bdb_transaction_cli/cli.py

+    ffill = Fulfillment(Ed25519Fulfillment(public_key=author_pubkey),
+                        [author_pubkey])
+    conditions = [Condition.from_dict(c) for c in listify(conditions)]
+    asset = asset and Asset.from_dict(asset)


This might get you in trouble, e.g. if I use {} as asset, {} will get passed to the Transaction init. Using something like

if asset is not None: asset = Asset.from_dict(asset)

is likely better.

Actually, I think asset = Asset.from_dict(asset) if asset else None would be even better. Protects against other values which do not evaluate to True, like an empty space.

Some of these other bad values should get picked up by the json validator, but I agree.

sohkai · 2016-10-19T15:08:16Z

bdb_transaction_cli/utils.py

+    name = 'JSON'
+
+    def convert(self, value, param, ctx):
+        # No try-except in case `value is None`, as it is a valid value.


Not sure how click's other types behave, but it might be useful to name this JsonOrNoneParamType for clarity.

sohkai · 2016-10-19T15:10:14Z

docs/examples.rst

+   }'
+
+
+   $ bdb create 35qDXhZTUvna23NLc1hMfmrgPniBwPgNjko1VfQuD3vF '$CONDITIONS'


Should be double (") quotes, not single (') quotes for $CONDITIONS

(and for the other examples too)

Not if you're passing a JSON object, I would have thought. This is shell script remember.

But using single quotes doesn't expand the variable; so cat '$CONDITIONS' would just print $CONDITIONS. Unless I'm missing something?

sohkai · 2016-10-19T15:12:43Z

docs/examples.rst

+   }'
+
+
+   $ bdb create 35qDXhZTUvna23NLc1hMfmrgPniBwPgNjko1VfQuD3vF '$CONDITIONS' --asset={"divisible": false, "data": {"b": 1}, "refillable": false, "id": "a", "updatable": true}


The json should be surrounded by single (') quotes.

sohkai · 2016-10-19T15:19:09Z

docs/examples.rst

+   }'
+
+
+   $ bdb create 35qDXhZTUvna23NLc1hMfmrgPniBwPgNjko1VfQuD3vF '$CONDITIONS' --asset={"data": {"b": 1}, "divisible": false, "refillable": false, "id": "a", "updatable": true}


The json here should be enclosed in single quotes.

And for the other examples too.

Yes, this is a bug I don't actually have time to fix right now, so I'll make an issue.

sohkai · 2016-10-19T15:21:43Z

docs/usage.rst

@@ -2,6 +2,120 @@
 Usage
 =====

-To use bdb-transaction-cli in a project::
+Here is demonstrated how Alice can make two transactions, first to


Slight wording (or similar):

Here we demonstrate how Alice can make two transactions:

sohkai · 2016-10-19T15:23:29Z

docs/usage.rst

+
+The required inputs to be able to do this are the public key of the
+validating node (server) that the transaction will be first submitted
+to.


Paragraph needs some rewording.

Yes, I updated the graph and the comment is redundant.

sohkai · 2016-10-20T07:54:19Z

LGTM -- waiting on @sbellem.

TimDaub

Created #10 for all bigchaindb.common related comments.

TimDaub · 2016-11-08T16:27:25Z

bdb_transaction_cli/cli.py

+
+
+@main.command()
+@click.argument('author_pubkey')


In all other packages, we call this owner_before or owners_before.
Additionally, a lot of blood has been shed about the naming of these variables in Github issues. A consensus yet as to emerge, so before it does I'd prefer to call this by it's current name: owner_before.

TimDaub · 2016-11-08T16:30:52Z

bdb_transaction_cli/cli.py

+    Convert a transaction's outputs to inputs.
+
+    Convert conditions in TRANSACTION (json) to signable/spendable
+    fulfillments. Conditions can individually selected by passing one or more


Conditions can be individually selected by passing

TimDaub · 2016-11-09T09:16:57Z

requirements_dev.txt

-cryptography==1.4
-PyYAML==3.11
-pytest==2.9.2
+.[dev]


TIL, nice trick.

TimDaub · 2016-11-09T09:17:51Z

tests/test_bdb_transaction_cli.py


 import pytest
-
-from contextlib import contextmanager
+from bigchaindb_common.exceptions import KeypairMismatchException


Shouldn't this be bigchaindb.common now?

TimDaub · 2016-11-09T09:20:46Z

bdb_transaction_cli/cli.py


 import click
+from bigchaindb_common.crypto import generate_key_pair


bigchaindb.common

TimDaub · 2016-11-09T09:20:49Z

bdb_transaction_cli/cli.py


 import click
+from bigchaindb_common.crypto import generate_key_pair
+from bigchaindb_common.transaction import Transaction, \


bigchaindb.common

TimDaub · 2016-11-09T09:33:46Z

tests/test_bdb_transaction_cli.py

+RECORD_EXAMPLES = 'RECORD_EXAMPLES' in os.environ
+
+
+@patch('bigchaindb_common.transaction.gen_timestamp', lambda: 42)


bigchaindb.common

TimDaub · 2016-11-09T09:33:49Z

tests/test_bdb_transaction_cli.py

+
+
+@patch('bigchaindb_common.transaction.gen_timestamp', lambda: 42)
+@patch('bigchaindb_common.transaction.Asset.to_hash', lambda self: ASSET['id'])


bigchaindb.common

TimDaub · 2016-11-09T09:34:14Z

setup.py

@@ -11,29 +11,42 @@

 requirements = [
    'Click>=6.0',
-    # TODO: put package requirements here
+    'bigchaindb-common>=0.0.3',


import bigchaindb here

Initial interface design

179aee3

TimDaub commented Oct 10, 2016

View reviewed changes

ssadler added 6 commits October 11, 2016 10:38

remove Stable release installation from docs until it's available on …

b74d9f8

…pypi

rename entry point to main

2cd69e8

pep8 fixes

3a8aa11

rename DictParamType

29ffb98

reset tests

e46cde3

move all commands to top level

84bcb2c

ssadler mentioned this pull request Oct 12, 2016

0.1 release #2

Closed

ssadler added 6 commits October 12, 2016 12:30

test for create_tx

1a5b93f

test generate_condition

94ad0b6

use pytest

1fbddd9

test for spend

60c0e35

test key generation

104a1bc

remove --type option for key gen

4738fec

ssadler force-pushed the initial-interface-design branch from 88a71b7 to 4738fec Compare October 12, 2016 13:54

ssadler added 4 commits October 12, 2016 15:54

test sign

7ecd901

remove pyinstaller 'import decimal' workaround until more obvious wha…

f8de66f

…t it's breaking #breakstuff

remove (currently) unfixable thing

9e4ea5c

add some TODOs

a555bc1

ssadler added 2 commits October 13, 2016 10:11

module refactor; rename executable to 'bdb'

815c7c2

documentation cleanup

f79aa95

TimDaub mentioned this pull request Oct 13, 2016

Add semi RPC commands to the API bigchaindb/bigchaindb#639

Closed

* fix tests to accomodate changes in model (asset)

9a427cd

* rename (back) to * create utils module for helpers like json_param

ssadler force-pushed the initial-interface-design branch from bc3e685 to 9a427cd Compare October 17, 2016 09:56

ssadler added 2 commits October 17, 2016 12:39

pass conditions directly into create method

028c044

add graphviz flow diagrams for documentation

af52be2

ssadler force-pushed the initial-interface-design branch from b794bc2 to af52be2 Compare October 17, 2016 16:02

sbellem reviewed Oct 19, 2016

View reviewed changes

sohkai reviewed Oct 19, 2016

View reviewed changes

change default output of generate_keys to JSON

58eacac

sohkai reviewed Oct 19, 2016

View reviewed changes

remove code which was a temporary workaround for a strange issue

60f5141

sohkai reviewed Oct 19, 2016

View reviewed changes

ssadler added 2 commits October 19, 2016 17:20

review fixes

857ef34

update dev installable version to [dev]

966fa7d

sohkai reviewed Oct 19, 2016

View reviewed changes

ssadler added 2 commits October 19, 2016 17:27

more review fixes

48a0807

fix bash variable escaping in examples.rst

455b377

ssadler added 6 commits November 8, 2016 13:41

Add basic feature list in README

ee3d075

fix references and mkvirtualenv call in CONTRIBUTING.rst

a04b106

move installation documentation to README.me

84dbb40

setup.py and requirements_dev.txt fixes

9fdf032

fix optional arguments in invocation examples generator

87961cc

fix tests using pytest

0f06b6a

TimDaub commented Nov 9, 2016

View reviewed changes

tim added 2 commits November 9, 2016 10:48

author_pubkey --> owner_before

bb5fd0a

Fix typo

6e81cc4

TimDaub merged commit 81d9eee into master Nov 9, 2016

		}'


		$ bdb create 35qDXhZTUvna23NLc1hMfmrgPniBwPgNjko1VfQuD3vF '$CONDITIONS'


		import click
		from bigchaindb_common.crypto import generate_key_pair

		RECORD_EXAMPLES = 'RECORD_EXAMPLES' in os.environ


		@patch('bigchaindb_common.transaction.gen_timestamp', lambda: 42)



		@patch('bigchaindb_common.transaction.gen_timestamp', lambda: 42)
		@patch('bigchaindb_common.transaction.Asset.to_hash', lambda self: ASSET['id'])



		@main.command()
		@click.argument('author_pubkey')

Initial interface design #1

Initial interface design #1

Conversation

TimDaub commented Oct 10, 2016 • edited by ssadler Loading

TimDaub Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssadler commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssadler Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Oct 20, 2016

TimDaub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub commented Oct 10, 2016 •

edited by ssadler

Loading

TimDaub Oct 10, 2016 •

edited

Loading

ssadler Oct 19, 2016 •

edited

Loading

sohkai Oct 19, 2016 •

edited

Loading

sohkai Oct 19, 2016 •

edited

Loading

sohkai Oct 19, 2016 •

edited

Loading