Optionally allow change to be sent to a different subaddress (#551)

* Optionally allow change to be sent to a different subaddress

* Adds test for change going to the correct subaddress

* Add change subaddress to mobilecoind-json API

* Set boolean override flag from mobilecoind-json

* Revert files that should have been in this PR

* Remove println

* Style changes and improved testing per feedback

* Additional assert to make sure change isn't returned twice

* fmt

mobilecoind-json/src/bin/main.rs

@@ -345,6 +345,14 @@ fn build_and_submit(
     req.set_sender_subaddress(subaddress_index);
     req.set_outlay_list(RepeatedField::from_vec(vec![outlay]));
     req.set_max_input_utxo_value(max_input_utxo_value);
+    if let Some(subaddress) = transfer.change_subaddress.as_ref() {
+        req.set_override_change_subaddress(true);
+        req.set_change_subaddress(
+            subaddress
+                .parse::<u64>()
+                .map_err(|err| format!("Failed to parse change subaddress: {}", err))?,
+        )
+    }
 
     let resp = state
         .mobilecoind_api_client
@@ -391,6 +399,14 @@ fn pay_address_code(
     req.set_receiver_b58_code(transfer.receiver_b58_address_code.clone());
     req.set_amount(amount);
     req.set_max_input_utxo_value(max_input_utxo_value);
+    if let Some(subaddress) = transfer.change_subaddress.as_ref() {
+        req.set_override_change_subaddress(true);
+        req.set_change_subaddress(
+            subaddress
+                .parse::<u64>()
+                .map_err(|err| format!("Failed to parse change subaddress: {}", err))?,
+        )
+    }
 
     let resp = state
         .mobilecoind_api_client

mobilecoind-json/src/data_types.rs

@@ -398,6 +398,7 @@ impl From<&mc_mobilecoind_api::ReceiverTxReceipt> for JsonReceiverTxReceipt {
 pub struct JsonSendPaymentRequest {
     pub request_data: JsonParseRequestCodeResponse,
     pub max_input_utxo_value: Option<String>, // String due to u64 limitation.
+    pub change_subaddress: Option<String>,
 }
 
 #[derive(Deserialize, Serialize)]
@@ -424,6 +425,7 @@ pub struct JsonPayAddressCodeRequest {
     pub receiver_b58_address_code: String,
     pub value: String,
     pub max_input_utxo_value: Option<String>,
+    pub change_subaddress: Option<String>,
 }
 
 #[derive(Deserialize, Serialize)]

mobilecoind/api/proto/mobilecoind_api.proto

@@ -625,6 +625,10 @@ message SendPaymentRequest {
     // Optional: When selecting input UTXOs for the transaction, limit selection only to UTXOs whose
     // value is lower or equal to to this.
     uint64 max_input_utxo_value = 6;
+
+    // Optional: Return change to a different subaddress than the sender
+    bool override_change_subaddress = 7;
+    uint64 change_subaddress = 8;
 }
 message SendPaymentResponse {
     // Information the sender can use to check if the transaction landed in the ledger.
@@ -663,6 +667,10 @@ message PayAddressCodeRequest {
     // Optional: When selecting input UTXOs for the transaction, limit selection only to UTXOs whose
     // value is lower or equal to to this.
     uint64 max_input_utxo_value = 7;
+
+    // Optional: Return change to a different subaddress than the sender
+    bool override_change_subaddress = 8;
+    uint64 change_subaddress = 9;
 }
 
 //

mobilecoind/src/service.rs

@@ -1419,12 +1419,19 @@ impl<
             })
             .collect::<Result<Vec<Outlay>, RpcStatus>>()?;
 
+        // Set change address to sender address unless it has been overridden
+        let change_subaddress = if request.override_change_subaddress {
+            request.change_subaddress
+        } else {
+            request.sender_subaddress
+        };
+
         // Attempt to construct a transaction.
         let tx_proposal = self
             .transactions_manager
             .build_transaction(
                 &sender_monitor_id,
-                request.sender_subaddress,
+                change_subaddress,
                 &utxos,
                 &outlays,
                 request.fee,
@@ -1479,6 +1486,8 @@ impl<
         send_payment_request.set_fee(request.get_fee());
         send_payment_request.set_tombstone(request.get_tombstone());
         send_payment_request.set_max_input_utxo_value(request.get_max_input_utxo_value());
+        send_payment_request.set_override_change_subaddress(request.override_change_subaddress);
+        send_payment_request.set_change_subaddress(request.change_subaddress);
 
         self.send_payment_impl(send_payment_request)
     }
@@ -1582,6 +1591,7 @@ mod test {
     use super::*;
     use crate::{
         payments::DEFAULT_NEW_TX_BLOCK_ATTEMPTS,
+        subaddress_store::SubaddressSPKId,
         test_utils::{
             self, add_block_to_ledger_db, add_txos_to_ledger_db, get_testing_environment,
             wait_for_monitors, DEFAULT_PER_RECIPIENT_AMOUNT,
@@ -1598,7 +1608,7 @@ mod test {
         constants::{MAX_INPUTS, MINIMUM_FEE, RING_SIZE},
         fog_hint::FogHint,
         get_tx_out_shared_secret,
-        onetime_keys::recover_onetime_private_key,
+        onetime_keys::{recover_onetime_private_key, recover_public_subaddress_spend_key},
         tx::{Tx, TxOut},
         Block, BlockContents, BLOCK_VERSION,
     };
@@ -3862,6 +3872,131 @@ mod test {
         );
     }
 
+    #[test_with_logger]
+    fn test_pay_address_code_alternate_change(logger: Logger) {
+        let mut rng: StdRng = SeedableRng::from_seed([23u8; 32]);
+
+        let sender = AccountKey::random(&mut rng);
+        let data = MonitorData::new(
+            sender.clone(),
+            0,  // first_subaddress
+            20, // num_subaddresses
+            0,  // first_block
+            "", // name
+        )
+        .unwrap();
+
+        // 1 known recipient, 3 random recipients and no monitors.
+        let (ledger_db, mobilecoind_db, client, _server, server_conn_manager) =
+            get_testing_environment(
+                3,
+                &vec![sender.default_subaddress()],
+                &vec![],
+                logger.clone(),
+                &mut rng,
+            );
+
+        // Insert into database.
+        let monitor_id = mobilecoind_db.add_monitor(&data).unwrap();
+
+        // Allow the new monitor to process the ledger.
+        wait_for_monitors(&mobilecoind_db, &ledger_db, &logger);
+
+        // Get list of unspent tx outs
+        let utxos = mobilecoind_db
+            .get_utxos_for_subaddress(&monitor_id, 0)
+            .unwrap();
+        assert!(!utxos.is_empty());
+
+        // Generate a random recipient.
+        let receiver = AccountKey::random(&mut rng);
+
+        // Generate b58 address code for this recipient.
+        let receiver_public_address = receiver.default_subaddress();
+        let mut wrapper = mc_mobilecoind_api::printable::PrintableWrapper::new();
+        wrapper.set_public_address((&receiver_public_address).into());
+        let b58_code = wrapper.b58_encode().unwrap();
+
+        let test_amount = 345;
+
+        let mut request = mc_mobilecoind_api::PayAddressCodeRequest::new();
+        request.set_sender_monitor_id(monitor_id.to_vec());
+        request.set_sender_subaddress(0);
+        request.set_receiver_b58_code(b58_code);
+        request.set_amount(test_amount);
+        request.set_override_change_subaddress(true);
+        request.set_change_subaddress(1);
+
+        // Explicitly set fee so we can check change amount
+        let fee = 1000;
+        request.set_fee(fee);
+
+        let response = client.pay_address_code(&request).unwrap();
+        let total_value = response
+            .get_tx_proposal()
+            .get_input_list()
+            .iter()
+            .map(|utxo| utxo.value as u64)
+            .sum::<u64>();
+
+        let mut opt_submitted_tx: Option<Tx> = None;
+        for mock_peer in server_conn_manager.conns() {
+            let inner = mock_peer.read();
+            match (inner.proposed_txs.len(), opt_submitted_tx.clone()) {
+                (0, _) => {
+                    // Nothing submitted to the current peer.
+                }
+                (1, None) => {
+                    // Found our tx.
+                    opt_submitted_tx = Some(inner.proposed_txs[0].clone())
+                }
+                (1, Some(_)) => {
+                    panic!("Tx submitted to two peers?!");
+                }
+                (_, _) => {
+                    panic!("Multiple transactions submitted?!");
+                }
+            }
+        }
+
+        let submitted_tx = opt_submitted_tx.unwrap();
+        let mut change_subaddress_found = false;
+        for tx_out in submitted_tx.prefix.outputs {
+            let tx_out_target_key = RistrettoPublic::try_from(&tx_out.target_key).unwrap();
+            let tx_public_key = RistrettoPublic::try_from(&tx_out.public_key).unwrap();
+
+            let subaddress_spk = SubaddressSPKId::from(&recover_public_subaddress_spend_key(
+                &sender.view_private_key(),
+                &tx_out_target_key,
+                &tx_public_key,
+            ));
+
+            match mobilecoind_db.get_subaddress_id_by_spk(&subaddress_spk) {
+                Ok(data) => {
+                    if data.index == 1 {
+                        assert!(!change_subaddress_found);
+                        change_subaddress_found = true;
+                        let shared_secret =
+                            get_tx_out_shared_secret(sender.view_private_key(), &tx_public_key);
+
+                        let (change_value, _blinding) = tx_out
+                            .amount
+                            .get_value(&shared_secret)
+                            .expect("Malformed amount");
+
+                        assert_eq!(total_value - test_amount - fee, change_value);
+                    }
+                }
+                Err(Error::SubaddressSPKNotFound) => continue,
+                Err(_err) => {
+                    panic!("Error matching subaddress");
+                }
+            };
+        }
+
+        assert!(change_subaddress_found);
+    }
+
     #[test_with_logger]
     fn test_request_code(logger: Logger) {
         let mut rng: StdRng = SeedableRng::from_seed([23u8; 32]);