2022 03 09 label refactor (#4175)

* Rename existing getaddresslabels -> getaddresslabel

* Fix missing rename of GetAddressLabel

* Modify pk constraint on wallet_address_tags to be tag_name rather than tag_type

* Add dropaddresslabel for a specific address

* Fix migrations

* Add unit tests and fix existing tests

* Add docs

app/cli/src/main/scala/org/bitcoins/cli/ConsoleCli.scala

@@ -320,8 +320,8 @@ object ConsoleCli {
                 case other => other
               }))
         ),
-      cmd("getaddresslabels")
+      cmd("getaddresslabel")
-        .action((_, conf) => conf.copy(command = GetAddressLabels(null)))
+        .action((_, conf) => conf.copy(command = GetAddressLabel(null)))
         .text("Get all the labels associated with this address")
         .children(
           arg[BitcoinAddress]("address")
@@ -329,14 +329,17 @@ object ConsoleCli {
             .required()
             .action((addr, conf) =>
               conf.copy(command = conf.command match {
-                case getAddressLabels: GetAddressLabels =>
+                case getAddressLabels: GetAddressLabel =>
                   getAddressLabels.copy(address = addr)
                 case other => other
               }))
         ),
+      cmd("getaddresslabels")
+        .action((_, conf) => conf.copy(command = GetAddressLabels))
+        .text("Returns all labels in wallet"),
       cmd("dropaddresslabels")
         .action((_, conf) => conf.copy(command = DropAddressLabels(null)))
-        .text("Drop all the labels associated with this address")
+        .text("Drop the label associated with the address")
         .children(
           arg[BitcoinAddress]("address")
             .text("The address to drop the associated labels of")
@@ -348,6 +351,29 @@ object ConsoleCli {
                 case other => other
               }))
         ),
