Audit function signatures

payjoin/src/hpke.rs

@@ -1,7 +1,9 @@
 use std::ops::Deref;
 use std::{error, fmt};
 
-use bitcoin::key::constants::{ELLSWIFT_ENCODING_SIZE, UNCOMPRESSED_PUBLIC_KEY_SIZE};
+use bitcoin::key::constants::{
+    ELLSWIFT_ENCODING_SIZE, PUBLIC_KEY_SIZE, UNCOMPRESSED_PUBLIC_KEY_SIZE,
+};
 use bitcoin::secp256k1::ellswift::ElligatorSwift;
 use hpke::aead::ChaCha20Poly1305;
 use hpke::kdf::HkdfSha256;
@@ -97,7 +99,7 @@ impl<'de> serde::Deserialize<'de> for HpkeSecretKey {
 pub struct HpkePublicKey(pub PublicKey);
 
 impl HpkePublicKey {
-    pub fn to_compressed_bytes(&self) -> [u8; 33] {
+    pub fn to_compressed_bytes(&self) -> [u8; PUBLIC_KEY_SIZE] {
         let compressed_key = bitcoin::secp256k1::PublicKey::from_slice(&self.0.to_bytes())
             .expect("Invalid public key from known valid bytes");
         compressed_key.serialize()
@@ -148,7 +150,7 @@ impl<'de> serde::Deserialize<'de> for HpkePublicKey {
 /// Message A is sent from the sender to the receiver containing an Original PSBT payload
 #[cfg(feature = "send")]
 pub fn encrypt_message_a(
-    body: Vec<u8>,
+    body: Vec<u8>, // FIXME: could be &[u8]
     reply_pk: &HpkePublicKey,
     receiver_pk: &HpkePublicKey,
 ) -> Result<Vec<u8>, HpkeError> {
@@ -172,7 +174,7 @@ pub fn encrypt_message_a(
 #[cfg(feature = "receive")]
 pub fn decrypt_message_a(
     message_a: &[u8],
-    receiver_sk: HpkeSecretKey,
+    receiver_sk: HpkeSecretKey, // FIXME: could be &HpkeSecretKey
 ) -> Result<(Vec<u8>, HpkePublicKey), HpkeError> {
     use std::io::{Cursor, Read};
 
@@ -203,7 +205,7 @@ pub fn decrypt_message_a(
 /// Message B is sent from the receiver to the sender containing a Payjoin PSBT payload or an error
 #[cfg(feature = "receive")]
 pub fn encrypt_message_b(
-    mut plaintext: Vec<u8>,
+    mut plaintext: Vec<u8>, // FIXME: could be &[u8]
     receiver_keypair: &HpkeKeyPair,
     sender_pk: &HpkePublicKey,
 ) -> Result<Vec<u8>, HpkeError> {
@@ -227,8 +229,8 @@ pub fn encrypt_message_b(
 #[cfg(feature = "send")]
 pub fn decrypt_message_b(
     message_b: &[u8],
-    receiver_pk: HpkePublicKey,
-    sender_sk: HpkeSecretKey,
+    receiver_pk: HpkePublicKey, // FIXME: could be &HpkePublicKey
+    sender_sk: HpkeSecretKey,   // FIXME: could be &HpkeSecretKey
 ) -> Result<Vec<u8>, HpkeError> {
     let enc = message_b.get(..ELLSWIFT_ENCODING_SIZE).ok_or(HpkeError::PayloadTooShort)?;
     let enc = encapped_key_from_ellswift_bytes(enc)?;
@@ -242,6 +244,8 @@ pub fn decrypt_message_b(
     Ok(plaintext)
 }
 
+// FIXME: could be &mut [u8; padded_length] and return &[u8; padded_length]
+// ACTYUALLY function should not exist at all
 fn pad_plaintext(msg: &mut Vec<u8>, padded_length: usize) -> Result<&[u8], HpkeError> {
     if msg.len() > padded_length {
         return Err(HpkeError::PayloadTooLarge { actual: msg.len(), max: padded_length });

payjoin/src/io.rs

@@ -9,7 +9,7 @@ use crate::{OhttpKeys, Url};
 ///
 /// * `payjoin_directory`: The payjoin directory from which to fetch the ohttp keys.  This
 ///   directory stores and forwards payjoin client payloads.
-///
+/// // FIXME split into two functions so that the docstring can be right
 /// * `cert_der` (optional): The DER-encoded certificate to use for local HTTPS connections.  This
 ///   parameter is only available when the "danger-local-https" feature is enabled.
 #[cfg(feature = "v2")]

payjoin/src/ohttp.rs

@@ -4,7 +4,9 @@ use std::{error, fmt};
 use bitcoin::base64::prelude::BASE64_URL_SAFE_NO_PAD;
 use bitcoin::base64::Engine;
 
-pub fn ohttp_encapsulate(
+pub const PADDED_MESSAGE_BYTES: usize = 8192;
+
+pub(crate) fn ohttp_encapsulate(
     ohttp_keys: &mut ohttp::KeyConfig,
     method: &str,
     target_resource: &str,
@@ -38,7 +40,7 @@ pub fn ohttp_encapsulate(
 }
 
 /// decapsulate ohttp, bhttp response and return http response body and status code
-pub fn ohttp_decapsulate(
+pub(crate) fn ohttp_decapsulate(
     res_ctx: ohttp::ClientResponse,
     ohttp_body: &[u8],
 ) -> Result<http::Response<Vec<u8>>, OhttpEncapsulationError> {
@@ -57,7 +59,7 @@ pub fn ohttp_decapsulate(
 
 /// Error from de/encapsulating an Oblivious HTTP request or response.
 #[derive(Debug)]
-pub enum OhttpEncapsulationError {
+pub(crate) enum OhttpEncapsulationError {
     Http(http::Error),
     Ohttp(ohttp::Error),
     Bhttp(bhttp::Error),

payjoin/src/psbt.rs

@@ -36,9 +36,9 @@ pub(crate) trait PsbtExt: Sized {
     fn proprietary_mut(&mut self) -> &mut BTreeMap<psbt::raw::ProprietaryKey, Vec<u8>>;
     fn unknown_mut(&mut self) -> &mut BTreeMap<psbt::raw::Key, Vec<u8>>;
     fn input_pairs(&self) -> Box<dyn Iterator<Item = InternalInputPair<'_>> + '_>;
-    // guarantees that length of psbt input matches that of unsigned_tx inputs and same
+    /// guarantees that length of psbt input matches that of unsigned_tx inputs and same
     /// thing for outputs.
-    fn validate(self) -> Result<Self, InconsistentPsbt>;
+    fn validate(self) -> Result<Self, InconsistentPsbt>; // FIXME a ValidPsbt wrapper makes more semantic sense
     fn validate_input_utxos(&self, treat_missing_as_error: bool) -> Result<(), PsbtInputsError>;
 }
 
@@ -70,6 +70,8 @@ impl PsbtExt for Psbt {
     }
 
     fn validate(self) -> Result<Self, InconsistentPsbt> {
+        // TODO try to simplify trait to handle all validation
+        // TODO use validate_input_utxos here
         let tx_ins = self.unsigned_tx.input.len();
         let psbt_ins = self.inputs.len();
         let tx_outs = self.unsigned_tx.output.len();
@@ -135,7 +137,7 @@ impl InternalInputPair<'_> {
 
     pub fn validate_utxo(
         &self,
-        treat_missing_as_error: bool,
+        treat_missing_as_error: bool, // FIXME never used! remove!
     ) -> Result<(), InternalPsbtInputError> {
         match (&self.psbtin.non_witness_utxo, &self.psbtin.witness_utxo) {
             (None, None) if treat_missing_as_error =>

payjoin/src/receive/mod.rs

@@ -109,7 +109,7 @@ pub struct UncheckedProposal {
 
 impl UncheckedProposal {
     pub fn from_request(
-        mut body: impl std::io::Read,
+        mut body: impl std::io::Read, // FIXME: could be &[u8] bc you need the whole thing
         query: &str,
         headers: impl Headers,
     ) -> Result<Self, RequestError> {
@@ -142,9 +142,7 @@ impl UncheckedProposal {
         let params = Params::from_query_pairs(pairs).map_err(InternalRequestError::SenderParams)?;
         log::debug!("Received request with params: {:?}", params);
 
-        // TODO check that params are valid for the request's Original PSBT
-
-        Ok(UncheckedProposal { psbt, params })
+        Ok(Self { psbt, params })
     }
 
     /// The Sender's Original PSBT transaction
@@ -354,7 +352,7 @@ impl WantsOutputs {
     pub fn substitute_receiver_script(
         self,
         output_script: &Script,
-    ) -> Result<WantsOutputs, OutputSubstitutionError> {
+    ) -> Result<Self, OutputSubstitutionError> {
         let output_value = self.original_psbt.unsigned_tx.output[self.change_vout].value;
         let outputs = vec![TxOut { value: output_value, script_pubkey: output_script.into() }];
         self.replace_receiver_outputs(outputs, output_script)
@@ -369,7 +367,7 @@ impl WantsOutputs {
         self,
         replacement_outputs: Vec<TxOut>,
         drain_script: &Script,
-    ) -> Result<WantsOutputs, OutputSubstitutionError> {
+    ) -> Result<Self, OutputSubstitutionError> {
         let mut payjoin_psbt = self.original_psbt.clone();
         let mut outputs = vec![];
         let mut replacement_outputs = replacement_outputs.clone();
@@ -427,7 +425,7 @@ impl WantsOutputs {
         // Update the payjoin PSBT outputs
         payjoin_psbt.outputs = vec![Default::default(); outputs.len()];
         payjoin_psbt.unsigned_tx.output = outputs;
-        Ok(WantsOutputs {
+        Ok(Self {
             original_psbt: self.original_psbt,
             payjoin_psbt,
             params: self.params,
@@ -494,7 +492,7 @@ impl WantsInputs {
     /// A simple consolidation is otherwise chosen if available.
     pub fn try_preserving_privacy(
         &self,
-        candidate_inputs: impl IntoIterator<Item = InputPair>,
+        candidate_inputs: impl IntoIterator<Item = InputPair>, // FIXME &InputPair & clone out
     ) -> Result<InputPair, SelectionError> {
         let mut candidate_inputs = candidate_inputs.into_iter().peekable();
         if candidate_inputs.peek().is_none() {
@@ -521,7 +519,7 @@ impl WantsInputs {
     /// https://eprint.iacr.org/2022/589.pdf
     fn avoid_uih(
         &self,
-        candidate_inputs: impl IntoIterator<Item = InputPair>,
+        candidate_inputs: impl IntoIterator<Item = InputPair>, // FIXME &InputPair & clone out
     ) -> Result<InputPair, SelectionError> {
         let min_original_out_sats = self
             .payjoin_psbt
@@ -559,7 +557,7 @@ impl WantsInputs {
 
     fn select_first_candidate(
         &self,
-        candidate_inputs: impl IntoIterator<Item = InputPair>,
+        candidate_inputs: impl IntoIterator<Item = InputPair>, // FIXME &InputPair & clone out
     ) -> Result<InputPair, SelectionError> {
         candidate_inputs.into_iter().next().ok_or(InternalSelectionError::NotFound.into())
     }
@@ -569,7 +567,7 @@ impl WantsInputs {
     pub fn contribute_inputs(
         self,
         inputs: impl IntoIterator<Item = InputPair>,
-    ) -> Result<WantsInputs, InputContributionError> {
+    ) -> Result<Self, InputContributionError> {
         let mut payjoin_psbt = self.payjoin_psbt.clone();
         // The payjoin proposal must not introduce mixed input sequence numbers
         let original_sequence = self
@@ -609,7 +607,7 @@ impl WantsInputs {
             return Err(InternalInputContributionError::ValueTooLow.into());
         }
 
-        Ok(WantsInputs {
+        Ok(Self {
             original_psbt: self.original_psbt,
             payjoin_psbt,
             params: self.params,
@@ -897,10 +895,6 @@ pub struct PayjoinProposal {
 }
 
 impl PayjoinProposal {
-    pub fn utxos_to_be_locked(&self) -> impl '_ + Iterator<Item = &bitcoin::OutPoint> {
-        self.payjoin_psbt.unsigned_tx.input.iter().map(|input| &input.previous_output)
-    }
-
     pub fn is_output_substitution_disabled(&self) -> bool {
         self.params.disable_output_substitution
     }

payjoin/src/receive/v2/mod.rs

@@ -113,7 +113,7 @@ impl Receiver {
     /// indicating no UncheckedProposal is available yet.
     pub fn process_res(
         &mut self,
-        mut body: impl std::io::Read,
+        mut body: impl std::io::Read, // FIXME: could be &[u8], not streamed
         context: ohttp::ClientResponse,
     ) -> Result<Option<UncheckedProposal>, Error> {
         let mut buf = Vec::new();
@@ -139,10 +139,12 @@ impl Receiver {
         ohttp_encapsulate(&mut self.context.ohttp_keys, "GET", fallback_target.as_str(), None)
     }
 
+    // FIXME: could be &str
     fn extract_proposal_from_v1(&mut self, response: String) -> Result<UncheckedProposal, Error> {
         Ok(self.unchecked_from_payload(response)?)
     }
 
+    // FIXME: could be &[u8] because we've got a known length
     fn extract_proposal_from_v2(&mut self, response: Vec<u8>) -> Result<UncheckedProposal, Error> {
         let (payload_bytes, e) = decrypt_message_a(&response, self.context.s.secret_key().clone())?;
         self.context.e = Some(e);
@@ -152,7 +154,7 @@ impl Receiver {
 
     fn unchecked_from_payload(
         &mut self,
-        payload: String,
+        payload: String, // FIXME: could be &str
     ) -> Result<UncheckedProposal, RequestError> {
         let (base64, padded_query) = payload.split_once('\n').unwrap_or_default();
         let query = padded_query.trim_matches('\0');
@@ -347,9 +349,9 @@ impl WantsOutputs {
     pub fn substitute_receiver_script(
         self,
         output_script: &Script,
-    ) -> Result<WantsOutputs, OutputSubstitutionError> {
+    ) -> Result<Self, OutputSubstitutionError> {
         let inner = self.inner.substitute_receiver_script(output_script)?;
-        Ok(WantsOutputs { inner, context: self.context })
+        Ok(Self { inner, context: self.context })
     }
 
     /// Replace **all** receiver outputs with one or more provided outputs.
@@ -359,11 +361,11 @@ impl WantsOutputs {
     /// receiver needs to pay for additional miner fees (e.g. in the case of adding many outputs).
     pub fn replace_receiver_outputs(
         self,
-        replacement_outputs: Vec<TxOut>,
+        replacement_outputs: Vec<TxOut>, // FIXME could be Iterator<Item = &TxOut>
         drain_script: &Script,
-    ) -> Result<WantsOutputs, OutputSubstitutionError> {
+    ) -> Result<Self, OutputSubstitutionError> {
         let inner = self.inner.replace_receiver_outputs(replacement_outputs, drain_script)?;
-        Ok(WantsOutputs { inner, context: self.context })
+        Ok(Self { inner, context: self.context })
     }
 
     /// Proceed to the input contribution step.
@@ -395,7 +397,7 @@ impl WantsInputs {
     /// https://eprint.iacr.org/2022/589.pdf
     pub fn try_preserving_privacy(
         &self,
-        candidate_inputs: impl IntoIterator<Item = InputPair>,
+        candidate_inputs: impl IntoIterator<Item = &InputPair>, // FIXME &InputPair & clone out
     ) -> Result<InputPair, SelectionError> {
         self.inner.try_preserving_privacy(candidate_inputs)
     }
@@ -404,7 +406,7 @@ impl WantsInputs {
     /// Any excess input amount is added to the change_vout output indicated previously.
     pub fn contribute_inputs(
         self,
-        inputs: impl IntoIterator<Item = InputPair>,
+        inputs: impl IntoIterator<Item = InputPair>, // Own and place in payjoin_psbt
     ) -> Result<WantsInputs, InputContributionError> {
         let inner = self.inner.contribute_inputs(inputs)?;
         Ok(WantsInputs { inner, context: self.context })
@@ -450,10 +452,6 @@ pub struct PayjoinProposal {
 }
 
 impl PayjoinProposal {
-    pub fn utxos_to_be_locked(&self) -> impl '_ + Iterator<Item = &bitcoin::OutPoint> {
-        self.inner.utxos_to_be_locked()
-    }
-
     pub fn is_output_substitution_disabled(&self) -> bool {
         self.inner.is_output_substitution_disabled()
     }
@@ -509,7 +507,7 @@ impl PayjoinProposal {
     /// choose to broadcast the original PSBT.
     pub fn process_res(
         &self,
-        res: Vec<u8>,
+        res: Vec<u8>, // FIXME could be &[u8]
         ohttp_context: ohttp::ClientResponse,
     ) -> Result<(), Error> {
         let res = ohttp_decapsulate(ohttp_context, &res)?;

payjoin/src/request.rs

@@ -27,10 +27,14 @@ pub struct Request {
 }
 
 impl Request {
+    // FIXME: could be &Url and clone inside to help caller
+    // FIXME: could be &[u8] and clone inside to help caller
     pub fn new_v1(url: Url, body: Vec<u8>) -> Self {
         Self { url, content_type: V1_REQ_CONTENT_TYPE, body }
     }
 
+    // FIXME: could be &Url and clone inside to help caller
+    // FIXME: could be &[u8] and clone inside to help caller
     #[cfg(feature = "v2")]
     pub fn new_v2(url: Url, body: Vec<u8>) -> Self {
         Self { url, content_type: V2_REQ_CONTENT_TYPE, body }