Problem

There is no indication whether blockchain-contracts crates (and hence comit-rs) uses the hex from the RFCs.

Goal

Make it easy and clear for a potential user/developer whether we do what we say and avoid discrepancies that could impact the trust in CoBloX.

Ensure that blockchain_contracts is aligned with the RFCs.

Recommendation

Come up with a process instead of solving it with technology.

Outdated recommendation

Any solutions that would impact the CI would deteriorate the devex (e.g. make CI fail if the hex used in the code is not the one from the RFC) as it would mean that both repos would need to be updated in a close timeframe.

Instead, we could provide an indicator on the blockchain_contract repo (or RFC repo?).

The indicator could be similar to the CI badges, where the badge goes read if the hex are not aligned with the RFC.

I imagine it could be possible with a mix of GitHub Pages and GitHub actions and extracting the hex in separate files in both repos.

Problem

Web3 and its transitive dependencies are unergonomic, bulky and overall pretty painful.
We need to get rid them.

Goal

Easy to consume and maintain code.

The way forward

Clarity provides a fairly rich and yet lightweight low-level Ethereum interface: https://github.com/althea-net/clarity

It gives us stuff like an Address type and transaction signing.

We only use a handful of RPC calls, those can easily be re-created with a small client that has exactly the interface we need.

Move the Ethereum HTLC to this repo.

DoD:

• Ethereum HTLCS are here
• test passes
• Ethereum HTLC generation is still done with docker/solc

Blocked by comit-network/comit-rs#626

Problem

We have a hand-rolled implementation for signing our Bitcoin HTLC.
This harms interoperability with other wallets and limits the usecases of COMIT.
This would also allow us to clean up some code (old bitcoin_witness crate).

Goal

Use miniscript because it is a standardized way for doing contracts on Bitcoin.
It will also allow us to untie the library component of COMIT from cnd.

Recommendation (copied from a previous comment)

In blockchain_contracts:

1. We would have to construct the HTLC from a miniscript instead of the current, serialized bytecode. A miniscript can be obtained by compiling a policy. Here is an example miniscript: c:or_i(and_v(v:sha256(0000000000000000000000000000000000000000000000000000000000000001),pk_h(A)),and_v(after(1000),pk_h(B)))
   When picking up this issue, it is probably best to start from scratch with a policy.
2. Change the BitcoinHtlc to not sign transactions anymore. Instead, we should return the miniscript that we used to create the BitcoinHtlc.
3. Separately, we should provide functionality that allows one to consume this miniscript and given the necessary data, signs a transaction for you.
   Most importantly though, this should be an optional feature so that users of blockchain-contracts are free to do the signing themselves or just pass the miniscript on to other clients.

In cnd:
We would have to re-work how SpendOutput is designed but basically, we just need to use the abstractions provided by blockchain-contracts. blockchain-contracts should provide everything so that we only need to put in:

• the secret
• the relevant keys
• the outpoint of the htlc
• the fee

Note: Make sure to use miniscript's fee estimation instead of hand rolling something!

Problem

We did quite some changes in blockchain contracts which are not yet used in comit-rs.

Goal

Release blockchain-contracts and use it in comit-rs

As per comit-network/spikes#30 (comment)

Problem

Merging PRs manually is cumbersome.

Goal

Make work as efficient as possible.

Recommendation

