Modify estimatefee endpoint to return a number rather than a string (#3973)

* Modify estimatefee endpoint to return a number rather than a string

* Fix compile

* Fix infinite loop on converting to sats/vb, add tests

app/server-test/src/test/scala/org/bitcoins/server/WalletRoutesSpec.scala

@@ -0,0 +1,41 @@
+package org.bitcoins.server
+
+import akka.http.scaladsl.testkit.ScalatestRouteTest
+import org.bitcoins.server.routes.ServerCommand
+import org.bitcoins.testkit.BitcoinSTestAppConfig
+import org.bitcoins.wallet.MockWalletApi
+import org.scalamock.scalatest.MockFactory
+import org.scalatest.wordspec.AnyWordSpec
+import akka.http.scaladsl.model.ContentTypes._
+import org.bitcoins.core.wallet.fee.{FeeUnit, SatoshisPerVirtualByte}
+
+import scala.concurrent.Future
+
+class WalletRoutesSpec
+    extends AnyWordSpec
+    with ScalatestRouteTest
+    with MockFactory {
+
+  implicit val conf: BitcoinSAppConfig =
+    BitcoinSTestAppConfig.getSpvTestConfig()
+  val mockWalletApi = mock[MockWalletApi]
+
+  val walletRoutes: WalletRoutes =
+    WalletRoutes(mockWalletApi)(system, conf.walletConf)
+  "WalletRoutes" should {
+    "estimatefee" in {
+
+      (mockWalletApi.getFeeRate: () => Future[FeeUnit])
+        .expects()
+        .returning(Future.successful(SatoshisPerVirtualByte.one))
+      val route =
+        walletRoutes.handleCommand(ServerCommand("estimatefee", ujson.Arr()))
+
+      Get() ~> route ~> check {
+        assert(contentType == `application/json`)
+        assert(responseAs[String] == s"""{"result":1,"error":null}""")
+      }
+    }
+  }
+
+}

app/server/src/main/scala/org/bitcoins/server/WalletRoutes.scala

@@ -810,8 +810,8 @@ case class WalletRoutes(wallet: AnyDLCHDWalletApi)(implicit
 
     case ServerCommand("estimatefee", _) =>
       complete {
-        wallet.getFeeRate.map { fee =>
+        wallet.getFeeRate().map { fee =>
-          Server.httpSuccess(fee.toString)
+          Server.httpSuccess(fee.toSatsPerVByte)
         }
       }
 

bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/common/BitcoindRpcClient.scala

@@ -83,7 +83,7 @@ class BitcoindRpcClient(override val instance: BitcoindInstance)(implicit
 
   // Fee Rate Provider
 
-  override def getFeeRate: Future[FeeUnit] =
+  override def getFeeRate(): Future[FeeUnit] =
     estimateSmartFee(blocks = 6).flatMap { result =>
       result.feerate match {
         case Some(feeRate) => Future.successful(feeRate)

core-test/src/test/scala/org/bitcoins/core/wallet/fee/FeeUnitTest.scala

@@ -65,10 +65,25 @@ class FeeUnitTest extends BitcoinSUnitTest {
     assert(satPerKb.toSatPerByte == SatoshisPerByte(Satoshis(3)))
   }
 
-  it must "correctly convert SatoshisPerVirtualByte to SatoshisPerKW" in {
+  it must "correctly convert SatoshisPerKiloByte to SatoshisPerVirtualByte and vice versa" in {
-    val satPerVb = SatoshisPerVirtualByte(Satoshis(3))
+    val satPerKb = SatoshisPerKiloByte(Satoshis(3000))
+    val expectedSatsPerVByte = SatoshisPerVirtualByte(Satoshis(3))
 
-    assert(satPerVb.toSatoshisPerKW == SatoshisPerKW(Satoshis(750)))
+    assert(satPerKb.toSatsPerVByte == expectedSatsPerVByte)
+  }
+
+  it must "correctly convert SatoshisPerVirtualByte to SatoshisPerKW and vice versa" in {
+    val satPerVb = SatoshisPerVirtualByte(Satoshis(3))
+    val expectedSatsPerKW = SatoshisPerKW(Satoshis(750))
+    assert(satPerVb.toSatoshisPerKW == expectedSatsPerKW)
+
+    assert(expectedSatsPerKW.toSatsPerVByte == satPerVb)
+  }
+
+  it must "correctly convert SatoshisPerByte to SatoshisPerVirtualByte and vice versa" in {
+    val satsPerVb = SatoshisPerVirtualByte(Satoshis(100))
+    val expectedSatsPerByte = SatoshisPerByte(Satoshis(100))
+    assert(expectedSatsPerByte.toSatsPerVByte == satsPerVb)
   }
 
   it must "calculate the same fee when using SatoshisPerVirtualByte.toSatoshisPerKW" in {

core/src/main/scala/org/bitcoins/core/api/feeprovider/FeeRateApi.scala

@@ -6,6 +6,6 @@ import scala.concurrent.Future
 
 trait FeeRateApi {
 
-  def getFeeRate: Future[FeeUnit]
+  def getFeeRate(): Future[FeeUnit]
 
 }

core/src/main/scala/org/bitcoins/core/api/wallet/WalletApi.scala

@@ -40,7 +40,7 @@ trait WalletApi extends StartStopAsync[WalletApi] {
   def broadcastTransaction(transaction: Transaction): Future[Unit] =
     nodeApi.broadcastTransaction(transaction)
 
-  def getFeeRate: Future[FeeUnit] = feeRateApi.getFeeRate
+  def getFeeRate(): Future[FeeUnit] = feeRateApi.getFeeRate()
 
   def start(): Future[WalletApi]
 
@@ -239,7 +239,7 @@ trait WalletApi extends StartStopAsync[WalletApi] {
   protected def determineFeeRate(feeRateOpt: Option[FeeUnit]): Future[FeeUnit] =
     feeRateOpt match {
       case None =>
-        feeRateApi.getFeeRate
+        feeRateApi.getFeeRate()
       case Some(feeRate) =>
         Future.successful(feeRate)
     }

core/src/main/scala/org/bitcoins/core/wallet/fee/FeeUnit.scala

@@ -35,6 +35,9 @@ sealed abstract class FeeUnit {
   def toLong: Long = currencyUnit.satoshis.toLong
 
   override def toString: String = s"$toLong ${factory.unitString}"
+
+  /** Converts the current fee unit to sats/vybte */
+  def toSatsPerVByte: SatoshisPerVirtualByte
 }
 
 trait FeeUnitFactory[+T <: FeeUnit] {
@@ -84,6 +87,9 @@ case class SatoshisPerByte(currencyUnit: CurrencyUnit) extends BitcoinFeeUnit {
   }
 
   override def factory: FeeUnitFactory[SatoshisPerByte] = SatoshisPerByte
+
+  override val toSatsPerVByte: SatoshisPerVirtualByte = SatoshisPerVirtualByte(
+    currencyUnit)
 }
 
 object SatoshisPerByte extends FeeUnitFactory[SatoshisPerByte] {
@@ -132,6 +138,10 @@ case class SatoshisPerKiloByte(currencyUnit: CurrencyUnit)
 
   lazy val toSatPerByte: SatoshisPerByte = toSatPerByteExact
 
+  /** Converts sats/kb -> sats/vbyte with rounding if necessary. */
+  override val toSatsPerVByte: SatoshisPerVirtualByte =
+    toSatPerByteRounded.toSatsPerVByte
+
   override def factory: FeeUnitFactory[SatoshisPerKiloByte] =
     SatoshisPerKiloByte
 
@@ -172,6 +182,9 @@ case class SatoshisPerVirtualByte(currencyUnit: CurrencyUnit)
   override def toString: String = s"$toLong sats/vbyte"
 
   lazy val toSatoshisPerKW: SatoshisPerKW = SatoshisPerKW(currencyUnit * 250)
+
+  override val toSatsPerVByte: SatoshisPerVirtualByte = this
+
 }
 
 object SatoshisPerVirtualByte extends FeeUnitFactory[SatoshisPerVirtualByte] {
@@ -210,6 +223,9 @@ case class SatoshisPerKW(currencyUnit: CurrencyUnit) extends BitcoinFeeUnit {
   override def factory: FeeUnitFactory[SatoshisPerKW] = SatoshisPerKW
 
   override def toString: String = s"$toLong sats/kw"
+
+  override val toSatsPerVByte: SatoshisPerVirtualByte = SatoshisPerVirtualByte(
+    currencyUnit / Satoshis(250))
 }
 
 object SatoshisPerKW extends FeeUnitFactory[SatoshisPerKW] {

fee-provider-test/src/test/scala/org/bitcoins/feeprovider/FeeRateProviderTest.scala

@@ -81,9 +81,9 @@ class FeeRateProviderTest extends BitcoinSAsyncTest {
   it must "get a cached fee rate from a cachedHttpFeeRateProvider" in {
     val provider = MempoolSpaceProvider(FastestFeeTarget, MainNet, proxyParams)
     for {
-      feeRate <- provider.getFeeRate
+      feeRate <- provider.getFeeRate()
       _ <- AsyncUtil.nonBlockingSleep(20.seconds)
-      cached <- provider.getFeeRate
+      cached <- provider.getFeeRate()
     } yield assert(feeRate == cached)
   }
 
@@ -94,13 +94,13 @@ class FeeRateProviderTest extends BitcoinSAsyncTest {
 
   it must "get the correct fee rate from a ConstantFeeRateProvider" in {
     val provider = ConstantFeeRateProvider(SatoshisPerByte(Satoshis(4)))
-    provider.getFeeRate.map { feeRate =>
+    provider.getFeeRate().map { feeRate =>
       assert(feeRate == SatoshisPerByte(Satoshis(4)))
     }
   }
 
   private def testProvider(provider: FeeRateApi): Future[Assertion] = {
-    provider.getFeeRate.map { feeRate =>
+    provider.getFeeRate().map { feeRate =>
       assert(feeRate.toLong > 0)
     }
   }

fee-provider/src/main/scala/org/bitcoins/feeprovider/ConstantFeeRateProvider.scala

@@ -6,5 +6,5 @@ import org.bitcoins.core.wallet.fee.FeeUnit
 import scala.concurrent.Future
 
 case class ConstantFeeRateProvider(feeUnit: FeeUnit) extends FeeRateApi {
-  def getFeeRate: Future[FeeUnit] = Future.successful(feeUnit)
+  def getFeeRate(): Future[FeeUnit] = Future.successful(feeUnit)
 }

fee-provider/src/main/scala/org/bitcoins/feeprovider/HttpFeeRateProvider.scala

@@ -38,7 +38,7 @@ abstract class HttpFeeRateProvider[T <: FeeUnit] extends FeeRateApi {
 
   protected def proxyParams: Option[Socks5ProxyParams]
 
-  def getFeeRate: Future[T] = {
+  override def getFeeRate(): Future[T] = {
     HttpFeeRateProvider
       .makeApiCall(uri, proxyParams)
       .flatMap(ret => Future.fromTry(converter(ret)))(system.dispatcher)
@@ -54,13 +54,13 @@ abstract class CachedHttpFeeRateProvider[T <: FeeUnit]
 
   private def updateFeeRate(): Future[T] = {
     implicit val ec: ExecutionContextExecutor = system.dispatcher
-    super.getFeeRate.map { feeRate =>
+    super.getFeeRate().map { feeRate =>
       cachedFeeRateOpt = Some((feeRate, TimeUtil.now))
       feeRate
     }
   }
 
-  override def getFeeRate: Future[T] = {
+  override def getFeeRate(): Future[T] = {
     cachedFeeRateOpt match {
       case None =>
         updateFeeRate()

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala

@@ -309,7 +309,7 @@ object BitcoinSWalletTest extends WalletLogger {
     // Useful for tests
     var lastFeeRate: Option[FeeUnit] = None
 
-    override def getFeeRate: Future[FeeUnit] = {
+    override def getFeeRate(): Future[FeeUnit] = {
       val feeRate = FeeUnitGen.feeUnit.sampleSome
 
       lastFeeRate = Some(feeRate)

wallet-test/src/test/scala/org/bitcoins/wallet/AddressTagIntegrationTest.scala

@@ -74,7 +74,7 @@ class AddressTagIntegrationTest extends BitcoinSWalletTest {
           .map(unconfirmed => assert(unconfirmed == valueFromBitcoind))
 
       account <- wallet.getDefaultAccount()
-      feeRate <- wallet.getFeeRate
+      feeRate <- wallet.getFeeRate()
       (txBuilder, utxoInfos) <- bitcoind.getNewAddress.flatMap { addr =>
         val output = TransactionOutput(valueToBitcoind, addr.scriptPubKey)
         wallet

wallet-test/src/test/scala/org/bitcoins/wallet/FundTransactionHandlingTest.scala

@@ -39,7 +39,7 @@ class FundTransactionHandlingTest
     fundedWallet: WalletWithBitcoind =>
       val wallet = fundedWallet.wallet
       for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations =
                                                 Vector(destination),
                                               feeRate = feeRate,
@@ -60,7 +60,7 @@ class FundTransactionHandlingTest
       val newDestination = destination.copy(value = amt)
       val wallet = fundedWallet.wallet
       for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations =
                                                 Vector(newDestination),
                                               feeRate = feeRate,
@@ -81,7 +81,7 @@ class FundTransactionHandlingTest
       val wallet = fundedWallet.wallet
 
       for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations = destinations,
                                               feeRate = feeRate,
                                               fromTagOpt = None,
@@ -106,7 +106,7 @@ class FundTransactionHandlingTest
       val wallet = fundedWallet.wallet
 
       val fundedTxF = for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations =
                                                 Vector(tooBigOutput),
                                               feeRate = feeRate,
@@ -127,7 +127,7 @@ class FundTransactionHandlingTest
       val wallet = fundedWallet.wallet
 
       val fundedTxF = for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations =
                                                 Vector(tooBigOutput),
                                               feeRate = feeRate,
@@ -150,7 +150,7 @@ class FundTransactionHandlingTest
       val account1 = WalletTestUtil.getHdAccount1(wallet.walletConfig)
       val account1DbF = wallet.accountDAO.findByAccount(account1)
       for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         account1DbOpt <- account1DbF
         fundedTx <- wallet.fundRawTransaction(Vector(newDestination),
                                               feeRate,
@@ -171,7 +171,7 @@ class FundTransactionHandlingTest
       val account1 = WalletTestUtil.getHdAccount1(wallet.walletConfig)
       val account1DbF = wallet.accountDAO.findByAccount(account1)
       val fundedTxF = for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         account1DbOpt <- account1DbF
         fundedTx <- wallet.fundRawTransaction(Vector(newDestination),
                                               feeRate,
@@ -188,7 +188,7 @@ class FundTransactionHandlingTest
       val wallet = fundedWallet.wallet
       val bitcoind = fundedWallet.bitcoind
       val fundedTxF = for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         _ <- wallet.createNewAccount(wallet.keyManager.kmParams)
         accounts <- wallet.accountDAO.findAll()
         account2 = accounts.find(_.hdAccount.index == 2).get
@@ -215,7 +215,7 @@ class FundTransactionHandlingTest
     fundedWallet: WalletWithBitcoind =>
       val wallet = fundedWallet.wallet
       for {
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         fundedTx <- wallet.fundRawTransaction(destinations =
                                                 Vector(destination),
                                               feeRate = feeRate,
@@ -235,7 +235,7 @@ class FundTransactionHandlingTest
       tag: AddressTag): Future[Assertion] = {
     for {
       account <- wallet.getDefaultAccount()
-      feeRate <- wallet.getFeeRate
+      feeRate <- wallet.getFeeRate()
       taggedAddr <- wallet.getNewAddress(Vector(tag))
       _ <-
         wallet.sendToAddress(taggedAddr, destination.value * 2, Some(feeRate))

wallet-test/src/test/scala/org/bitcoins/wallet/UTXOLifeCycleTest.scala

@@ -316,7 +316,7 @@ class UTXOLifeCycleTest extends BitcoinSWalletTestCachedBitcoindNewest {
 
     for {
       oldTransactions <- wallet.listTransactions()
-      feeRate <- wallet.getFeeRate
+      feeRate <- wallet.getFeeRate()
       tx <- wallet.fundRawTransaction(Vector(dummyOutput),
                                       feeRate,
                                       fromTagOpt = None,
@@ -342,7 +342,7 @@ class UTXOLifeCycleTest extends BitcoinSWalletTestCachedBitcoindNewest {
 
       for {
         oldTransactions <- wallet.listTransactions()
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         tx <- wallet.fundRawTransaction(Vector(dummyOutput),
                                         feeRate,
                                         fromTagOpt = None,
@@ -372,7 +372,7 @@ class UTXOLifeCycleTest extends BitcoinSWalletTestCachedBitcoindNewest {
 
       for {
         oldTransactions <- wallet.listTransactions()
-        feeRate <- wallet.getFeeRate
+        feeRate <- wallet.getFeeRate()
         tx <- wallet.fundRawTransaction(Vector(dummyOutput),
                                         feeRate,
                                         fromTagOpt = None,

wallet-test/src/test/scala/org/bitcoins/wallet/WalletIntegrationTest.scala

@@ -279,7 +279,7 @@ class WalletIntegrationTest extends BitcoinSWalletTestCachedBitcoindNewest {
       walletBal1 <- wallet.getBalance()
 
       // Create child tx
-      childFeeRate <- wallet.feeRateApi.getFeeRate
+      childFeeRate <- wallet.feeRateApi.getFeeRate()
       childTx <- wallet.bumpFeeCPFP(parentTx.txIdBE, childFeeRate)
       _ <- bitcoind.sendRawTransaction(childTx)
 

wallet-test/src/test/scala/org/bitcoins/wallet/WalletSendingTest.scala

@@ -383,7 +383,7 @@ class WalletSendingTest extends BitcoinSWalletTest {
     val wallet = fundedWallet.wallet
     for {
       parent <- wallet.sendToAddress(testAddress, amountToSend, None)
-      bumpRate <- wallet.feeRateApi.getFeeRate
+      bumpRate <- wallet.feeRateApi.getFeeRate()
       child <- wallet.bumpFeeCPFP(parent.txIdBE, bumpRate)
 
       received <- wallet.spendingInfoDAO.findTx(child).map(_.nonEmpty)
@@ -463,7 +463,7 @@ class WalletSendingTest extends BitcoinSWalletTest {
       algo: CoinSelectionAlgo): Future[Assertion] = {
     for {
       account <- wallet.getDefaultAccount()
-      feeRate <- wallet.getFeeRate
+      feeRate <- wallet.getFeeRate()
       allUtxos <- wallet.listUtxos(account.hdAccount)
       output = TransactionOutput(amountToSend, testAddress.scriptPubKey)
       expectedUtxos =