Using keyword-only arguments forces people away from long lists of parameters that impair future readability.

E.g. Transaction's init.

The current exceptions all inherit from Python's Exception class. This ticket wishes to review the current design, and to consider having some "key" parent classes wherever it makes sense.

From bigchaindb/bigchaindb#641 (comment):

... if a certain group of exceptions share certain characteristics, and for this reason are often caught together (as above), then it may make sense to have a parent class from which these classes inherit. One advantage of doing so would be that in some cases the parent class can be caught if all children are involved.

try:
    something_may_go_wrong()
except (ValueError, OperationError, TransactionError):
    handle_what_went_wrong()

instead of

try:
    something_may_go_wrong()
except (ValueError, OperationError, TransactionDoesNotExist, TransactionOwnerError,
        DoubleSpend, InvalidHash, InvalidSignature):
    handle_what_went_wrong()

The simpler version would require something like:

class TransactionError(Exception):
    """Base class for all transaction related exceptions.""""

class TransactionDoesNotExist(TransactionError):
    """Raised if the transaction is not in the database"""

class TransactionOwnerError(TransactionError):
    """Raised if a user tries to transfer a transaction they don't own"""

class DoubleSpend(TransactionError):
    """Raised if a double spend is found"""

class InvalidHash(TransactionError):
    """Raised if there was an error checking the hash for a particular operation"""

Please note that is is just the idea, and the grouping may be done differently.

As of now, we do very little to ensure that the given inputs are what we expect them to be when constructing the models. We really should be doing at least some checks with json schema or any other validation approach; not doing so results in the worst and most nefarious types of bugs for users.

Earlier, I created a transfer transaction and accidentally passed through a keypair dict as part of the owners_after argument in the driver (rather than just a string verifying key). The driver was able to create, sign, and send this wrongly-formatted condition all the way to BigchainDB, who accepted it and wrote it onto RethinkDB (see below). Now my public/private keypair is forever visible to the world, and this condition is probably impossible to fulfill.

If I was a naive user and assumed everything had worked correctly, since nothing had told me otherwise, I might now start creating n transactions this way without ever realizing that I've completely shot myself in the foot. Oops.

What really should've happened is me getting an error immediately upon the owners_after signature being wrong. Crisis averted.

The persisted transaction, available in a block in RethinkDB:

{'id': '960a684bb92de50384ebf568b7e1e78d88f8afbb674872b3d25356eae77fba51',
 'transaction': {'conditions': [{'cid': 0,
                                 'condition': {'uri': {'signing_key': '4S1dzx3PSdMAfs59aBkQefPASizTs728HnhLNpYZWCad',
                                                       'verifying_key': '2dBVUoATxEzEqRdsi64AFsJnn2ywLCwnbNwW7K9BuVuS'}},
                                 'owners_after': [{'signing_key': '4S1dzx3PSdMAfs59aBkQefPASizTs728HnhLNpYZWCad',
                                                   'verifying_key': '2dBVUoATxEzEqRdsi64AFsJnn2ywLCwnbNwW7K9BuVuS'}]}],
                 'data': None,
                 'fulfillments': [{'fid': 0,
                                   'fulfillment': 'cf:4:4HwjqBgNkDK0fD1ajmFn0OZ75N3Jk-xIV2zlhgPxP2a7KeSZy56l5uU_Wqj6FmQz9O8xUYS3eIdR0RGDAobM4grfbtmeWbv4FXwtd5FyYbh6Azs4sle_eYFOKwsGREcL',
                                   'input': {'cid': 0,
                                             'txid': 'fad5ec32672b75cccf92992cc977f456bd587a7d49956fed67a227fb26d13250'},
                                   'owners_before': ['G7J7bXF8cqSrjrxUKwcF8tCriEKC5CgyPHmtGwUi4BK3']}],
                 'operation': 'TRANSFER',
                 'timestamp': '1475228620'},
 'version': 1}

Notice the condition, which lists the entire given dict:

