Feedback on SDK / documentation

[yosriady]

As development goes on:

• Go through https://comit.network/docs/getting-started/create-comit-app
• Identify gaps in documentation
• Raise open questions that can be documented

Collated feedback so far:

• The create-comit-app documentation do not explain anything about the API surface. https://comit.network/docs/getting-started/create-comit-app
• The documentation above just shows outputs of console commands. Not very useful.
• We are forcing developers go through the example repo themselves without any explanation about each step / function. This is poor developer experience. COMIT should have a tutorial: https://docs.renproject.io/developers/ren-sdk
• From a third-party developer's perspective, the protocol sections are not that useful. They only care about shipping something and needs to know how the SDK works and where they can 'plug in' their code. They do not care about HTLCs.
• There's no documentation about nodes / infrastructure. No recommendations of how to make production-ready COMIT apps on either testnet or mainnet. This leaves a lot up to the third-party developers.
• To address the above, perhaps we can have a section explaining the internals of comit-scripts, and what nodes / infrastructure the developer needs to execute a COMIT swap (e.g. what nodes, how many nodes, other recommendations.)
• The fact that developers have to think about nodes may be a bad abstraction.
• Why does the SDK even need InMemoryBitcoinWallet and EthereumWallet? Users should be able to just use bcoin or ethers.js directly.
• Why not call.InMemoryBitcoinWallet just BitcoinWallet?

--

• Is createActor idempotent and generate the same peerId every time it's called? If so, 'create' may not be the best name for it because create is usually not idempotent.
• Do we have a transaction monitoring solution? Developers would need to monitor each transaction to proceed with a swap. Possible by polling swap.fetchDetails.
• What's the difference between getNewSwaps and getOngoingSwaps and getDoneSwaps? We may want to make it clear for developers what the corresponding statuses mean.
  • Which category do expired swaps go under?
• What if instead of having three different functions, the SDK provides a single getSwaps({ status: ...}) that accept filtering arguments? By default, it can return all swaps regardless of status.
• What is a Swap and its properties?
  • What is peerId?
  • What does alpha and beta correspond to? Why not maker and taker?
• Why does swap.fetchDetails() return SwapDetails instead of SwapProperties directly? The SwapDetails interface seems to only contain a single properties attribute. In we can simplify this, perhaps swap.getProperties() would be a better name.
• Swaps may need a serialization function such as .toJSON() for ease of transport over HTTP.
• What's important in the documentation is to help developers create a 'model' on how to think about what they're doing and how it relates to what they're trying to achieve. For example, functions like redeem, refund etc means nothing to third-party developers unless we explain how it relates to what they're trying to do - which is to swap assets across chains.
• How is expiry handled? Is there an EXPIRED status?
• In building the app, I realized that developers would need to implement a state machine to keep track of the swap status and what to do next when polling. Would it make sense to have a similar functionality in the SDK?
• Is it possible to have the ETH deploy and fund steps be done in a single transaction? Nontrivial

[D4nte]

@yosriady ping me once you feel you have aggregated enough feedback so I can see how we can take action.

[yosriady]

@D4nte Noted.

I will continue to update this issue with more feedback. In the meantime, feel free to pick out a few items if any catches your eye.

[thomaseizinger]

Why does the SDK even need InMemoryBitcoinWallet and EthereumWallet? Users should be able to just use bcoin or ethers.js directly.

That is an interesting point. bcoin and ethers.js don't provide the exact APIs we need for executing a swap. The wallets we defined on top of them are supposed to be for the user's convenience. Would your feedback be addressed if we add additional constructors to just create one of these from an instance of a bcoin / ethers.js wallet?

[thomaseizinger]

Why not call.InMemoryBitcoinWallet just BitcoinWallet?

It stores the data necessary (UTXOs, block headers, etc) in-memory, meaning throwing away the instance and creating a new one will require a re-sync with the network.

An alternative implementation of the BitcoinWallet could store this information on disk or somewhere different, depending on the environment the SDK is running in (browser, etc).

[yosriady]

Why not call.InMemoryBitcoinWallet just BitcoinWallet?

It stores the data necessary (UTXOs, block headers, etc) in-memory, meaning throwing away the instance and creating a new one will require a re-sync with the network.

An alternative implementation of the BitcoinWallet could store this information on disk or somewhere different, depending on the environment the SDK is running in (browser, etc).

That makes sense, however there is so far only a single BitcoinWallet implementation. InMemoryBitcoinWallet feels like it's leaking too much information to users. Third-party developers would only see InMemoryBitcoinWallet as a bitcoin wallet. Since there is only a single EthereumWallet implementation as well, would it make sense to consolidate it into one BitcoinWallet?

[yosriady]

Why does the SDK even need InMemoryBitcoinWallet and EthereumWallet? Users should be able to just use bcoin or ethers.js directly.

