Draft 02 impl #67

sbellem · 2017-06-20T09:18:01Z

Note to reviewers

Since this is a relatively big PR, just ask me if you have questions, chances are that it'll be faster.

I have tried to minimize any changes except those needed for the upgrade.

Moreover the changes are more or less a translation of the JS implementation.

Known things left to do

Docs in various places.
~~- [ ] Duplicate tests under tests/test_generated_valid_spec.py~~ moved to issue Do something about the duplicate tests #78
code coverage needs to be improved

codecov-io · 2017-06-20T09:21:24Z

Codecov Report

Merging #67 into master will increase coverage by 3.68%.
The diff coverage is 87.86%.

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage    85.5%   89.19%   +3.68%     
==========================================
  Files          15       15              
  Lines         766      824      +58     
==========================================
+ Hits          655      735      +80     
+ Misses        111       89      -22

r-marques · 2017-06-20T14:38:12Z

cryptoconditions/__init__.py

+    fulfillment = Fulfillment.from_uri(serialized_fulfillment)
+    condition_uri = fulfillment.condition_uri
+    if condition_uri != serialized_condition:
+        raise Exception(


Maybe we should define an exception for this case.

r-marques · 2017-06-20T14:43:26Z

cryptoconditions/__init__.py

+
+def fulfillment_to_condition(serialized_fulfillment):
+    fulfillment = Fulfillment.from_uri(serialized_fulfillment)
+    return fulfillment.condition_uri()


condition_uri is a property

r-marques · 2017-06-20T16:05:16Z

cryptoconditions/condition.py

@@ -74,63 +94,98 @@ def from_uri(serialized_condition):
        Returns:
            Condition: Resulting object
        """
+        # TODO consider removing.


Agree. Don't see the reason for it. The check to see if its a string should be enough

r-marques · 2017-06-21T07:51:29Z

cryptoconditions/condition.py

-        if not re.match(CONDITION_REGEX_STRICT, serialized_condition):
-            raise ValueError('Invalid condition format')
-
+        if pieces[0] != CONDITION_URI_SCHEME:


Is there a need for this check here? Wouldn't the regex_match also catch this error?

Yes, I think the regex would be enough. I kept it there as this check was already there.

I guess it is useful to give a more explicit error message as to what is wrong.

r-marques · 2017-06-21T08:03:30Z

cryptoconditions/condition.py

-    def max_fulfillment_length(self, value):
+    # TODO update docstrings
+    @cost.setter
+    def cost(self, value):


If we want to be consistent we should to cost the same we did to hash. Check that the value has the correct type on the setter and and raise a ValueError on the property if the value is not set.

r-marques · 2017-06-21T08:21:56Z

cryptoconditions/condition.py

-        # Bitmask can have at most 32 bits with current implementation
-        if self.bitmask > Condition.MAX_SAFE_BITMASK:
-            raise ValueError('Bitmask too large to be safely represented')
+        # Subtypes can have at most 32 bits with current implementation


I don't understand this check. We are not dealing with bits here. We are only checking the number of subtypes.
I think the next check for supported subtypes should be enough.

Yeah, it is somewhat clumsy. This was done quickly, as the JS implementation had yet to be updated there. See interledgerjs/five-bells-condition#67

r-marques · 2017-06-21T09:10:58Z

cryptoconditions/condition.py

+        condition_type = TypeRegistry.find_by_type_id(self.type_id)
+        condition_class = condition_type['class']
+        asn1_dict = {
+            'type': condition_class.TYPE_ASN1_CONDITION,


We can just use condition_type['asn1_condition'] instead of declaring condition_class

hmmm, yes, we need it for the TYPE_CATEGORY though ...

r-marques · 2017-06-21T09:14:42Z

cryptoconditions/condition.py

        """
+        # TODO Use more precise Exception class
+        if len(value) != 32:
+            raise Exception(


Would it make sense to raise a ValueError here instead of Exception?

Changing it now.

addressed in 91d6852

r-marques · 2017-06-21T11:25:34Z

cryptoconditions/condition.py

 # This is a stricter version based on limitations of the current implementation.
 # Specifically, we can't handle bitmasks greater than 32 bits.
-CONDITION_REGEX_STRICT = \
-    r'^cc:([1-9a-f][0-9a-f]{0,3}|0):[1-9a-f][0-9a-f]{0,7}:[a-zA-Z0-9_-]{0,86}:([1-9][0-9]{0,17}|0)$'
+CONDITION_REGEX_STRICT = CONDITION_REGEX


Is there a reason to have both CONDITION_REGEX_STRICT and CONDITION_REGEX since they are now the same?

I know 😄

we can clean that yes

r-marques · 2017-06-21T11:27:20Z

cryptoconditions/condition.py

+            raise ParsingError(
+                'Invalid condition format: "cost" parameter or value missing.')
+
+        if not re.match(INTEGER_REGEX, cost):


Isn't this the same as int(cost)?
It would eliminate the need for INTEGER_REGEX

oh, sure

issue: #72

r-marques · 2017-06-21T11:55:08Z

cryptoconditions/condition.py

+        condition.cost = value['cost']
+        condition._subtypes = set()
+        if registered_type['class'].TYPE_CATEGORY == 'compound':
+            subtypes = {


Nice use of itertools. But I feel that this could be in a separate method. Something like subtypes_from_bitstring and subtypes_to_bitstring for to_ans1_dict

Doesn't the asn1 lib provide something to do this?

Ah ok, good point!

r-marques · 2017-06-21T12:03:40Z

cryptoconditions/condition.py

+    # The asn1 json payload that ILP uses in the JS implementation differs from
+    # the dictionary payload generated by pyasn1 -- hence the question:
+    # Is there a standard JSON representation of a condition?
+    def to_asn1_json(self):


This could reuse to_asn1_dict. The value is the payload produced by to_asn1_dict

addressed in 0359c52

note: broke the thing and fixed it now

r-marques · 2017-06-21T12:15:55Z

cryptoconditions/schemas/rsa.py

@@ -0,0 +1 @@
+"""ASN.1 definition for an RSA public key object."""


Do we need this file?

No, removed it. The JS implementation had one.

r-marques · 2017-06-21T12:18:19Z

cryptoconditions/type_registry.py

@@ -23,13 +26,49 @@ def get_class_from_type_id(type_id):
        """
        # Determine type of condition
        if type_id > MAX_SAFE_INTEGER_JS:


This check here seems unnecessary

Yeah, I don't know, this came from the JS implementation.

r-marques · 2017-06-21T12:22:37Z

cryptoconditions/types/base_sha256.py



-class BaseSha256Fulfillment(Fulfillment):
+class BaseSha256(Fulfillment):


Why do we need this class? It seems that this method could just be added to the Fulfillment class

Hmmm, good point. I think the idea is that the Fulfillment is "SHA" agnostic. This way, one can imagine in the near future a new suite of types based of SHA-512 for instance, that would still use the class Fulfillment as a parent class.

As a side note: These things could be achieved via mixins probably.

r-marques · 2017-06-21T12:26:10Z

cryptoconditions/types/ed25519.py

+        self._signature = signature
+
+    # TODO check type or use static typing (mypy)
+    def _validate_public_key(self, public_key):


Should we check isinstance(public_key, bytes) the same way we do with the signature.

r-marques · 2017-06-21T12:29:20Z

cryptoconditions/types/ed25519.py

+            raise Exception(
+                'Public key must be {} bytes, was: {}'.format(
+                    self.PUBLIC_KEY_LENGTH, len(public_key)))
+        # TODO More validation? Ask ILP folks.


The underlying crypto library should already do extra checks. In fact this could just call VerifyKey and see if an exception is thrown.

r-marques · 2017-06-21T12:35:17Z

cryptoconditions/types/ed25519.py

+
+    def parse_asn1_dict_payload(self, data):
+        self.public_key = data['publicKey']
+        self.signature = data['signature']

    def validate(self, message=None, **kwargs):


I don't think there is a reason for message to be an optional argument. verify should fail if message is None. Also I don't see a reason for **kwargs

r-marques · 2017-06-21T12:43:24Z

cryptoconditions/types/preimage.py

+    def bitmask(self):
+        return self.FEATURE_BITMASK
+
+    #def write_hash_payload(self, hasher):


Can we remove all this commented code?

r-marques · 2017-06-21T12:45:08Z

cryptoconditions/types/preimage.py

+
+    @property
+    def bitmask(self):
+        return self.FEATURE_BITMASK


FEATURE_BITMASK is not declared anywhere.

r-marques · 2017-06-21T14:08:25Z

cryptoconditions/types/prefix.py

+    @prefix.setter
+    def prefix(self, prefix):
+        if not isinstance(prefix, bytes):
+            raise TypeError('Prefix must be a Buffer, was: {}'.format(prefix))


must be a byte string

r-marques · 2017-06-21T14:12:01Z

cryptoconditions/types/prefix.py

+        """
+        self._prefix = b''
+        self._subcondition = None
+        self.max_message_length = 16384


Should this be self._max_message_length?

Why 16384?

r-marques · 2017-06-21T14:12:36Z

cryptoconditions/types/prefix.py

+    TYPE_ASN1_FULFILLMENT = 'prefixSha256Fulfillment'
+    TYPE_CATEGORY = 'compound'
+
+    CONSTANT_BASE_COST = 16384


are these two constants used anywhere?

true, they are not used right now

r-marques · 2017-06-21T14:19:49Z

cryptoconditions/types/prefix.py

+            verify this fulfillment to begin with.
+
+        """
+        return {t for t in chain(self.subcondition.subtypes,


Will this work with arbitrarily deep subconditions?

if I am not wrong, the subcondition.subtypes should list all subtypes, regardless of the nesting level

r-marques · 2017-06-21T14:20:50Z

cryptoconditions/types/prefix.py

+        cost = (len(self.prefix) +
+                self.max_message_length + subcondition_cost + 1024)
+        return cost
+        #(len(self.prefix) +


r-marques · 2017-06-21T15:21:20Z

Some next steps that can be done after this as been merged.

I don't think we should expose methods like to/from asn1_dict/asn1_json since they are on the spec and are specific to different asn1 implementations.
We should only expose binary, uri and json. Ideally we would have a json schema that is part of the cryptoconditions spec.

We should cleanup conftest.py. There are a lot of constants and fixtures that are no longer being used.

The crypto module does not seem to be used by cryptoconditions anymore. BigchainDB probably still uses it. Ideally we would move the code we need into BigchainDB and remove the crypto module from cryptoconditions.

- Fixed a bug in fulfillment_to_condition

- Added a custom exception for validate_fulfillment - Fixed validate_fulfillment test

sbellem requested review from r-marques and diminator June 20, 2017 09:18

sbellem force-pushed the draft-02-impl branch from c73e5bd to 8e6a435 Compare June 20, 2017 13:35

r-marques reviewed Jun 20, 2017

View reviewed changes

sbellem force-pushed the draft-02-impl branch from e94189a to df23503 Compare June 20, 2017 17:01