{
    ...
    'conditions': [{'cid': 0,
                    'condition': {'uri': {'signing_key': '4S1dzx3PSdMAfs59aBkQefPASizTs728HnhLNpYZWCad',
                                          'verifying_key': '2dBVUoATxEzEqRdsi64AFsJnn2ywLCwnbNwW7K9BuVuS'}},
                    'owners_after': [{'signing_key': '4S1dzx3PSdMAfs59aBkQefPASizTs728HnhLNpYZWCad',
                                      'verifying_key': '2dBVUoATxEzEqRdsi64AFsJnn2ywLCwnbNwW7K9BuVuS'}]}],
    ...

(In fact, I probably could've put anything there; it just so happened that it was the keypair.)

At the very least, as suggested by @sbellem, it should be made very clear (with big warning signs and all that) that whatever you give that is turned into the owners_after is forever written in the transaction's condition. We could also add some sort of identity verification system, but that's out of scope.

cc @TimDaub @diminator

In multiple places, function/method arguments are required to be lists. As an interface user, it may be frequent that I only need to pass one element, in which case I need to do the work of casting that element into a list.

This issue proposes to move the burden of casting a single element into a list from the library user onto the library code.

As an example, consider the following snippet extracted from https://github.com/bigchaindb/bigchaindb-common/blob/master/bigchaindb_common/transaction.py#L384:

class Transaction(object):
    CREATE = 'CREATE'
    # ...

    @classmethod
    def create(cls, owners_before, owners_after, metadata=None, asset=None,
               secret=None, time_expire=None):
        if not isinstance(owners_before, list):
            raise TypeError('`owners_before` must be a list instance')
        if not isinstance(owners_after, list):
            raise TypeError('`owners_after` must be a list instance')

rather than raising errors when arguments owners_before and owners_after are not list instances, the code could be changed to support both str and list instances`:

    # ...
        if isinstance(owners_before, str):
            owners_before = [owners_before]
        elif not isinstance(owners_before, list):
            raise TypeError('`owners_before` must be a list instance')

something along those lines.

The proposed change does is backward compatible, and would simplify the code using the common library.

@diminator gave a hint here: #4 (comment)

Some functions are currently documented very lightly.
Especially the public ones should give more information about implementation specific details.

This somewhat pretentious issue is meant to make the code more pythonic. The changes it proposes will be backed by external references (e.g.: PEP 8).

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

# Yes:
if not seq:
     if seq:

# No:
if len(seq):
    if not len(seq):

ref: PEP 8 Programming Recommendations

For example (https://github.com/bigchaindb/bigchaindb-common/blob/master/bigchaindb_common/transaction.py#L353):

elif len(owners_before) > 0 and len(owners_after) == 0 and secret is None:

could be changed to:

elif owners_before > 0 and not owners_after and secret is None:

Accessing a class attribute/constant via self alone

This needs a good "Why?" reference, but the point is that, in the following snippet:

class Transaction(object):
    VERSION = 1

    def __init__(self, version=None):
        self.version = self.__class__.VERSION

the class attribute/constant VERSION can be accessed in a simpler manner via self alone:

class Transaction(object):
    VERSION = 1

    def __init__(self, version=None):
        self.version = self.VERSION

Assets and metadata don't have hashes associated with them (yet?), so the naming of to_hash() to get a uuid is somewhat misleading.

this is for setup.py, and wherever else needed

We have a lot of:

CONSTANT = 'constant'

@fixture
def constant():
    return CONSTANT

when they can just be

@fixture
def constant():
    return 'constant'

We shouldn't ever be using these constants outside of their fixtures anyway.

When creating a CREATE transaction, currently only the first owner_before is taken as the "issuer" of the asset.

Instead all owners_before should be taken into account and required to sign the transaction thresholdOfThresholdCondition == len(owners_before)

//cc @r-marques @sbellem

I just had a talk with @vrde and @rhsimplex about the complex naming regarding fulfillments, conditions, inputs and outputs. In the last couple of weeks I had quite a few people that were confused by this. Things that came up:

• How is a fulfillment in a CREATE transaction generated. Why is input == None?

This is usually referred to as the implicit condition. A condition is generated, immediately a fulfillment is derived from it, signed and its URI is put as fulfillments[x].fulfillment.

• What's input (and output) in regard to fulfillment and condition?

If you think of a door then fulfillment is the key to a lock (a condition). inputs and outputs define the relationship between doors (e.g. can a certain key open a certain lock).

A few thoughts from myself:

• Could conditions and fulfillments be renamed/simplified to inputs and outputs in the transaction model? Would that still be compliant with the Interledger spec?

or

• Could we remove all notion of inputs and outputs and comply to some other language that is used within Interledger?
• What is fulfillment[x].fulfillment and why is a transaction not signed?

fulfillment[x].fulfillment is the signature of a transaction.

A few thoughts from myself:

• Ideally, a fulfillment (meaning the URI part) should be stored outside of the transaction's body (compare to the Block model) so that validation doesn't have to rearrange/delete/ certain keys.

@vrde @rhsimplex Please add more if you can.

The goal of this ticket is to have a discussion around simplifying the language (only fulfillments, conditions, inputs, outputs) in a transaction, to make it easier for newcomers to understand.
There are already threads for other concepts:

• Rename 'owners_after' to 'fulfillers'

//cc @r-marques and @ttmc as it might be interesting for them as well

A number of nice things we can add to make transactions more pleasant to work with:

• Expose a builder, potentially removing the create and transfer factory methods
• Make Transaction immutable; signing a Transaction should output a new transaction that's been signed
  • Perhaps use read-only properties for the instance variables, and hide their implementations

In both Transaction.create and Transaction.transfer

This concerns cases for which the payload is None, when creating a transaction, e.g.:

tx = Transaction.create(owners_before, owners_after, payload=None)

The new behaviour makes it so that transactions without payloads and duplicate timestamps, created within a loop for instance, will hash to the same value. The legacy behaviour was such that a uuid would be generated for the data field, regardless of whether the payload was None or not, which would prevent duplicate hashes.

The "legacy" behaviour was:

data = {
    'uuid': str(uuid.uuid4()),
    'payload': payload
}

regardless of whether payload is None or not.

This would give something like:

ipdb> pp tx.to_dict()['transaction']['data']
{
    'uuid': 'asdghasgdfds...',
    'payload': None
}

The new behaviour is given by:

    def to_dict(self):
        if self.payload is None:
            return None
        else:
            return {
                'payload': self.payload,
                'uuid': self.payload_id,
            }

which gives:

ipdb> pp tx.to_dict()['transaction']['data']
None

For a list, see: https://github.com/bigchaindb/bigchaindb-common/search?utf8=%E2%9C%93&q=TODO

class Asset(object):
    def __init__(self, data=None, data_id=None, divisible=False,
                 updatable=False, refillable=False):
        self.data = data
        self.data_id = data_id if data_id is not None else self.to_hash()
        self.divisible = divisible
        self.updatable = updatable
        self.refillable = refillable

    def to_dict(self):
        return {
            'id': self.data_id,
            'divisible': self.divisible,
            'updatable': self.updatable,
            'refillable': self.refillable,
            'data': self.data,
        }

doing so prevents one from passing directly the results of Asset.to_dict() to the Python built-in class initialization

It may be depends on the model, but let's consider the Asset model, as an attempt to make point:

>>> pp asset_dict
{'data': {'a': 'b'},
 'data_id': 'ebd621db-53fa-4eea-ba1b-334605a2b0e9',
 'divisible': False,
 'refillable': False,
 'updatable': False}
>>> asset_model = Asset(**asset_dict)
>>> asset_model.__dict__ == asset_dict
True

In response to bigchaindb/bigchaindb#599 (which are the specs for this ticket).

TODOs:

• Rename payload to metadata
• Implement Asset class
• Integrate Assets into Transaction.create and Transaction.transfer
• Integrate Assets into Transaction constructor

//cc @r-marques

In transaction.py, it's trying to serialize an asset, which is not provided in the case of a genesis transaction, so I think this will fail.

Can someone confirm that a GENESIS transaction shouldn't have an asset?

Edit: Worth noting that maybe we could still remove GENESIS transaction altogether ala bigchaindb/#444.

Relates to bigchaindb/cryptoconditions#28 and bigchaindb/bigchaindb#617

The new changes to cryptoconditions change how encoding is handled.
The api was also simplified which may introduce breaking changes.

Haven't double checked all of them, but a large number of the exceptions could be removed since they're BDB-specific.

Looking at how the asset is implemented and to maintain consistency the metadata object should be created and then passed to Transaction, Transaction.create, Transaction.transfer

I find the name data_id confusing in both the Metadata and Asset model.
Since they are already inside a namespace I propose to rename data_id to id.
This way we have:

• Transaction.id
• Transaction.metadata.id
• Transaction.asset.id

This method: https://github.com/bigchaindb/bigchaindb-common/blob/master/bigchaindb_common/transaction.py#L435

Currently, a transaction can just be signed using all the corresponding private keys.
In a real-life scenario this is often unpractical. Hence, Transaction.sign should accept a partial set of keys to sign a transaction (while still be (de)serializable to_dict, from_dict).

Currently Transaction.transfer does not support creating hashlocks as fulfillments or conditions.

At one point I decided that str() is super useful when signing transactions internally.
Now, I just used it externally and had to find out that it removes all signatures (which is obviously quite weird in some use cases).

Solution:

• Have a to-string method internally that gets rid of the signatures
• Have a to-string method externally that just deserializes the transaction 1:1

See these tests: https://github.com/bigchaindb/bigchaindb/pull/641/files#diff-5832adf374920d75d4bf48e546367f53L165

Make sure they're integrated into this repo.

//cc @diminator