That is an interesting point. bcoin and ethers.js don't provide the exact APIs we need for executing a swap. The wallets we defined on top of them are supposed to be for the user's convenience. Would your feedback be addressed if we add additional constructors to just create one of these from an instance of a bcoin / ethers.js wallet?

Yes, I think that makes sense. Perhaps in the case of the Ethereum wallet:

// Before
const wallet = new EthereumWallet(
    process.env.ETHEREUM_NODE_URL, // url
    process.env.ETHEREUM_KEY // private key
);

// After
const web3 = new Web3(...) // (or Ethers.js)
const wallet = new EthereumWallet(
  web3Provider: web3.currentProvider
);

In the Ethereum ecosystem, this style of web3 provider injection is fairly common.

[D4nte]

Why not call.InMemoryBitcoinWallet just BitcoinWallet?

It stores the data necessary (UTXOs, block headers, etc) in-memory, meaning throwing away the instance and creating a new one will require a re-sync with the network. An alternative implementation of the BitcoinWallet could store this information on disk or somewhere different, depending on the environment the SDK is running in (browser, etc).

That makes sense, however there is so far only a single BitcoinWallet implementation. InMemoryBitcoinWallet feels like it's leaking too much information to users. Third-party developers would only see InMemoryBitcoinWallet as a bitcoin wallet. Since there is only a single EthereumWallet implementation as well, would it make sense to consolidate it into one BitcoinWallet?

We expect people to use their own external bitcoin wallet (nano ledger, mobile app) and use this interface to create with such external wallet.

[da-kami]

Just summing up my thoughts here for creating tickets. We can hide this once the tickets are done :)

From what @yosriady currently wrote down I see three main categories for improvement:

1. Coding Tutorials: Detailed coding tutorials explaining the code in the create-comit-app example are needed.
2. Infrastructure Documentation / Tutorials: A section that explains the setup around the different "components" (i.e. programs) is needed. @rishflab also mentioned issues around the understandability of the "setup".
3. SDK improvements: Especially the way we integrated the wallets seems to cause confusion.

When it comes to creating tickets:

1. Coding Tutorials: We already have https://github.com/comit-network/comit.network/issues/4 and should put it into the next sprint. I would create follow up tickets after that one was done or during the process of doing it.

2. Infrastructure Documentation / Tutorials: We have to decide where we put that and how detailed it shall be described. I was planning to add more slides for that to the presentation master - we could re-use them in the documentation. @D4nte this needs some discussion.

3. SDK improvements: For me it is unclear if/how we should change the wallet integration to make it better. The proposal for the Ethereum wallet change (i.e. web3.provider) could be done easily, but I think the problem sits somewhere else. Maybe we have to add tutorials on how to plug different wallets to tackle this properly. @D4nte this needs discussion.

[D4nte]

• The create-comit-app documentation do not explain anything about the API surface. https://comit.network/docs/getting-started/create-comit-app
• The documentation above just shows outputs of console commands. Not very useful.
• We are forcing developers go through the example repo themselves without any explanation about each step / function. This is poor developer experience. COMIT should have a tutorial: https://docs.renproject.io/developers/ren-sdk

Create an app tutorial: https://github.com/comit-network/comit.network/issues/4

• From a third-party developer's perspective, the protocol sections are not that useful. They only care about shipping something and needs to know how the SDK works and where they can 'plug in' their code. They do not care about HTLCs.

Proposal: https://github.com/comit-network/comit.network/issues/41

• There's no documentation about nodes / infrastructure. No recommendations of how to make production-ready COMIT apps on either testnet or mainnet. This leaves a lot up to the third-party developers.
• To address the above, perhaps we can have a section explaining the internals of comit-scripts, and what nodes / infrastructure the developer needs to execute a COMIT swap (e.g. what nodes, how many nodes, other recommendations.)
• The fact that developers have to think about nodes may be a bad abstraction.

Proposal: https://github.com/comit-network/comit.network/issues/42

• Why does the SDK even need InMemoryBitcoinWallet and EthereumWallet? Users should be able to just use bcoin or ethers.js directly.
• Why not call.InMemoryBitcoinWallet just BitcoinWallet?

Proposal for Bitcoin: https://github.com/comit-network/comit-js-sdk/pull/153

Yes, I think that makes sense. Perhaps in the case of the Ethereum wallet:

// Before
const wallet = new EthereumWallet(
    process.env.ETHEREUM_NODE_URL, // url
    process.env.ETHEREUM_KEY // private key
);

// After
const web3 = new Web3(...) // (or Ethers.js)
const wallet = new EthereumWallet(
  web3Provider: web3.currentProvider
);

In the Ethereum ecosystem, this style of web3 provider injection is fairly common.

Proposal: https://github.com/comit-network/comit-js-sdk/issues/154

@yosriady I believe we have covered all points above with new issues, please review and comments in the tickets.

Let me know if I missed anything.

[yosriady]

Closing for staleness