Setup bors (modify comit-rs').

Problem

The protocol naming spike introduces a split into protocols per ledger.

Goal

Adopt the new terminology in this crate. Do not mention rfc003 in this repo anymore.

Recommendation

The current folder structure already suggests that protocols are actually per ledger, we just need to rename some of the folders and move files around. There shouldn't be any functionality change necessary.

This involves reviewing/eliminating dependencies on the support crates.

Move issues

• https://github.com/comit-network/comit-rs/issues/1085
• https://github.com/comit-network/comit-rs/issues/1048

Copy issues

• comit-network/comit-rs#1195

Also open issues to contribute back to rust-bitcoin orga (see TODOs in code).

to this repository as this one is done.

Problem

Blockchain-contracts is a security critical part of COMIT. At the moment, we have to watch out of dependency updates manually, which we practically never do. Additionally, cnd depends on it, hence it is crucial that all the dependencies are on the latest version, otherwise we cannot update them in cnd (f.e. rust-bitcoin update comit-network/comit-rs#1493 is currently blocked by blockchain-contracts not using the up-to-date version of rust-bitcoin)

Goal

Keep dependencies up-to-date and make integration in cnd easy.

Recommendation

Setup dependabot and mergify the same way as in cnd.

Problem

As part of doing #4, a fair amount of duplication was created in the test-harness.

Goal

This crate contains code that is critical to COMIT. Not only the actual production code, but also the testcode should be easy to understand, verify and maintain.

Recommendation

• Cut down on the number of dependencies
• Only take trust-worthy dependencies

Major painpoints:

• ethereum_helper::ToEthereumAddress #24
• ethereum_helper::InMemoryWallet #24
• bitcoin_helper::RegtestHelperClient

Problem

@bonomat summarized some findings around the gas consumption of our ethereum related actions in #52 (deploy contract/fund/redeem/refund).
This should be documented in the code accordingly.

Goal

It should be clear from the code why certain gas limits have been chosen (e.g. code comments). These limits should also be ensured via tests.
See #52 (comment) for explanations

Recommendation

There was a draft PR #56 showing these limits and in #52 one can find a textual description.

Someone wanting to learn about gas cost and ethereum should grab this ticket and pair (e.g. with @bonomat ).

Replaces #52

Problem

Our make file defaults to stable if RUST_TOOLCHAIN is not defined.

We have two CI targets, one for stable and one for beta. However, both set the environment variable to beta:

Hence, we never build on stable :D

Problem

It is hard for a developer to understand why the HTLC does not behave as expected in case of failure.

Goal

Make it easier to understand why a call to the Ethereum HTLCs fail.

Agreed Recommendation

Use revert and on failure paths. See comit-network/spikes#37 for more details.

Tasks:

• Update Ether HTLC to revert with a reason instead of return. The transaction must be marked as Failed if secret is incorrect or attempt to refund is made and time is not yet expired.
• Update ERC20 HTLC to revert as above.
• Create RFC issue to update the 2 RFCs (HTLC Ether and ERC20)

With PR comit-network/comit-rs#1073, I noticed that our HTLCs currently have different APIs:

• BitcoinHtlc provides you methods for unlock_with_secret and unlock_after_timeout that return a single struct that contains all the data you need for creating the final transaction
• EtherHtlc and Erc20Htlc provide several accessor methods for this kind of data.

The variant chosen for BitcoinHtlc deemed to be very helpful down the line and helps with embedding the domain into the code: An HTLC can only be unlocked in one of two ways. Having two public methods represents that very clearly.

We should try and create a similar abstraction for EtherHtlc and Erc20Htlc.
Each HTLC should have three methods:

• how to deploy (for Bitcoin, compute_address, for Ethereum bytecode and gas_limit)
• how to unlock with secret
• how to unlock after timeout

Problem

It seems like both HTLCs for Ethereum (Ether and Erc20) do not print the keccak256 of the proposed messages:

Code claims:

However, I was not able to reproduce the same hash for neither of the 2:

Tested with web3js and http://emn178.github.io/online-tools/keccak_256.html

> var Web3 = require('web3');
> console.log(web3.utils.soliditySha3({ type: 'string', value: "Refunded"}))
0x51e3aa8099bfbb7b9fee513355876c379349ac1dca81cd9eb4e0653e784ff985
undefined
> console.log(web3.utils.soliditySha3({ type: 'string', value: "Refunded()"}))
0x8616bbbbad963e4e65b1366f1d75dfb63f9e9704bbbf91fb01bec70849906cf7

Proposal

• Regenerate it properly 😬. To simplify it, remove () which are meant to be included, i.e. keccak256("Refunded")
• Provide web3js snippet in code for reproducibility

Provide Htlc for Bitcoin (rfc003).

Blocked by comit-network/comit-rs#626

Problem

We provide gas limit recommendation for ERC20 and Ether contracts interactions.
These recommendations are coming from the actual gas usage during the tests run:

Except for

As you can see, the actual gas usage in comments is actually way lower than the recommendation. This is because when the recommendation is decreased from 100,000 to 50,000 then the Ethereum transaction fails due to out of gas.

We don't know why.

Goal

Be able to justify the gas limit recommendations and explain why we recommend 100,000 when actually the max usage is 30,000.

Recommendation

A. Get a transaction trace from dev network.

You can use https://github.com/comit-network/comit-rs/blob/master/api_tests/e2e/rfc003/eth_btc/ignore_fail_eth_transactions.ts

1. Put a very high gas limit value in fundLowGas (rename to fundWithInsufficientGas) and run the test with --keep-on-failure to have parity still running after test failure. The test will fail because fundLowGas checks that the transaction is failed. If the transaction is successful because there is enough gas, an exception is thrown making the test fail.
2. Check test logs to grab txid, get gas consumption from parity node
3. Readjust the gas limit to be just above the gas consumption, As per the assumptions of this issue, this should fail. Grab the trace, look at it, get @yosriady's help

B. Get transaction from testnet

If you did not reach any conclusion from A. Play around in testnet and try to reach a conclusion.

Note: If the conclusion that parity is not behaving like testnet, then need to question whether our recommendations are valid, whether they are testable, we should swap eth node, etc.
Note: Gas prices are mentioned in the comments, these are not in scope.