Integrate with External Wallets / Ledger

yosriady commented 4 years ago

Problem

We want to support external wallets, but the current SDK doesn't provide support for it. To add external wallet support, we will likely need to update the SDK ComitClient to support providers. However, how this looks is not yet 'shaped up'.

Goal

Provide a document in coblox/spikes repo that summarises the approaches used in the blockchain ecosystem to support a variety of wallets for Ethereum and Bitcoin.

The document is structured as a Shape Up pitch: https://basecamp.com/shapeup/1.5-chapter-06

TODOs

[ ] Define current wallet interface and what functions the SDK uses
[ ] Research and summarize external wallet support / the Provider API in other projects
- Liquality
- Metamask
- Ethers providers
- 0x subproviders
- Ledger providers
[ ] Research what the Provider actually does and how the SDK can consume it
- https://d2uvd02r4antif.cloudfront.net/docs/guides/web3-provider-explained#introduction
[ ] Recommendations for SDK
- How the two implementations differ
- What needs to be implemented on the SDK

Links

https://github.com/MetaMask/web3-provider-engine https://github.com/Neufund/ledger-wallet-provider https://github.com/liquality/chainabstractionlayer https://github.com/blockmason/web3-provider-ledger https://0x.org/docs/tools/subproviders

yosriady commented 4 years ago

Ethereum wallet provider libraries uses hooks + context to support different wallet providers in a modular way

https://github.com/NoahZinsmeister/web3-react https://github.com/aragon/use-wallet

yosriady commented 4 years ago

The SDK would need to support a similar provider interface to https://github.com/liquality/chainabstractionlayer/tree/dev/packages/ethereum-ledger-provider https://github.com/liquality/liquality-swap/blob/dev/src/services/chainClient.js

yosriady commented 4 years ago

https://github.com/ethers-io/ethers.js/tree/master/providers https://github.com/ethers-io/ethers.js/tree/ethers-v5-beta/packages/hardware-wallets

yosriady commented 4 years ago

https://github.com/Web3Modal/web3modal

const provider = await web3Modal.connectTo("walletconnect");
const web = new Web3(provider);

https://github.com/NoahZinsmeister/web3-react/tree/v6/docs

yosriady commented 4 years ago

Alternative non-Provider API

https://github.com/defireio/defire-js/tree/master/src/account/apis
- https://github.com/defireio/defire-js/blob/master/src/account/apis/Web3jsAccount.ts

yosriady commented 4 years ago

https://docs.metamask.io/guide/ethereum-provider.html

bonomat commented 4 years ago

I would go for Ledger's JS Library. Integrating with it looks relatively straight forward.

From their readme here https://github.com/LedgerHQ/ledgerjs#basic-gist:

import Transport from "@ledgerhq/hw-transport-node-hid";
// import Transport from "@ledgerhq/hw-transport-web-usb";
// import Transport from "@ledgerhq/react-native-hw-transport-ble";
import AppBtc from "@ledgerhq/hw-app-btc";
const getBtcAddress = async () => {
  const transport = await Transport.create();
  const btc = new AppBtc(transport);
  const result = await btc.getWalletPublicKey("44'/0'/0'/0/0");
  return result.bitcoinAddress;
};
getBtcAddress().then(a => console.log(a));

// Edit // I looked a bit into this and it seems to be quite cumbersome :S Especially for Bitcoin you will need to manage the derivation paths ourselves: https://github.com/LedgerHQ/ledger-live-common/blob/master/docs/derivation.md

thomaseizinger commented 4 years ago

Especially for Bitcoin you will need to manage the derivation paths ourselves: LedgerHQ/ledger-live-common@master/docs/derivation.md

Not only that, you also need to figure out yourselves which UTXOs you want to spend and keep track of them. That is what LedgerLive is doing for you with their server APIs. Running an Electrum server yourself instead of a bitcoind backend could be a good fit here as it allows you to query those things on-demand. Electrum's API would also work as a COMIT backend alternatively to btsieve and bitcoind.

yosriady commented 4 years ago

What we want is probably a pluggable system of providers like what https://github.com/liquality/chainabstractionlayer has done.

For example, their Bitcoin ledger provider implements:

signMessage()
sign*Transaction()
and a few other methods

Whereas the Comit SDK's BitcoindWallet implements:

broadcastTransaction()
and a few other methods

