Delete empty labels when they are updated

In order to delete a label, client sends in the
request a null value as label value.

Co-Authored-by: Antoine Poinsot <darosior@protonmail.com>

doc/API.md

@@ -312,8 +312,9 @@ cover the requested feerate.
 
 ### `updatelabels`
 
-Update the labels from a given map of key/value, with the labelled bitcoin addresses, txids and outpoints as keys
-and the label as value. If a label already exist for the given item, the new label overrides the previous one. 
+Update the labels from a given map of key/value, with the labelled bitcoin addresses, txids and
+outpoints as keys and the label as value. If a label already exist for the given item, the new label
+overrides the previous one. If a `null` value is passed, the label is deleted.
 
 #### Request
 

src/commands/mod.rs

@@ -592,7 +592,7 @@ impl DaemonControl {
         Ok(())
     }
 
-    pub fn update_labels(&self, items: &HashMap<LabelItem, String>) {
+    pub fn update_labels(&self, items: &HashMap<LabelItem, Option<String>>) {
         let mut db_conn = self.db.connection();
         db_conn.update_labels(items);
     }

src/database/mod.rs

@@ -129,7 +129,9 @@ pub trait DatabaseConnection {
     /// Delete a Spend transaction from database.
     fn delete_spend(&mut self, txid: &bitcoin::Txid);
 
-    fn update_labels(&mut self, items: &HashMap<LabelItem, String>);
+    /// Update, for a set of items (as key), their label (as value). A `None` value deletes the
+    /// label.
+    fn update_labels(&mut self, items: &HashMap<LabelItem, Option<String>>);
 
     fn labels(&mut self, labels: &HashSet<LabelItem>) -> HashMap<String, String>;
 
@@ -275,7 +277,7 @@ impl DatabaseConnection for SqliteConn {
         self.delete_spend(txid)
     }
 
-    fn update_labels(&mut self, items: &HashMap<LabelItem, String>) {
+    fn update_labels(&mut self, items: &HashMap<LabelItem, Option<String>>) {
         self.update_labels(items)
     }
 

src/database/sqlite/mod.rs

@@ -585,7 +585,7 @@ impl SqliteConn {
         .expect("Db must not fail")
     }
 
-    pub fn update_labels(&mut self, items: &HashMap<LabelItem, String>) {
+    pub fn update_labels(&mut self, items: &HashMap<LabelItem, Option<String>>) {
         db_exec(&mut self.conn, |db_tx| {
             for (labelled, kind, value) in items
                 .iter()
@@ -596,11 +596,18 @@ impl SqliteConn {
                          LabelItem::OutPoint(a) =>(a.to_string(), DbLabelledKind::OutPoint, v),
                      }
                 }) {
-                db_tx.execute(
-                    "INSERT INTO labels (wallet_id, item, item_kind, value) VALUES (?1, ?2, ?3, ?4) \
-                    ON CONFLICT DO UPDATE SET value=excluded.value",
-                    rusqlite::params![WALLET_ID, labelled, kind as i64, value],
-                )?;
+                if let Some(value) = value {
+                    db_tx.execute(
+                        "INSERT INTO labels (wallet_id, item, item_kind, value) VALUES (?1, ?2, ?3, ?4) \
+                        ON CONFLICT DO UPDATE SET value=excluded.value",
+                        rusqlite::params![WALLET_ID, labelled, kind as i64, value],
+                    )?;
+                } else {
+                    db_tx.execute(
+                        "DELETE FROM labels WHERE wallet_id = ?1 AND item = ?2",
+                        rusqlite::params![WALLET_ID, labelled],
+                    )?;
+                }
             }
             Ok(())
         })
@@ -895,18 +902,24 @@ CREATE TABLE spend_transactions (
             assert!(db_labels.is_empty());
 
             let mut txids_labels = HashMap::new();
-            txids_labels.insert(txid.clone(), "hello".to_string());
+            txids_labels.insert(txid.clone(), Some("hello".to_string()));
 
             conn.update_labels(&txids_labels);
 
             let db_labels = conn.db_labels(&items);
             assert_eq!(db_labels[0].value, "hello");
 
-            txids_labels.insert(txid, "hello again".to_string());
+            txids_labels.insert(txid.clone(), Some("hello again".to_string()));
             conn.update_labels(&txids_labels);
 
             let db_labels = conn.db_labels(&items);
             assert_eq!(db_labels[0].value, "hello again");
+
+            // Now delete the label by passing a None value.
+            *txids_labels.get_mut(&txid).unwrap() = None;
+            conn.update_labels(&txids_labels);
+            let db_labels = conn.db_labels(&items);
+            assert!(db_labels.is_empty());
         }
 
         fs::remove_dir_all(tmp_dir).unwrap();
@@ -2102,7 +2115,7 @@ CREATE TABLE spend_transactions (
             let txid_str = "0c62a990d20d54429e70859292e82374ba6b1b951a3ab60f26bb65fee5724ff7";
             let txid = LabelItem::from_str(txid_str, bitcoin::Network::Bitcoin).unwrap();
             let mut txids_labels = HashMap::new();
-            txids_labels.insert(txid.clone(), "hello".to_string());
+            txids_labels.insert(txid.clone(), Some("hello".to_string()));
             conn.update_labels(&txids_labels);
 
             let mut items = HashSet::new();

src/jsonrpc/api.rs

@@ -223,15 +223,14 @@ fn update_labels(control: &DaemonControl, params: Params) -> Result<serde_json::
         .ok_or_else(|| Error::invalid_params("Invalid 'labels' parameter."))?
         .iter()
     {
-        let value = value
-            .as_str()
-            .map(|s| s.to_string())
-            .ok_or_else(|| Error::invalid_params(format!("Invalid 'labels.{}' value.", item)))?;
-        if value.len() > 100 {
-            return Err(Error::invalid_params(format!(
-                "Invalid 'labels.{}' value length: must be less or equal than 100 characters",
-                item
-            )));
+        let value = value.as_str().map(|s| s.to_string());
+        if let Some(value) = &value {
+            if value.len() > 100 {
+                return Err(Error::invalid_params(format!(
+                    "Invalid 'labels.{}' value length: must be less or equal than 100 characters",
+                    item
+                )));
+            }
         }
         let item =
             LabelItem::from_str(item, control.config.bitcoin_config.network).ok_or_else(|| {

src/testutils.rs

@@ -363,7 +363,7 @@ impl DatabaseConnection for DummyDatabase {
         todo!()
     }
 
-    fn update_labels(&mut self, _items: &HashMap<LabelItem, String>) {
+    fn update_labels(&mut self, _items: &HashMap<LabelItem, Option<String>>) {
         todo!()
     }
 

tests/test_rpc.py

@@ -952,3 +952,17 @@ def test_labels(lianad, bitcoind):
     assert res[inexistent_txid] == "inex_txid"
     assert res[inexistent_outpoint] == "inex_outpoint"
     assert res[random_address] == "bitcoind-addr"
+
+    # Delete 2 of the labels set above. They shouldn't be returned anymore.
+    lianad.rpc.updatelabels(
+        {
+            addr: None,
+            sec_addr: None,
+            random_address: "this address is random",
+        }
+    )
+    res = lianad.rpc.getlabels([addr, sec_addr, random_address])["labels"]
+    assert len(res) == 1
+    assert addr not in res
+    assert sec_addr not in res
+    assert res[random_address] == "this address is random"