Replace OutputSubstitutionDisabled string context with sub variants (payjoin#590)

closes payjoin#588

Author: DanGould

payjoin-test-utils/src/lib.rs

@@ -266,6 +266,8 @@ pub const SYMMETRIC: &[SymmetricSuite] =
 // |-----------------|-----------------------|------------------------------|-------------------------|
 // | P2SH-P2WPKH     |  2 sat/vbyte          | 0.00000182                   | 0                       |
 
+pub const QUERY_PARAMS: &str = "maxadditionalfeecontribution=182&additionalfeeoutputindex=0";
+
 pub const ORIGINAL_PSBT: &str = "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";
 
 pub const PAYJOIN_PROPOSAL: &str = "cHNidP8BAJwCAAAAAo8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////jye60aAl3JgZdaIERvjkeh72VYZuTGH/ps2I4l0IO4MBAAAAAP7///8CJpW4BQAAAAAXqRQd6EnwadJ0FQ46/q6NcutaawlEMIcACT0AAAAAABepFHdAltvPSGdDwi9DR+m0af6+i2d6h9MAAAAAAQEgqBvXBQAAAAAXqRTeTh6QYcpZE1sDWtXm1HmQRUNU0IcBBBYAFMeKRXJTVYKNVlgHTdUmDV/LaYUwIgYDFZrAGqDVh1TEtNi300ntHt/PCzYrT2tVEGcjooWPhRYYSFzWUDEAAIABAACAAAAAgAEAAAAAAAAAAAEBIICEHgAAAAAAF6kUyPLL+cphRyyI5GTUazV0hF2R2NWHAQcXFgAUX4BmVeWSTJIEwtUb5TlPS/ntohABCGsCRzBEAiBnu3tA3yWlT0WBClsXXS9j69Bt+waCs9JcjWtNjtv7VgIge2VYAaBeLPDB6HGFlpqOENXMldsJezF9Gs5amvDQRDQBIQJl1jz1tBt8hNx2owTm+4Du4isx0pmdKNMNIjjaMHFfrQABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUIgICygvBWB5prpfx61y1HDAwo37kYP3YRJBvAjtunBAur3wYSFzWUDEAAIABAACAAAAAgAEAAAABAAAAAAA=";

payjoin/src/receive/error.rs

@@ -265,13 +265,15 @@ impl std::error::Error for PayloadError {
 ///
 /// This is currently opaque type because we aren't sure which variants will stay.
 /// You can only display it.
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub struct OutputSubstitutionError(InternalOutputSubstitutionError);
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub(crate) enum InternalOutputSubstitutionError {
-    /// Output substitution is disabled
-    OutputSubstitutionDisabled(&'static str),
+    /// Output substitution is disabled and output value was decreased
+    DecreasedValueWhenDisabled,
+    /// Output substitution is disabled and script pubkey was changed
+    ScriptPubKeyChangedWhenDisabled,
     /// Current output substitution implementation doesn't support reducing the number of outputs
     NotEnoughOutputs,
     /// The provided drain script could not be identified in the provided replacement outputs
@@ -281,7 +283,8 @@ pub(crate) enum InternalOutputSubstitutionError {
 impl fmt::Display for OutputSubstitutionError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self.0 {
-            InternalOutputSubstitutionError::OutputSubstitutionDisabled(reason) => write!(f, "{}", &format!("Output substitution is disabled: {}", reason)),
+            InternalOutputSubstitutionError::DecreasedValueWhenDisabled => write!(f, "Decreasing the receiver output value is not allowed when output substitution is disabled"),
+            InternalOutputSubstitutionError::ScriptPubKeyChangedWhenDisabled => write!(f, "Changing the receiver output script pubkey is not allowed when output substitution is disabled"),
             InternalOutputSubstitutionError::NotEnoughOutputs => write!(
                 f,
                 "Current output substitution implementation doesn't support reducing the number of outputs"
@@ -299,7 +302,8 @@ impl From<InternalOutputSubstitutionError> for OutputSubstitutionError {
 impl std::error::Error for OutputSubstitutionError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self.0 {
-            InternalOutputSubstitutionError::OutputSubstitutionDisabled(_) => None,
+            InternalOutputSubstitutionError::DecreasedValueWhenDisabled => None,
+            InternalOutputSubstitutionError::ScriptPubKeyChangedWhenDisabled => None,
             InternalOutputSubstitutionError::NotEnoughOutputs => None,
             InternalOutputSubstitutionError::InvalidDrainScript => None,
         }

payjoin/src/receive/v1/exclusive/mod.rs

@@ -71,7 +71,7 @@ fn parse_body(
 #[cfg(test)]
 mod tests {
     use bitcoin::{Address, AddressType};
-    use payjoin_test_utils::ORIGINAL_PSBT;
+    use payjoin_test_utils::{ORIGINAL_PSBT, QUERY_PARAMS};
 
     use super::*;
     struct MockHeaders {
@@ -96,7 +96,7 @@ mod tests {
     fn test_from_request() -> Result<(), Box<dyn std::error::Error>> {
         let body = ORIGINAL_PSBT.as_bytes();
         let headers = MockHeaders::new(body.len() as u64);
-        let proposal = UncheckedProposal::from_request(body, super::test::QUERY_PARAMS, headers)?;
+        let proposal = UncheckedProposal::from_request(body, QUERY_PARAMS, headers)?;
 
         let witness_utxo =
             proposal.psbt.inputs[0].witness_utxo.as_ref().expect("witness_utxo should be present");

payjoin/src/receive/v1/mod.rs

@@ -310,10 +310,7 @@ impl WantsOutputs {
                             && txo.value < original_output.value
                         {
                             return Err(
-                                InternalOutputSubstitutionError::OutputSubstitutionDisabled(
-                                    "Decreasing the receiver output value is not allowed",
-                                )
-                                .into(),
+                                InternalOutputSubstitutionError::DecreasedValueWhenDisabled.into(),
                             );
                         }
                         outputs.push(txo);
@@ -322,10 +319,8 @@ impl WantsOutputs {
                     None => {
                         if self.params.disable_output_substitution {
                             return Err(
-                                InternalOutputSubstitutionError::OutputSubstitutionDisabled(
-                                    "Changing the receiver output script pubkey is not allowed",
-                                )
-                                .into(),
+                                InternalOutputSubstitutionError::ScriptPubKeyChangedWhenDisabled
+                                    .into(),
                             );
                         }
                         let index = rng.gen_range(0..replacement_outputs.len());
@@ -788,20 +783,36 @@ pub(crate) mod test {
     use std::str::FromStr;
 
     use bitcoin::{Address, Network};
-    use payjoin_test_utils::ORIGINAL_PSBT;
+    use payjoin_test_utils::{BoxError, ORIGINAL_PSBT, QUERY_PARAMS};
     use rand::rngs::StdRng;
     use rand::SeedableRng;
 
     use super::*;
-    pub const QUERY_PARAMS: &str = "maxadditionalfeecontribution=182&additionalfeeoutputindex=0";
-
-    pub(crate) fn proposal_from_test_vector(
-    ) -> Result<UncheckedProposal, Box<dyn std::error::Error>> {
+    pub(crate) fn proposal_from_test_vector() -> Result<UncheckedProposal, BoxError> {
         let pairs = url::form_urlencoded::parse(QUERY_PARAMS.as_bytes());
         let params = Params::from_query_pairs(pairs, &[1])?;
         Ok(UncheckedProposal { psbt: bitcoin::Psbt::from_str(ORIGINAL_PSBT)?, params })
     }
 
+    fn wants_outputs_from_test_vector(
+        proposal: UncheckedProposal,
+    ) -> Result<WantsOutputs, BoxError> {
+        Ok(proposal
+            .assume_interactive_receiver()
+            .check_inputs_not_owned(|_| Ok(false))
+            .expect("No inputs should be owned")
+            .check_no_inputs_seen_before(|_| Ok(false))
+            .expect("No inputs should be seen before")
+            .identify_receiver_outputs(|script| {
+                let network = Network::Bitcoin;
+                Ok(Address::from_script(script, network).unwrap()
+                    == Address::from_str("3CZZi7aWFugaCdUCS15dgrUUViupmB8bVM")
+                        .unwrap()
+                        .require_network(network)
+                        .unwrap())
+            })?)
+    }
+
     #[test]
     fn can_get_proposal_from_request() {
         let proposal = proposal_from_test_vector();
@@ -935,6 +946,75 @@ pub(crate) mod test {
         );
     }
 
+    #[test]
+    fn test_pjos_disabled() {
+        let mut proposal = proposal_from_test_vector().unwrap();
+        // Specify outputsubstitution is disabled
+        proposal.params.disable_output_substitution = true;
+        let wants_outputs = wants_outputs_from_test_vector(proposal).unwrap();
+
+        let output_value =
+            wants_outputs.original_psbt.unsigned_tx.output[wants_outputs.change_vout].value
+                + Amount::ONE_SAT;
+        let outputs = vec![TxOut {
+            value: output_value,
+            script_pubkey: wants_outputs.original_psbt.unsigned_tx.output
+                [wants_outputs.change_vout]
+                .script_pubkey
+                .clone(),
+        }];
+        let increased_amount = wants_outputs.clone().replace_receiver_outputs(
+            outputs,
+            wants_outputs.original_psbt.unsigned_tx.output[wants_outputs.change_vout]
+                .script_pubkey
+                .as_script(),
+        );
+        assert!(increased_amount.is_ok(), "Replacement Outputs should be a valid WantsOutput");
+        assert_ne!(wants_outputs.payjoin_psbt, increased_amount.unwrap().payjoin_psbt);
+
+        let output_value =
+            wants_outputs.original_psbt.unsigned_tx.output[wants_outputs.change_vout].value
+                - Amount::ONE_SAT;
+        let outputs = vec![TxOut {
+            value: output_value,
+            script_pubkey: wants_outputs.original_psbt.unsigned_tx.output
+                [wants_outputs.change_vout]
+                .script_pubkey
+                .clone(),
+        }];
+        let decreased_amount = wants_outputs.clone().replace_receiver_outputs(
+            outputs,
+            wants_outputs.original_psbt.unsigned_tx.output[wants_outputs.change_vout]
+                .script_pubkey
+                .as_script(),
+        );
+        match decreased_amount {
+            Ok(_) => panic!("Expected error, got success"),
+            Err(error) => {
+                assert_eq!(
+                    error,
+                    OutputSubstitutionError::from(
+                        InternalOutputSubstitutionError::DecreasedValueWhenDisabled
+                    )
+                );
+            }
+        };
+
+        let script = Script::new();
+        let replace_receiver_script_pubkey = wants_outputs.substitute_receiver_script(script);
+        match replace_receiver_script_pubkey {
+            Ok(_) => panic!("Expected error, got success"),
+            Err(error) => {
+                assert_eq!(
+                    error,
+                    OutputSubstitutionError::from(
+                        InternalOutputSubstitutionError::ScriptPubKeyChangedWhenDisabled
+                    )
+                );
+            }
+        };
+    }
+
     #[test]
     fn test_interleave_shuffle() {
         let mut original1 = vec![1, 2, 3];