Merge pull request zingolabs#1209 from Oscar-Pepper/deprecate_is_change

deprecate is_change

libtonode-tests/tests/legacy.rs

@@ -2065,9 +2065,6 @@ mod slow {
             notes["spent_sapling_notes"][0]["value"].as_u64().unwrap(),
             value
         );
-        assert!(!notes["spent_sapling_notes"][0]["is_change"]
-            .as_bool()
-            .unwrap());
         assert!(!notes["spent_sapling_notes"][0]["spendable"]
             .as_bool()
             .unwrap()); // Already spent
@@ -2963,7 +2960,6 @@ mod slow {
                 "created_in_txid" => "",
                 "value" =>  24_000,
                 "pending" =>  false,
-                "is_change" =>  false,
                 "address" =>  "uregtest1m8un60udl5ac0928aghy4jx6wp59ty7ct4t8ks9udwn8y6fkdmhe6pq0x5huv8v0pprdlq07tclqgl5fzfvvzjf4fatk8cpyktaudmhvjcqufdsfmktgawvne3ksrhs97pf0u8s8f8h",
                 "spendable" =>  true,
                 "spent" =>  JsonValue::Null,

zingolib/src/commands.rs

@@ -1546,17 +1546,17 @@ struct NotesCommand {}
 impl Command for NotesCommand {
     fn help(&self) -> &'static str {
         indoc! {r#"
-            Show all sapling notes and utxos in this wallet
+            Show all shielded notes and transparent coins in this wallet
             Usage:
             notes [all]
 
-            If you supply the "all" parameter, all previously spent sapling notes and spent utxos are also included
+            If you supply the "all" parameter, all previously spent shielded notes and transparent coins are also included
 
         "#}
     }
 
     fn short_help(&self) -> &'static str {
-        "List all sapling notes and utxos in the wallet"
+        "Show all shielded notes and transparent coins in this wallet"
     }
 
     fn exec(&self, args: &[&str], lightclient: &LightClient) -> String {

zingolib/src/lightclient/deprecated.rs

@@ -111,6 +111,7 @@ impl LightClient {
     }
 
     /// TODO: Add Doc Comment Here!
+    #[allow(deprecated)]
     pub async fn do_list_transactions(&self) -> JsonValue {
         // Create a list of TransactionItems from wallet transactions
         let mut consumer_ui_notes = self

zingolib/src/lightclient/describe.rs

@@ -114,6 +114,8 @@ impl LightClient {
     /// Returns the wallet balance, broken out into several figures that are expected to be meaningful to the user.
     /// # Parameters
     /// * `auto_shielding` - if true, UTXOs will be considered immature rather than spendable.
+    #[allow(deprecated)]
+    #[deprecated(note = "uses unstable deprecated functions")]
     pub async fn get_user_balances(
         &self,
         auto_shielding: bool,
@@ -690,7 +692,6 @@ impl LightClient {
                             "created_in_txid"    => format!("{}", transaction_id.clone()),
                             "value"              => note_metadata.sapling_crypto_note.value().inner(),
                             "pending"        => !transaction_metadata.status.is_confirmed(),
-                            "is_change"          => note_metadata.is_change,
                             "address"            => address,
                             "spendable"          => spendable,
                             "spent"              => note_metadata.spent.map(|(spent_transaction_id, _)| format!("{}", spent_transaction_id)),
@@ -734,7 +735,6 @@ impl LightClient {
                             "created_in_txid"    => format!("{}", transaction_id),
                             "value"              => orch_note_metadata.orchard_crypto_note.value().inner(),
                             "pending"        => !transaction_metadata.status.is_confirmed(),
-                            "is_change"          => orch_note_metadata.is_change,
                             "address"            => address,
                             "spendable"          => spendable,
                             "spent"              => orch_note_metadata.spent.map(|(spent_transaction_id, _)| format!("{}", spent_transaction_id)),
@@ -781,7 +781,6 @@ impl LightClient {
                             "created_in_txid"    => format!("{}", transaction_id),
                             "value"              => utxo.value,
                             "scriptkey"          => hex::encode(utxo.script.clone()),
-                            "is_change"          => false, // TODO: Identify notes as change if we send change to our own taddrs
                             "address"            => self.wallet.wallet_capability().get_ua_from_contained_transparent_receiver(&taddr).map(|ua| ua.encode(&self.config.chain)),
                             "spent"              => utxo.spent().map(|(spent_transaction_id, _)| format!("{}", spent_transaction_id)),
                             "spent_at_height"    => utxo.spent().map(|(_, h)| h),

zingolib/src/wallet/disk.rs

@@ -14,7 +14,6 @@ use zcash_client_backend::proto::service::TreeState;
 use zcash_encoding::{Optional, Vector};
 
 use zcash_primitives::consensus::BlockHeight;
-use zcash_primitives::transaction::{self};
 
 use zingoconfig::ZingoConfig;
 
@@ -139,24 +138,11 @@ impl LightWallet {
             blocks = blocks.into_iter().rev().collect();
         }
 
-        let mut transactions = if external_version <= 14 {
+        let transactions = if external_version <= 14 {
             TxMapAndMaybeTrees::read_old(&mut reader, &wallet_capability)
         } else {
             TxMapAndMaybeTrees::read(&mut reader, &wallet_capability)
         }?;
-        let txids = transactions
-            .transaction_records_by_id
-            .keys()
-            .cloned()
-            .collect::<Vec<transaction::TxId>>();
-        // We've marked notes as change inconsistently in the past
-        // so we make sure that they are marked as change or not based on our
-        // current definition
-        for txid in txids {
-            transactions
-                .transaction_records_by_id
-                .check_notes_mark_change(&txid)
-        }
 
         let chain_name = utils::read_string(&mut reader)?;
 

zingolib/src/wallet/notes/orchard.rs

@@ -37,7 +37,7 @@ pub struct OrchardNote {
     /// TODO: Add Doc Comment Here!
     pub memo: Option<Memo>,
 
-    /// TODO: Add Doc Comment Here!
+    /// DEPRECATED
     pub is_change: bool,
 
     /// If the spending key is available in the wallet (i.e., whether to keep witness up-to-date)

zingolib/src/wallet/notes/sapling.rs

@@ -38,7 +38,7 @@ pub struct SaplingNote {
     /// TODO: Add Doc Comment Here!
     pub memo: Option<Memo>,
 
-    /// TODO: Add Doc Comment Here!
+    /// DEPRECATED
     pub is_change: bool,
 
     /// If the spending key is available in the wallet (i.e., whether to keep witness up-to-date) Todo should this data point really be here?

zingolib/src/wallet/transaction_context.rs

@@ -103,7 +103,7 @@ pub mod decrypt_transaction {
 
             // Post process scan results
             {
-                let mut tx_map = self.transaction_metadata_set.write().await;
+                let tx_map = self.transaction_metadata_set.write().await;
                 if let Some(transaction_record) =
                     tx_map.transaction_records_by_id.get(&transaction.txid())
                 {
@@ -129,14 +129,8 @@ pub mod decrypt_transaction {
                                 }
                             }
                         }
-                        // Also, if this is an outgoing transaction, then mark all the *incoming* notes to this transaction as change.
-                        // Note that this is also done in `WalletTxns::add_new_spent`, but that doesn't take into account transparent spends,
-                        // so we'll do it again here.
-                        tx_map
-                            .transaction_records_by_id
-                            .check_notes_mark_change(&transaction.txid());
                     }
-                };
+                }
             }
 
             if !outgoing_metadatas.is_empty() {

zingolib/src/wallet/transaction_record.rs

@@ -251,6 +251,7 @@ impl TransactionRecord {
     }
 
     /// TODO: Add Doc Comment Here!
+    #[allow(deprecated)]
     #[deprecated(
         note = "replaced by `calculate_transaction_fee` method for [`crate::wallet::transaction_records_by_id::TransactionRecordsById`]"
     )]
@@ -275,12 +276,14 @@ impl TransactionRecord {
     /// TODO: Add Doc Comment Here!
     // TODO: This is incorrect in the edge case where where we have a send-to-self with
     // no text memo and 0-value fee
+    #[deprecated(note = "uses unstable deprecated is_change")]
     pub fn is_outgoing_transaction(&self) -> bool {
         (!self.outgoing_tx_data.is_empty()) || self.total_value_spent() != 0
     }
 
     /// This means there's at least one note that adds funds
     /// to this capabilities control
+    #[deprecated(note = "uses unstable deprecated is_change")]
     pub fn is_incoming_transaction(&self) -> bool {
         self.sapling_notes
             .iter()
@@ -302,6 +305,7 @@ impl TransactionRecord {
     }
 
     /// TODO: Add Doc Comment Here!
+    #[deprecated(note = "uses unstable deprecated functions")]
     pub fn total_change_returned(&self) -> u64 {
         self.pool_change_returned::<sapling_crypto::note_encryption::SaplingDomain>()
             + self.pool_change_returned::<orchard::note_encryption::OrchardDomain>()
@@ -843,8 +847,6 @@ mod tests {
     use zcash_client_backend::ShieldedProtocol::{Orchard, Sapling};
 
     use crate::wallet::notes::query::OutputQuery;
-    use crate::wallet::notes::transparent::mocks::TransparentOutputBuilder;
-    //use crate::wallet::notes::{OrchardNote, SaplingNote, TransparentOutput};
     use crate::wallet::transaction_record::mocks::{
         nine_note_transaction_record, nine_note_transaction_record_default,
         TransactionRecordBuilder,
@@ -854,9 +856,6 @@ mod tests {
     pub fn blank_record() {
         let new = TransactionRecordBuilder::default().build();
         assert_eq!(new.total_transparent_value_spent, 0);
-        assert!(!new.is_outgoing_transaction());
-        assert!(!new.is_incoming_transaction());
-        // assert_eq!(new.net_spent(), 0);
         assert_eq!(
             new.pool_change_returned::<orchard::note_encryption::OrchardDomain>(),
             0
@@ -871,14 +870,6 @@ mod tests {
         let t: [u64; 3] = [0, 0, 0];
         assert_eq!(new.value_spent_by_pool(), t);
     }
-    #[test]
-    fn single_transparent_note_makes_is_incoming_true() {
-        // A single transparent note makes is_incoming_transaction true.
-        let transaction_record = TransactionRecordBuilder::default()
-            .transparent_outputs(TransparentOutputBuilder::default())
-            .build();
-        assert!(transaction_record.is_incoming_transaction());
-    }
 
     #[test_matrix(
         [true, false],

zingolib/src/wallet/transaction_records_by_id.rs

@@ -397,6 +397,7 @@ impl TransactionRecordsById {
     //
     // TODO: When we start working on multi-sig, this could cause issues about hiding sends-to-self
     /// TODO: Add Doc Comment Here!
+    #[deprecated(note = "uses unstable deprecated functions")]
     pub fn check_notes_mark_change(&mut self, txid: &TxId) {
         //TODO: Incorrect with a 0-value fee somehow
         if self.total_funds_spent_in(txid) > 0 {
@@ -449,8 +450,6 @@ impl TransactionRecordsById {
             self.create_modify_get_transaction_metadata(&txid, status, timestamp);
 
         transaction_metadata.total_transparent_value_spent = total_transparent_value_spent;
-
-        self.check_notes_mark_change(&txid);
     }
 
     /// TODO: Add Doc Comment Here!

zingolib/src/wallet/tx_map_and_maybe_trees/recording.rs

@@ -112,10 +112,6 @@ impl super::TxMapAndMaybeTrees {
             transaction_metadata.add_spent_nullifier(spent_nullifier.into(), value)
         }
 
-        // Since this Txid has spent some funds, output notes in this Tx that are sent to us are actually change.
-        self.transaction_records_by_id
-            .check_notes_mark_change(&spending_txid);
-
         Ok(())
     }