Respond to code review on legacy address generation

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

@@ -10,26 +10,23 @@ import org.bitcoins.core.crypto.AesPassword
 import org.bitcoins.wallet.api.UnlockWalletError.MnemonicNotFound
 import com.typesafe.config.ConfigFactory
 import org.bitcoins.core.protocol.P2PKHAddress
+import org.bitcoins.core.hd.HDPurposes
 
 class LegacyWalletTest extends BitcoinSWalletTest {
 
   override type FixtureParam = UnlockedWalletApi
 
-  override def withFixture(test: OneArgAsyncTest): FutureOutcome = {
-    val confOverride =
-      ConfigFactory.parseString("bitcoin-s.wallet.defaultAccountType = legacy")
-    withNewConfiguredWallet(confOverride)(test)
-  }
+  override def withFixture(test: OneArgAsyncTest): FutureOutcome =
+    withLegacyWallet(test)
 
-  // eventually this test should NOT succeed, as BIP44
-  // requires a limit to addresses being generated when
-  // they haven't received any funds
   it should "generate legacy addresses" in { wallet: UnlockedWalletApi =>
     for {
       addr <- wallet.getNewAddress()
+      account <- wallet.getDefaultAccount()
       otherAddr <- wallet.getNewAddress()
       allAddrs <- wallet.listAddresses()
     } yield {
+      assert(account.hdAccount.purpose == HDPurposes.Legacy)
       assert(allAddrs.forall(_.address.isInstanceOf[P2PKHAddress]))
       assert(allAddrs.length == 2)
       assert(allAddrs.exists(_.address == addr))

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

@@ -11,27 +11,24 @@ import org.bitcoins.wallet.api.UnlockWalletError.MnemonicNotFound
 import com.typesafe.config.ConfigFactory
 import org.bitcoins.core.protocol.P2PKHAddress
 import org.bitcoins.core.protocol.Bech32Address
+import org.bitcoins.core.hd.HDPurposes
 
 class SegwitWalletTest extends BitcoinSWalletTest {
 
   override type FixtureParam = UnlockedWalletApi
 
   override def withFixture(test: OneArgAsyncTest): FutureOutcome = {
-    val confOverride =
-      ConfigFactory.parseString("bitcoin-s.wallet.defaultAccountType = segwit")
-    val conf = config.walletConf.withOverrides(confOverride)
-    withNewConfiguredWallet(confOverride)(test)
+    withSegwitWallet(test)
   }
 
-  // eventually this test should NOT succeed, as BIP44
-  // requires a limit to addresses being generated when
-  // they haven't received any funds
   it should "generate segwit addresses" in { wallet: UnlockedWalletApi =>
     for {
       addr <- wallet.getNewAddress()
+      account <- wallet.getDefaultAccount()
       otherAddr <- wallet.getNewAddress()
       allAddrs <- wallet.listAddresses()
     } yield {
+      assert(account.hdAccount.purpose == HDPurposes.SegWit)
       assert(allAddrs.forall(_.address.isInstanceOf[Bech32Address]))
       assert(allAddrs.length == 2)
       assert(allAddrs.exists(_.address == addr))

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

@@ -28,9 +28,6 @@ class WalletUnitTest extends BitcoinSWalletTest {
     }
   }
 
-  // eventually this test should NOT succeed, as BIP44
-  // requires a limit to addresses being generated when
-  // they haven't received any funds
   it should "generate addresses" in { wallet: UnlockedWalletApi =>
     for {
       addr <- wallet.getNewAddress()

wallet-test/src/test/scala/org/bitcoins/wallet/util/BitcoinSWalletTest.scala

@@ -24,6 +24,7 @@ import org.bitcoins.db.AppConfig
 import org.bitcoins.testkit.BitcoinSAppConfig
 import org.bitcoins.testkit.BitcoinSAppConfig._
 import com.typesafe.config.Config
+import com.typesafe.config.ConfigFactory
 
 trait BitcoinSWalletTest
     extends fixture.AsyncFlatSpec
@@ -58,33 +59,39 @@ trait BitcoinSWalletTest
       .map(_ => ())
   }
 
-  private val createNewWallet: Option[Config] => () => Future[
-    UnlockedWalletApi] =
-    extraConfig =>
-      () => {
-        val defaultConf = implicitly[WalletAppConfig]
-        val walletConfig = extraConfig match {
-          case None    => defaultConf
-          case Some(c) => defaultConf.withOverrides(c)
-        }
+  /** Returns a function that can be used to create a wallet fixture.
+    * If you pass in a configuration to this method that configuration
+    * is given to the wallet as user-provided overrides. You could for
+    * example use this to override the default data directory, network
+    * or account type.
+    */
+  private def createNewWallet(
+      extraConfig: Option[Config]): () => Future[UnlockedWalletApi] =
+    () => {
+      val defaultConf = config.walletConf
+      val walletConfig = extraConfig match {
+        case None    => defaultConf
+        case Some(c) => defaultConf.withOverrides(c)
+      }
 
-        AppConfig.throwIfDefaultDatadir(walletConfig)
+      // we want to check we're not overwriting
+      // any user data
+      AppConfig.throwIfDefaultDatadir(walletConfig)
 
-        for {
-          _ <- walletConfig.initialize()
-          wallet <- Wallet
-            .initialize()(implicitly[ExecutionContext], walletConfig)
-            .map {
-              case InitializeWalletSuccess(wallet) => wallet
-              case err: InitializeWalletError =>
-                logger.error(s"Could not initialize wallet: $err")
-                fail(err)
-            }
-        } yield wallet
+      walletConfig.initialize().flatMap { _ =>
+        Wallet
+          .initialize()(implicitly[ExecutionContext], walletConfig)
+          .map {
+            case InitializeWalletSuccess(wallet) => wallet
+            case err: InitializeWalletError =>
+              logger.error(s"Could not initialize wallet: $err")
+              fail(err)
+          }
+      }
     }
 
   /** Creates a wallet with the default configuration  */
-  def withDefaultWallet(): Future[UnlockedWalletApi] =
+  private def createDefaultWallet(): Future[UnlockedWalletApi] =
     createNewWallet(None)() // get the standard config
 
   /** Lets you customize the parameters for the created wallet */
@@ -93,8 +100,22 @@ trait BitcoinSWalletTest
       makeDependentFixture(build = createNewWallet(Some(walletConfig)),
                            destroy = destroyWallet)
 
+  /** Fixture for an initialized wallet which produce legacy addresses */
+  def withLegacyWallet(test: OneArgAsyncTest): FutureOutcome = {
+    val confOverride =
+      ConfigFactory.parseString("bitcoin-s.wallet.defaultAccountType = legacy")
+    withNewConfiguredWallet(confOverride)(test)
+  }
+
+  /** Fixture for an initialized wallet which produce segwit addresses */
+  def withSegwitWallet(test: OneArgAsyncTest): FutureOutcome = {
+    val confOverride =
+      ConfigFactory.parseString("bitcoin-s.wallet.defaultAccountType = segwit")
+    withNewConfiguredWallet(confOverride)(test)
+  }
+
   def withNewWallet(test: OneArgAsyncTest): FutureOutcome =
-    makeDependentFixture(build = withDefaultWallet, destroy = destroyWallet)(
+    makeDependentFixture(build = createDefaultWallet, destroy = destroyWallet)(
       test)
 
   case class WalletWithBitcoind(
@@ -120,7 +141,7 @@ trait BitcoinSWalletTest
 
   def withNewWalletAndBitcoind(test: OneArgAsyncTest): FutureOutcome = {
     val builder: () => Future[WalletWithBitcoind] = composeBuildersAndWrap(
-      withDefaultWallet,
+      createDefaultWallet,
       createWalletWithBitcoind,
       (_: UnlockedWalletApi, walletWithBitcoind: WalletWithBitcoind) =>
         walletWithBitcoind

wallet/src/main/scala/org/bitcoins/wallet/Wallet.scala

@@ -181,9 +181,16 @@ object Wallet extends CreateWalletApi with BitcoinSLogger {
             val createAccountFutures =
               HDPurposes.all.map(createRootAccount(wallet, _))
 
-            Future
-              .sequence(createAccountFutures)
-              .map(_ => logger.debug(s"Created root level accounts for wallet"))
+            val accountCreationF = Future.sequence(createAccountFutures)
+
+            accountCreationF.foreach(_ =>
+              logger.debug(s"Created root level accounts for wallet"))
+
+            accountCreationF.failed.foreach { err =>
+              logger.error(s"Failed to create root level accounts: $err")
+            }
+
+            accountCreationF
           }
 
         } yield wallet

wallet/src/main/scala/org/bitcoins/wallet/models/UTXOSpendingInfoTable.scala

@@ -19,7 +19,11 @@ import org.bitcoins.core.crypto.BIP39Seed
 import org.bitcoins.core.util.BitcoinSLogger
 import org.bitcoins.core.hd.LegacyHDPath
 
-case class SegWitUTOXSpendingInfoDb(
+/**
+  * DB representation of a native V0
+  * SegWit UTXO
+  */
+case class NativeV0UTXOSpendingInfoDb(
     id: Option[Long],
     outPoint: TransactionOutPoint,
     output: TransactionOutput,
@@ -31,7 +35,7 @@ case class SegWitUTOXSpendingInfoDb(
 
   override type PathType = SegWitHDPath
 
-  override def copyWithId(id: Long): SegWitUTOXSpendingInfoDb =
+  override def copyWithId(id: Long): NativeV0UTXOSpendingInfoDb =
     copy(id = Some(id))
 }
 
@@ -51,6 +55,14 @@ case class LegacyUTXOSpendingInfoDb(
 }
 
 // TODO add case for nested segwit
+/**
+  * The database level representation of a UTXO.
+  * When storing a UTXO we don't want to store
+  * sensitive material such as private keys.
+  * We instead store the necessary information
+  * we need to derive the private keys, given
+  * the root wallet seed.
+  */
 sealed trait UTXOSpendingInfoDb
     extends DbRowAutoInc[UTXOSpendingInfoDb]
     with BitcoinSLogger {
@@ -68,6 +80,9 @@ sealed trait UTXOSpendingInfoDb
 
   def value: CurrencyUnit = output.value
 
+  /** Converts a non-sensitive DB representation of a UTXO into
+    * a signable (and sensitive) real-world UTXO
+    */
   def toUTXOSpendingInfo(
       account: AccountDb,
       walletSeed: BIP39Seed): BitcoinUTXOSpendingInfo = {
@@ -127,9 +142,9 @@ case class UTXOSpendingInfoTable(tag: Tag)
           outpoint,
           output,
           path: SegWitHDPath,
-          None, // ScriptPubKey
+          None, // ReedemScript
           Some(scriptWitness)) =>
-      SegWitUTOXSpendingInfoDb(id, outpoint, output, path, scriptWitness)
+      NativeV0UTXOSpendingInfoDb(id, outpoint, output, path, scriptWitness)
 
     case (id,
           outpoint,