+      cmd("dropaddresslabel")
+        .action((_, conf) => conf.copy(command = DropAddressLabel(null, null)))
+        .text("Drop all the labels associated with this address")
+        .children(
+          arg[BitcoinAddress]("address")
+            .text("The address to drop the associated labels of")
+            .required()
+            .action((addr, conf) =>
+              conf.copy(command = conf.command match {
+                case dropAddressLabel: DropAddressLabel =>
+                  dropAddressLabel.copy(address = addr)
+                case other => other
+              })),
+          arg[String]("label")
+            .text("The label to drop")
+            .required()
+            .action((label, conf) =>
+              conf.copy(command = conf.command match {
+                case dropAddressLabel: DropAddressLabel =>
+                  dropAddressLabel.copy(label = label)
+                case other => other
+              }))
+        ),
       cmd("sendtoaddress")
         .action(
           // TODO how to handle null here?
@@ -1852,8 +1878,13 @@ object ConsoleCli {
                      Seq(up.writeJs(address), up.writeJs(label)))
       case GetAddressTags(address) =>
         RequestParam("getaddresstags", Seq(up.writeJs(address)))
-      case GetAddressLabels(address) =>
+      case GetAddressLabel(address) =>
-        RequestParam("getaddresslabels", Seq(up.writeJs(address)))
+        RequestParam("getaddresslabel", Seq(up.writeJs(address)))
+      case GetAddressLabels =>
+        RequestParam("getaddresslabels")
+      case DropAddressLabel(address, label) =>
+        RequestParam("dropaddresslabel",
+                     Seq(up.writeJs(address), ujson.Str(label)))
       case DropAddressLabels(address) =>
         RequestParam("dropaddresslabels", Seq(up.writeJs(address)))
       case Rescan(addressBatchSize,
@@ -2355,7 +2386,12 @@ object CliCommand {
 
   case class GetAddressTags(address: BitcoinAddress) extends AppServerCliCommand
 
-  case class GetAddressLabels(address: BitcoinAddress)
+  case class GetAddressLabel(address: BitcoinAddress)
+      extends AppServerCliCommand
+
+  case object GetAddressLabels extends AppServerCliCommand
+
+  case class DropAddressLabel(address: BitcoinAddress, label: String)
       extends AppServerCliCommand
 
   case class DropAddressLabels(address: BitcoinAddress)

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

@@ -744,7 +744,7 @@ class RoutesSpec extends AnyWordSpec with ScalatestRouteTest with MockFactory {
       }
     }
 
-    "get address labels" in {
+    "get address label" in {
       (mockWalletApi
         .getAddressTags(_: BitcoinAddress, _: AddressTagType))
         .expects(testAddress, AddressLabelTagType)
@@ -753,7 +753,7 @@ class RoutesSpec extends AnyWordSpec with ScalatestRouteTest with MockFactory {
 
       val route =
         walletRoutes.handleCommand(
-          ServerCommand("getaddresslabels", Arr(Str(testAddressStr))))
+          ServerCommand("getaddresslabel", Arr(Str(testAddressStr))))
 
       Get() ~> route ~> check {
         assert(contentType == `application/json`)
@@ -762,6 +762,41 @@ class RoutesSpec extends AnyWordSpec with ScalatestRouteTest with MockFactory {
       }
     }
 
+    "get address labels" in {
+      (mockWalletApi.getAddressTags: () => Future[Vector[AddressTagDb]])
+        .expects()
+        .returning(
+          Future.successful(Vector(AddressTagDb(testAddress, testLabel))))
+
+      val route =
+        walletRoutes.handleCommand(ServerCommand("getaddresslabels", Arr()))
+
+      Get() ~> route ~> check {
+        assert(contentType == `application/json`)
+        assert(
+          responseAs[String] == """{"result":[{"address":"1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa","labels":["test"]}],"error":null}""")
+      }
+    }
+
+    "drop address label" in {
+      val labelName = "label"
+      (mockWalletApi
+        .dropAddressTagName(_: BitcoinAddress, _: AddressTagName))
+        .expects(testAddress, AddressLabelTagName(labelName))
+        .returning(Future.successful(1))
+
+      val route =
+        walletRoutes.handleCommand(
+          ServerCommand("dropaddresslabel",
+                        Arr(Str(testAddressStr), Str(labelName))))
+
+      Get() ~> route ~> check {
+        assert(contentType == `application/json`)
+        assert(
+          responseAs[String] == """{"result":"""" + "1 label dropped" + """","error":null}""")
+      }
+    }
+
     "drop address labels with no labels" in {
       (mockWalletApi
         .dropAddressTagType(_: BitcoinAddress, _: AddressTagType))

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

@@ -114,17 +114,17 @@ object GetAddressTags extends ServerJsonModels {
   }
 }
 
-case class GetAddressLabels(address: BitcoinAddress)
+case class GetAddressLabel(address: BitcoinAddress)
 
-object GetAddressLabels extends ServerJsonModels {
+object GetAddressLabel extends ServerJsonModels {
 
-  def fromJsArr(jsArr: ujson.Arr): Try[GetAddressLabels] = {
+  def fromJsArr(jsArr: ujson.Arr): Try[GetAddressLabel] = {
     jsArr.arr.toList match {
       case addrJs :: Nil =>
         Try {
           val addr = jsToBitcoinAddress(addrJs)
 
-          GetAddressLabels(addr)
+          GetAddressLabel(addr)
         }
       case other =>
         Failure(
@@ -134,6 +134,25 @@ object GetAddressLabels extends ServerJsonModels {
   }
 }
 
+case class DropAddressLabel(address: BitcoinAddress, label: String)
+
+object DropAddressLabel extends ServerJsonModels {
+
+  def fromJsArr(jsonArr: ujson.Arr): Try[DropAddressLabel] = {
+    jsonArr.arr.toList match {
+      case address :: label :: Nil =>
+        Try {
+          val addr = jsToBitcoinAddress(address)
+          DropAddressLabel(addr, label.str)
+        }
+      case other =>
+        Failure(
+          new IllegalArgumentException(
+            s"Bad number of arguments: ${other.length}. Expected: 2"))
+    }
+  }
+}
+
 case class DropAddressLabels(address: BitcoinAddress)
 
 object DropAddressLabels extends ServerJsonModels {

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

@@ -1,6 +1,7 @@
 package org.bitcoins.server
 
 import akka.actor.ActorSystem
+import akka.http.scaladsl.model.HttpEntity
 import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server._
 import akka.stream.Materializer
@@ -12,7 +13,11 @@ import org.bitcoins.core.currency._
 import org.bitcoins.core.protocol.tlv._
 import org.bitcoins.core.protocol.transaction.Transaction
 import org.bitcoins.core.wallet.fee.SatoshisPerVirtualByte
-import org.bitcoins.core.wallet.utxo.{AddressLabelTagType, TxoState}
+import org.bitcoins.core.wallet.utxo.{
+  AddressLabelTagName,
+  AddressLabelTagType,
+  TxoState
+}
 import org.bitcoins.crypto.NetworkElement
 import org.bitcoins.keymanager._
 import org.bitcoins.keymanager.config.KeyManagerAppConfig
@@ -228,9 +233,9 @@ case class WalletRoutes(wallet: AnyDLCHDWalletApi)(implicit
           }
       }
 
-    case ServerCommand("getaddresslabels", arr) =>
+    case ServerCommand("getaddresslabel", arr) =>
-      withValidServerCommand(GetAddressLabels.fromJsArr(arr)) {
+      withValidServerCommand(GetAddressLabel.fromJsArr(arr)) {
-        case GetAddressLabels(address) =>
+        case GetAddressLabel(address) =>
           complete {
             wallet.getAddressTags(address, AddressLabelTagType).map { tagDbs =>
               val retStr = tagDbs.map(_.tagName.name)
@@ -239,20 +244,38 @@ case class WalletRoutes(wallet: AnyDLCHDWalletApi)(implicit
           }
       }
 
+    case ServerCommand("getaddresslabels", _) =>
+      complete {
+        val allTagsF = wallet.getAddressTags()
+        for {
+          allTags <- allTagsF
+          grouped = allTags.groupBy(_.address)
+        } yield {
+          val json: Vector[ujson.Obj] = grouped.map { case (address, labels) =>
+            val tagNames: Vector[ujson.Str] =
+              labels.map(l => ujson.Str(l.tagName.name))
+            ujson.Obj(("address", address.toString),
+                      ("labels", ujson.Arr.from(tagNames)))
+          }.toVector
+          Server.httpSuccess(ujson.Arr.from(json))
+        }
+      }
+    case ServerCommand("dropaddresslabel", arr) =>
+      withValidServerCommand(DropAddressLabel.fromJsArr(arr)) {
+        case DropAddressLabel(address, label) =>
+          complete {
+            val tagName = AddressLabelTagName(label)
+            val droppedF = wallet.dropAddressTagName(address, tagName)
+            droppedF.map(handleTagResponse)
+          }
+      }
     case ServerCommand("dropaddresslabels", arr) =>
       withValidServerCommand(DropAddressLabels.fromJsArr(arr)) {
         case DropAddressLabels(address) =>
           complete {
-            wallet.dropAddressTagType(address, AddressLabelTagType).map {
+            val droppedF =
-              numDropped =>
+              wallet.dropAddressTagType(address, AddressLabelTagType)
-                if (numDropped <= 0) {
+            droppedF.map(handleTagResponse)
-                  Server.httpSuccess(s"Address had no labels")
-                } else if (numDropped == 1) {
-                  Server.httpSuccess(s"$numDropped label dropped")
-                } else {
-                  Server.httpSuccess(s"$numDropped labels dropped")
-                }
-            }
           }
       }
 
@@ -898,4 +921,14 @@ case class WalletRoutes(wallet: AnyDLCHDWalletApi)(implicit
       Bitcoins(currencyUnit.satoshis).toBigDecimal.toDouble
     }
   }
+
+  private def handleTagResponse(numDropped: Int): HttpEntity.Strict = {
+    if (numDropped <= 0) {
+      Server.httpSuccess(s"Address had no labels")
+    } else if (numDropped == 1) {
+      Server.httpSuccess(s"$numDropped label dropped")
+    } else {
+      Server.httpSuccess(s"$numDropped labels dropped")
+    }
+  }
 }

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

@@ -17,7 +17,12 @@ import org.bitcoins.core.protocol.transaction.{
 }
 import org.bitcoins.core.util.{FutureUtil, StartStopAsync}
 import org.bitcoins.core.wallet.fee.FeeUnit
-import org.bitcoins.core.wallet.utxo.{AddressTag, AddressTagType, TxoState}
+import org.bitcoins.core.wallet.utxo.{
+  AddressTag,
+  AddressTagName,
+  AddressTagType,
+  TxoState
+}
 import org.bitcoins.crypto.DoubleSha256DigestBE
 
 import java.time.Instant
@@ -218,7 +223,7 @@ trait WalletApi extends StartStopAsync[WalletApi] {
       address: BitcoinAddress,
       tagType: AddressTagType): Future[Vector[AddressTagDb]]
 
-  def getAddressTags: Future[Vector[AddressTagDb]]
+  def getAddressTags(): Future[Vector[AddressTagDb]]
 
   def getAddressTags(tagType: AddressTagType): Future[Vector[AddressTagDb]]
 
@@ -230,6 +235,10 @@ trait WalletApi extends StartStopAsync[WalletApi] {
       address: BitcoinAddress,
       addressTagType: AddressTagType): Future[Int]
 
+  def dropAddressTagName(
+      address: BitcoinAddress,
+      tagName: AddressTagName): Future[Int]
+
   /** Generates a new change address */
   protected[wallet] def getNewChangeAddress()(implicit
       ec: ExecutionContext): Future[BitcoinAddress]

db-commons-test/src/test/scala/org/bitcoins/db/DbManagementTest.scala

@@ -106,13 +106,13 @@ class DbManagementTest extends BitcoinSAsyncTest with EmbeddedPg {
     val result = walletDbManagement.migrate()
     walletAppConfig.driver match {
       case SQLite =>
-        val expected = 14
+        val expected = 15
         assert(result == expected)
         val flywayInfo = walletDbManagement.info()
         assert(flywayInfo.applied().length == expected)
         assert(flywayInfo.pending().length == 0)
       case PostgreSQL =>
-        val expected = 12
+        val expected = 13
         assert(result == expected)
         val flywayInfo = walletDbManagement.info()
 

docs/applications/server.md

@@ -306,9 +306,13 @@ the `-p 9999:9999` port mapping on the docker container to adjust for this.
     - `message` - Peer's message or note (optional)
  - `"offer-remove` `hash` - Remove an incoming offer from inbox
     - `hash` - Hash of the offer TLV
-- `offer-send` `offerOrTempContractId` `peerAddress` `message` - Sends an offer to a peer. `offerOrTempContractId` is either an offer TLV or a temporary contract ID.
+ - `offer-send` `offerOrTempContractId` `peerAddress` `message` - Sends an offer to a peer. `offerOrTempContractId` is either an offer TLV or a temporary contract ID.
-- `offers-list` - List all incoming offers from the inbox
+ - `offers-list` - List all incoming offers from the inbox
-- `getdlcoffer` `tempContractId` - Gets a DLC offer by temporary contract ID.
+ - `getdlcoffer` `tempContractId` - Gets a DLC offer by temporary contract ID.
+ - `getaddresslabel` `address` - gets all labels for an address
+ - `getaddresslabels` - returns all addresses with labels in the wallet
+ - `dropaddresslabel` `address` `label` - drops the label for a given address
+ - `dropaddresslabels` `address` - drops all labels for the given address
 
 ### Network
  - `getpeers` - List the connected peers

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

@@ -283,7 +283,7 @@ class AddressHandlingTest extends BitcoinSWalletTest {
       for {
         _ <- addressF
         _ <- wallet.clearAllUtxosAndAddresses()
-        tags <- wallet.getAddressTags
+        tags <- wallet.getAddressTags()
       } yield {
         assert(tags.isEmpty)
       }

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

@@ -5,6 +5,8 @@ import org.bitcoins.testkit.wallet.BitcoinSWalletTest
 import org.bitcoins.testkit.wallet.FundWalletUtil.FundedWallet
 import org.scalatest.FutureOutcome
 
+import java.sql.SQLException
+
 class AddressLabelTest extends BitcoinSWalletTest {
   type FixtureParam = FundedWallet
 
@@ -14,18 +16,18 @@ class AddressLabelTest extends BitcoinSWalletTest {
 
   behavior of "Address tags"
 
-  it must "add two labels to the database" in { fundedWallet =>
+  it must "add two tags to the database" in { fundedWallet =>
     val wallet = fundedWallet.wallet
     val tag1 = UnknownAddressTag("test_tag_name_1", "test_tag_type_1")
     val tag2 = UnknownAddressTag("test_tag_name_2", "test_tag_type_2")
     val addressF = for {
       address <- wallet.getNewAddress(Vector(tag1))
       //add another tag to address
-      tagDb1 <- wallet.tagAddress(address, tag1)
+      tagDb1 <- wallet.getAddressTags(address)
       tagDb2 <- wallet.tagAddress(address, tag2)
     } yield {
-      assert(tagDb1.address == address)
+      assert(tagDb1.head.address == address)
-      assert(tagDb1.tagName == tag1.tagName)
+      assert(tagDb1.head.tagName == tag1.tagName)
 
       assert(tagDb2.tagName == tag2.tagName)
       assert(tagDb2.address == address)
@@ -33,4 +35,21 @@ class AddressLabelTest extends BitcoinSWalletTest {
 
     addressF
   }
+
+  it must "fail if we tag the address with the same tag twice" in {
+    fundedWallet =>
+      val wallet = fundedWallet.wallet
+      val tag1 = UnknownAddressTag(tagName = "test_tag_name_1",
+                                   tagType = "test_tag_type_1")
+      val tag2 = UnknownAddressTag(tagName = "test_tag_name_1",
+                                   tagType = "test_tag_type_2")
+      val resultF = for {
+        address <- wallet.getNewAddress()
+        //add another tag to address
+        _ <- wallet.tagAddress(address, tag1)
+        _ <- wallet.tagAddress(address, tag2)
+      } yield ()
+
+      recoverToSucceededIf[SQLException](resultF)
+  }
 }

wallet-test/src/test/scala/org/bitcoins/wallet/models/AddressTagDAOTest.scala

@@ -83,4 +83,29 @@ class AddressTagDAOTest extends WalletDAOFixture {
     daos =>
       testInsertion(daos, HotStorage)
   }
+
+  it must "delete a tag by name" in { daos =>
+    val accountDAO = daos.accountDAO
+    val addressDAO = daos.addressDAO
+    val addressTagDAO = daos.addressTagDAO
+    for {
+      createdAccount <- {
+        val account = WalletTestUtil.firstAccountDb
+        accountDAO.create(account)
+      }
+      createdAddress <- {
+        val addressDb = WalletTestUtil.getAddressDb(createdAccount)
+        addressDAO.create(addressDb)
+      }
+      createdAddressTag <- {
+        val tagDb =
+          AddressTagDb(createdAddress.address, exampleTag)
+        addressTagDAO.create(tagDb)
+      }
+      dropped <- addressTagDAO.dropByAddressAndName(createdAddress.address,
+                                                    createdAddressTag.tagName)
+    } yield {
+      assert(dropped == 1)
+    }
+  }
 }

wallet/src/main/resources/postgresql/wallet/migration/V16__add_tag_name_constraint.sql

@@ -0,0 +1,4 @@
+--changes pk to (address,tag_name) rather than (address,tag_type)
+ALTER TABLE wallet_address_tags DROP CONSTRAINT IF EXISTS "pk_address_tags";
+
+ALTER TABLE wallet_address_tags ADD CONSTRAINT pk_address_tags PRIMARY KEY (address, tag_name);

wallet/src/main/resources/sqlite/wallet/migration/V15__add_tag_name_constraint.sql

@@ -0,0 +1,7 @@
+-- This changes the primary key to (address, tag_name)
+CREATE TABLE "wallet_address_tags_temp" ("address" VARCHAR(254) NOT NULL,"tag_name" VARCHAR(254) NOT NULL,"tag_type" VARCHAR(254) NOT NULL);
+INSERT INTO "wallet_address_tags_temp" SELECT "address", "tag_name", "tag_type" FROM "wallet_address_tags";
+DROP TABLE "wallet_address_tags";
+CREATE TABLE "wallet_address_tags" ("address" VARCHAR(254) NOT NULL,"tag_name" VARCHAR(254) NOT NULL,"tag_type" VARCHAR(254) NOT NULL,constraint "pk_address_tags" primary key ("address", "tag_name"), constraint "fk_address" foreign key("address") references "addresses"("address") on update NO ACTION on delete NO ACTION);
+INSERT INTO "wallet_address_tags" SELECT "address", "tag_name", "tag_type" FROM "wallet_address_tags_temp";
+DROP TABLE "wallet_address_tags_temp";

wallet/src/main/scala/org/bitcoins/wallet/internal/AddressHandling.scala

@@ -13,7 +13,11 @@ import org.bitcoins.core.protocol.transaction.{
   TransactionOutPoint,
   TransactionOutput
 }
-import org.bitcoins.core.wallet.utxo.{AddressTag, AddressTagType}
+import org.bitcoins.core.wallet.utxo.{
+  AddressTag,
+  AddressTagName,
+  AddressTagType
+}
 import org.bitcoins.crypto.ECPublicKey
 import org.bitcoins.wallet._
 
@@ -414,7 +418,7 @@ private[wallet] trait AddressHandling extends WalletLogger {
       address: BitcoinAddress,
       tag: AddressTag): Future[AddressTagDb] = {
     val addressTagDb = AddressTagDb(address, tag)
-    val f = addressTagDAO.upsert(addressTagDb)
+    val f = addressTagDAO.create(addressTagDb)
     f
   }
 
@@ -428,7 +432,7 @@ private[wallet] trait AddressHandling extends WalletLogger {
     addressTagDAO.findByAddressAndTag(address, tagType)
   }
 
-  def getAddressTags: Future[Vector[AddressTagDb]] = {
+  def getAddressTags(): Future[Vector[AddressTagDb]] = {
     addressTagDAO.findAll()
   }
 
@@ -451,6 +455,12 @@ private[wallet] trait AddressHandling extends WalletLogger {
     addressTagDAO.dropByAddressAndTag(address, addressTagType)
   }
 
+  override def dropAddressTagName(
+      address: BitcoinAddress,
+      addressTagName: AddressTagName): Future[Int] = {
+    addressTagDAO.dropByAddressAndName(address, addressTagName)
+  }
+
   private lazy val addressRequestQueue = {
     val queue = new java.util.concurrent.ArrayBlockingQueue[AddressRequest](
       walletConfig.addressQueueSize

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

@@ -125,6 +125,16 @@ case class AddressTagDAO()(implicit
     safeDatabase.run(query.delete)
   }
 
+  def dropByAddressAndName(
+      address: BitcoinAddress,
+      tagName: AddressTagName): Future[Int] = {
+    val query = table
+      .filter(_.address === address)
+      .filter(_.tagName === tagName)
+
+    safeDatabase.run(query.delete)
+  }
+
   def findTx(
       tx: Transaction,
       network: NetworkParameters): Future[Vector[AddressTagDb]] = {
@@ -201,7 +211,7 @@ case class AddressTagDAO()(implicit
       (address, tagName, tagType).<>(fromTuple, toTuple)
 
     def primaryKey: PrimaryKey =
-      primaryKey("pk_address_tags", sourceColumns = (address, tagType))
+      primaryKey("pk_address_tags", sourceColumns = (address, tagName))
 
     /** All tags must have an associated address */
     def fk_address = {