To support Ledger, the SDK 'wallets' would need to take in a Bitcoin ledger provider (or other providers) which implements the functions it needs for the ledger, and call those methods in broadcastTransaction() and other methods used by the swap protocol.

At least I think this is how we can make the SDK support external wallets.

thomaseizinger commented 4 years ago

Is there really so much duplication between wallet implementations that we need to abstract over that through a provider-like concept?

I think the trickier part is to actually make the whole system work.

The problem with Ledger is that we need to provide UTXOs in order to create valid transactions for funding. To do that, we would need to more or less run arbitrary queries against the UTXO database of bitcoind and that is something that the node is not really optimized for. We would have to register the account stored on the ledger as "watch-only" account in bitcoind and have it scan the blockchain before we could use that.

Electrum-server on the other hand supports these kind of queries fairly well on-demand if I am not mistaken.

At least for the foreseeable future, we can assume that cnd is going to be running on the same machine as an application built on top of the SDK. Hence, it makes sense to reuse the same "backend" for both services. At the moment, this backend is bitcoind which fulfills the following needs:

providing a wallet (used via the RPC interface from the SDK)
providing blocks (used by cnd to watch the blockchain)

With Electrum-server, I believe we can also query arbitrary blocks and hence we can satisfy cnd's needs for this backend. With the arbitrary query for addresses, we should be able to fulfill the needs to interact with Ledger.

yosriady commented 4 years ago

For ledger-live-common, looks like there's an example here: https://github.com/LedgerHQ/ledger-live-common/blob/master/docs/gist-tx.md

yosriady commented 4 years ago

@thomaseizinger

Is there really so much duplication between wallet implementations that we need to abstract over that through a provider-like concept?

I was thinking we need this for the SDK to support Ledger because the ComitClientright now only accepts the SDK wallets: https://github.com/comit-network/comit-js-sdk/blob/dev/src/comit_client.ts#L20-L43

thomaseizinger commented 4 years ago

@thomaseizinger

Is there really so much duplication between wallet implementations that we need to abstract over that through a provider-like concept?

I was thinking we need this for the SDK to support Ledger because the ComitClientright now only accepts the SDK wallets: https://github.com/comit-network/comit-js-sdk/blob/dev/src/comit_client.ts#L20-L43

@yosriady So the ComitClient actually just needs a very limited subset which could be hidden behind an interface that every wallet has to implement. I thought actually that we had already done that :thinking:

EDIT: It seems we did it for Bitcoin but not for Ethereum.

For ledger-live-common, looks like there's an example here: LedgerHQ/ledger-live-common@master/docs/gist-tx.md

This is interesting! It seems like the UTXO scanning is abstracted away here, cool :)

cc @luckysori @bonomat

bonomat commented 4 years ago

@yosriady

For ledger-live-common, looks like there's an example here: https://github.com/LedgerHQ/ledger-live-common/blob/master/docs/gist-tx.md

Cool, looks easier than I thought :D

@thomaseizinger

@yosriady So the ComitClient actually just needs a very limited subset which could be hidden behind an interface that every wallet has to implement. I thought actually that we had already done that 🤔

EDIT: It seems we did it for Bitcoin but not for Ethereum.

Yes, we never needed it for Ethereum. As far as I remember I initially extracted the BitcoinWallet interface so that I could build a testnet wallet :) mmhmm... bcoin... good memories 😅

bonomat commented 4 years ago

wups

yosriady commented 4 years ago

@yosriady So the ComitClient actually just needs a very limited subset which could be hidden behind an interface that every wallet has to implement. I thought actually that we had already done that 🤔

EDIT: It seems we did it for Bitcoin but not for Ethereum.

Yes, and we'll need to implement a 'provider' or 'adapter' for the Ledger (implements the SDK's BitcoinWallet and EthereumWallet) based on the subset of functions that the swap protocol needs so we can use it in the SDK.

bonomat commented 4 years ago

@da-kami / @yosriady :

I gave LedgerJS a try on the weekend. Using the library is not that hard and it works pretty much out of the box. Testnet/Ropsten support was also straight forward, I wasn't able to use a custom network (e.g. regtest on Bitcoin) with the current setup. This shouldn't be a problem for Ethereum (we could follow this example here): but for Bitcoin it might be a bit more complicated.

https://github.com/bonomat/ledgerjs-example

da-kami commented 4 years ago

ledger integration done already, outdated.

comit-network / ambrosia