Use correct type for parameters in ExtendedBitcoinClient

[araspitzu]

Replace the usage of String with the more precise ByteVector32

[pm47]

You missed those ones:

eclair/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWallet.scala

Line 73 in 61efb22

def isTransactionOutputSpendable(txId: String, outputIndex: Int, includeMempool: Boolean)(implicit ec: ExecutionContext): Future[Boolean] = rpcClient.invoke("gettxout", txId, outputIndex, includeMempool) collect { case j => j != JNull }

eclair/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWallet.scala

Line 148 in 61efb22

case class Utxo(txid: String, vout: Long)

Also maybe take the opportunity to homogeneize between txId and txid? I think the latter is what bitcoin-lib uses.

[araspitzu]

Didn't miss them but they're out of scope for the PR. In fact i think we shouldn't just fix them in BitcoinCoreWallet but we should use ExtendedBitcoinClient , if you notice isTransactionOutputSpendable and it's implementation are duplicated between wallet and client. What do you think of using the ExtendedBitcoinClient inside BitcoinCoreWallet?

[pm47]

I think the problem is that ExtendedBitcoinClient is bulky and I didn't want to make the small BitcoinCoreWallet depend on it.

How about making ExtendedBitcoinClient use the pimp pattern to extend BitcoinJsonRPCClient, the same way we made ExtendedResultSet extend ResultSet in order to do rs.getByteVector with the database? We could then move all the bitcoind-related functions of BitcoinCoreWallet to ExtendedBitcoinClient and that would address your concern.

[pm47]

Wait I'm changing my mind. I don't think pimp pattern is a good idea because we are not extending BitcoinJsonRPCClient with other functions similar to the ones it already has, we are adding a set of functions that are much more higher level than what it currently does.

[pm47]

What do you think of using the ExtendedBitcoinClient inside BitcoinCoreWallet?

Yes let's do this. I think maybe as a next step we could consider making ExtendedBitcoinClient an object.

[araspitzu]

Wait I'm changing my mind. I don't think pimp pattern is a good idea because we are not extending BitcoinJsonRPCClient with other functions similar to the ones it already has, we are adding a set of functions that are much more higher level than what it currently does.

👍 I was also hesitant with making ExtendedBitcoinClient use the pimp pattern to extend BitcoinJsonRPCClient, it feels more natural that one uses the other but without extending it. Maybe we should take some time to clean up ExtendedBitcoinClient to be able to use it from BitcoinCoreWallet, i would like to remove the code duplication that we currently have. I'm in favour of doing the necessary cleansing here in this PR.

[codecov-io]

Codecov Report

Merging #1248 into master will increase coverage by 0.06%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1248      +/-   ##
==========================================
+ Coverage   86.35%   86.41%   +0.06%     
==========================================
  Files         119      119              
  Lines        9253     9251       -2     
  Branches      398      375      -23     
==========================================
+ Hits         7990     7994       +4     
+ Misses       1263     1257       -6     

Impacted Files	Coverage Δ
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala	88.88% <83.33%> (-0.40%)	⬇️
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala	92.30% <83.33%> (ø)
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	96.26% <87.50%> (ø)
...chain/bitcoind/rpc/BasicBitcoinJsonRPCClient.scala	100.00% <100.00%> (ø)
...q/eclair/blockchain/electrum/ElectrumWatcher.scala	55.20% <0.00%> (+1.60%)	⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala	82.79% <0.00%> (+4.30%)	⬆️

[araspitzu]

Latest commit 3410f46 is a small improvement in the BitcoinCoreWallet to remove duplicate code. I didn't migrate the ExtendedBitcoinClient to be an object because it makes it harder to test and breaks composability.

[t-bast]

Is there something blocking this PR (apart from a rebase)?
It looks like a refactoring we may want to include before the next release.

[araspitzu]

Rebased on master

[araspitzu]

For reviewers: the commit history of this PR is not great let me know if you want a squash/rebase.

[t-bast]

let me know if you want a squash/rebase

Yes let's start fresh with a rebase (maybe once cltv-tie-breaker is merged since there will be conflicts).

[araspitzu]

Rebased on master and squash/merged the commit history of this PR.

[araspitzu]

In 380e4b3 i addressed some of your comments and i cleaned up BitcoinCoreWallet of some unused functions that were just an alias for ExtendedBitcoinClient.

[t-bast]

LGTM, let's not dive into the rabbit hole of refactoring all our bitcoin-cli calls now :)
I'm not ack-ing now, I think this should be merged right after we cut the release for eclair 0.3.4.