interledger-deprecated / java-ilp-core

WE'VE MOVED: This project has moved to Hyperledger Quilt
https://github.com/hyperledger/quilt/tree/master/ilp-core
Apache License 2.0
16 stars 10 forks source link

PROPOSAL: Remove org.interledger.Condition/Fulfilment and reintroduce java-crypto-conditions dependency. #101

Closed sappenin closed 6 years ago

sappenin commented 6 years ago

I propose that we remove org.interledger.Condition/Fulfilment and reintroduce the java-crypto-conditions dependency.

The Condition and Fulfillment in the java-ilp-core class works great for ILP Universal mode, but causes confusion and conversion issues in ILP Atomic-mode scenarios. In atomic-mode, the local-ledger transfers use threshold conditions (among other things) to wrap Preimage conditions used in the ILP packet. The problem is, the two types are different between org.interledger.Fulfillment and org.interledger.cryptoconditions.Fulfillment, so validation is tricky because conversion needs to occur between the ILP Condition/Fulfillment and the crypto-condtions Condition/Fulfillment.

This problem becomes worse when trying to create a java variant of the Ledger Plugin Interface, because in order to support both ILP modes, the java-crypto-conditions versions need to be used. This will create more headaches for Java Connectors that want to operate in either Universal or Atomic modes.

Some ideas to solve this issue:

  1. Option 1: Remove Condition/Fulfillment from java-ilp-core, and restore the dependency on java-crypto-condtions.
  2. Option 2: Create a secondary project related to java-crypto-conditions that only contains base-code related to the Condition and Fulfillment. Then, both java-ilp-core and java-crypto-conditions could depend on this project, and implement their own flavor of PreimageSha256, but program interfaces to the main Condition/Fulfillment interfaces defined in the parent project.
  3. Option 3: Something else?

I kind of prefer Option1, because the original rationale for removing the crypto-conditions dependency has now gone away: There is now a published SNAPSHOT of the crypto-conditions library that can be trivially included in java-ilp-core.

That said, I'm open to other ideas.

adrianhopebailie commented 6 years ago

I'm supportive of this but suggest we consider merging the code or splitting out the codecs from core or something.

What if we think of the current Condition and Fulfillment in ilp-core as an alternative encoding of a subset of crypto-conditions? i.e. Always PREIMAGE-SHA-256, always cost of 32 and encoded in OER as just the hash and preimage.

If we do that we could have a standard condition interface that could be compliant with crypto-conditions and then 3 encodings:

  1. Crypto-Conditions Binary encoding as defined in the crypto-conditions spec
  2. Crypto-Conditions URI encoding as defined in the crypto-conditions spec
  3. Interledger Universal mode encoding as currently implemented in ilp-core and defined in the ILP RFCs

This aligns with earlier discussions we have had about decoupling the encoding from the objects.

My proposal is along the lines of option 1:

  1. Re-add crypto-conditions as a dependency of ilp-core
  2. Deprecate/delete org.interledger.Condition and org.interledger.Fulfillment
  3. Update the existing codecs in ilp-core to marshal org.interledger.cryptoconditions.Condition and org.interledger.cryptoconditions.Fulfillment instead but encode/decode them as required for universal mode ILP.
sappenin commented 6 years ago

If we do that we could have a standard condition interface that could be compliant with crypto-conditions and then 3 encodings...this aligns with earlier discussions we have had about decoupling the encoding from the objects.

I added a comment in #102, and I think my counter-proposal satisfies the encoding requirement (encoded in OER as just the hash and preimage) as well as the other 2 requirements (Always PREIMAGE-SHA-256, always cost of 32).

What do you think?

sappenin commented 6 years ago

Fixed by #102