Skip to content

Commit

Permalink
Use ImplementationError alias for closures
Browse files Browse the repository at this point in the history 
It's more semantically correct to show that closures return a Boxed error
than expecting a specific variant of ReplyableError
  • Loading branch information
@DanGould
DanGould committed Feb 7, 2025
1 parent 8b919d3 commit 1358c50
Show file tree
Hide file tree
Showing 7 changed files with 88 additions and 75 deletions.
37 changes: 20 additions & 17 deletions payjoin-cli/src/app/v1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use hyper_util::rt::TokioIo;
use payjoin::bitcoin::psbt::Psbt;
use payjoin::bitcoin::{self, FeeRate};
use payjoin::receive::v1::{PayjoinProposal, UncheckedProposal};
use payjoin::receive::ImplementationError;
use payjoin::receive::ReplyableError::{self, Implementation, V1};
use payjoin::send::v1::SenderBuilder;
use payjoin::{Uri, UriExt};
Expand Down Expand Up @@ -303,17 +304,19 @@ impl App {
let network = bitcoind.get_blockchain_info().map_err(|e| Implementation(e.into()))?.chain;

// Receive Check 1: Can Broadcast
let proposal = proposal.check_broadcast_suitability(None, |tx| {
let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx);
let mempool_results =
bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Implementation(e.into()))?;
match mempool_results.first() {
Some(result) => Ok(result.allowed),
None => Err(Implementation(
anyhow!("No mempool results returned on broadcast check").into(),
)),
}
})?;
let proposal =
proposal.check_broadcast_suitability(None, |tx| {
let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx);
let mempool_results = bitcoind
.test_mempool_accept(&[raw_tx])
.map_err(|e| Implementation(e.into()))?;
match mempool_results.first() {
Some(result) => Ok(result.allowed),
None => Err(ImplementationError::from(
"No mempool results returned on broadcast check",
)),
}
})?;
log::trace!("check1");

// Receive Check 2: receiver can't sign for proposal inputs
Expand All @@ -322,7 +325,7 @@ impl App {
bitcoind
.get_address_info(&address)
.map(|info| info.is_mine.unwrap_or(false))
.map_err(|e| Implementation(e.into()))
.map_err(ImplementationError::from)
} else {
Ok(false)
}
Expand All @@ -331,7 +334,7 @@ impl App {

// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal.check_no_inputs_seen_before(|input| {
self.db.insert_input_seen_before(*input).map_err(|e| Implementation(e.into()))
self.db.insert_input_seen_before(*input).map_err(ImplementationError::from)
})?;
log::trace!("check3");

Expand All @@ -340,7 +343,7 @@ impl App {
bitcoind
.get_address_info(&address)
.map(|info| info.is_mine.unwrap_or(false))
.map_err(|e| Implementation(e.into()))
.map_err(ImplementationError::from)
} else {
Ok(false)
}
Expand All @@ -366,10 +369,10 @@ impl App {

let payjoin_proposal = provisional_payjoin.finalize_proposal(
|psbt: &Psbt| {
bitcoind
let res = bitcoind
.wallet_process_psbt(&psbt.to_string(), None, None, Some(false))
.map(|res| Psbt::from_str(&res.psbt).map_err(|e| Implementation(e.into())))
.map_err(|e| Implementation(e.into()))?
.map_err(ImplementationError::from)?;
Psbt::from_str(&res.psbt).map_err(ImplementationError::from)
},
None,
self.config.max_fee_rate,
Expand Down
45 changes: 24 additions & 21 deletions payjoin-cli/src/app/v2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ use payjoin::bitcoin::consensus::encode::serialize_hex;
use payjoin::bitcoin::psbt::Psbt;
use payjoin::bitcoin::{Amount, FeeRate};
use payjoin::receive::v2::{Receiver, UncheckedProposal};
use payjoin::receive::ReplyableError::Implementation;
use payjoin::receive::{Error, ReplyableError};
use payjoin::receive::{Error, ImplementationError, ReplyableError};
use payjoin::send::v2::{Sender, SenderBuilder};
use payjoin::{bitcoin, Uri};
use tokio::signal;
Expand Down Expand Up @@ -254,25 +253,29 @@ impl App {
&self,
proposal: payjoin::receive::v2::UncheckedProposal,
) -> Result<payjoin::receive::v2::PayjoinProposal, Error> {
let bitcoind = self.bitcoind().map_err(|e| Implementation(e.into()))?;
let bitcoind = self.bitcoind().map_err(|e| ReplyableError::Implementation(e.into()))?;

// in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx
let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast();

// The network is used for checks later
let network = bitcoind.get_blockchain_info().map_err(|e| Implementation(e.into()))?.chain;
let network = bitcoind
.get_blockchain_info()
.map_err(|e| ReplyableError::Implementation(e.into()))?
.chain;
// Receive Check 1: Can Broadcast
let proposal = proposal.check_broadcast_suitability(None, |tx| {
let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx);
let mempool_results =
bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Implementation(e.into()))?;
match mempool_results.first() {
Some(result) => Ok(result.allowed),
None => Err(Implementation(
anyhow!("No mempool results returned on broadcast check").into(),
)),
}
})?;
let proposal =
proposal.check_broadcast_suitability(None, |tx| {
let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx);
let mempool_results =
bitcoind.test_mempool_accept(&[raw_tx]).map_err(ImplementationError::from)?;
match mempool_results.first() {
Some(result) => Ok(result.allowed),
None => Err(ImplementationError::from(
"No mempool results returned on broadcast check",
)),
}
})?;
log::trace!("check1");

// Receive Check 2: receiver can't sign for proposal inputs
Expand All @@ -281,7 +284,7 @@ impl App {
bitcoind
.get_address_info(&address)
.map(|info| info.is_mine.unwrap_or(false))
.map_err(|e| Implementation(e.into()))
.map_err(ImplementationError::from)
} else {
Ok(false)
}
Expand All @@ -290,7 +293,7 @@ impl App {

// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal.check_no_inputs_seen_before(|input| {
self.db.insert_input_seen_before(*input).map_err(|e| Implementation(e.into()))
self.db.insert_input_seen_before(*input).map_err(ImplementationError::from)
})?;
log::trace!("check3");

Expand All @@ -300,7 +303,7 @@ impl App {
bitcoind
.get_address_info(&address)
.map(|info| info.is_mine.unwrap_or(false))
.map_err(|e| Implementation(e.into()))
.map_err(ImplementationError::from)
} else {
Ok(false)
}
Expand All @@ -315,10 +318,10 @@ impl App {

let payjoin_proposal = provisional_payjoin.finalize_proposal(
|psbt: &Psbt| {
bitcoind
let res = bitcoind
.wallet_process_psbt(&psbt.to_string(), None, None, Some(false))
.map(|res| Psbt::from_str(&res.psbt).map_err(|e| Implementation(e.into())))
.map_err(|e| Implementation(e.into()))?
.map_err(ImplementationError::from)?;
Psbt::from_str(&res.psbt).map_err(ImplementationError::from)
},
None,
self.config.max_fee_rate,
Expand Down
4 changes: 3 additions & 1 deletion payjoin/src/receive/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use crate::error_codes::{
NOT_ENOUGH_MONEY, ORIGINAL_PSBT_REJECTED, UNAVAILABLE, VERSION_UNSUPPORTED,
};

pub type ImplementationError = Box<dyn error::Error + Send + Sync>;

/// The top-level error type for the payjoin receiver
#[derive(Debug)]
#[non_exhaustive]
Expand Down Expand Up @@ -57,7 +59,7 @@ pub enum ReplyableError {
/// Error arising due to the specific receiver implementation
///
/// e.g. database errors, network failures, wallet errors
Implementation(Box<dyn error::Error + Send + Sync>),
Implementation(ImplementationError),
}

/// A trait for errors that can be serialized to JSON in a standardized format.
Expand Down
3 changes: 2 additions & 1 deletion payjoin/src/receive/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use std::str::FromStr;
use bitcoin::{psbt, AddressType, Psbt, TxIn, TxOut};
pub(crate) use error::InternalPayloadError;
pub use error::{
Error, JsonError, OutputSubstitutionError, PayloadError, ReplyableError, SelectionError,
Error, ImplementationError, JsonError, OutputSubstitutionError, PayloadError, ReplyableError,
SelectionError,
};
use optional_parameters::Params;

Expand Down
27 changes: 16 additions & 11 deletions payjoin/src/receive/v1/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ use super::error::{
InternalSelectionError,
};
use super::optional_parameters::Params;
use super::{InputPair, OutputSubstitutionError, ReplyableError, SelectionError};
use super::{
ImplementationError, InputPair, OutputSubstitutionError, ReplyableError, SelectionError,
};
use crate::psbt::PsbtExt;
use crate::receive::InternalPayloadError;

Expand Down Expand Up @@ -91,7 +93,7 @@ impl UncheckedProposal {
pub fn check_broadcast_suitability(
self,
min_fee_rate: Option<FeeRate>,
can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, ReplyableError>,
can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, ImplementationError>,
) -> Result<MaybeInputsOwned, ReplyableError> {
let original_psbt_fee_rate = self.psbt_fee_rate()?;
if let Some(min_fee_rate) = min_fee_rate {
Expand All @@ -103,7 +105,9 @@ impl UncheckedProposal {
.into());
}
}
if can_broadcast(&self.psbt.clone().extract_tx_unchecked_fee_rate())? {
if can_broadcast(&self.psbt.clone().extract_tx_unchecked_fee_rate())
.map_err(ReplyableError::Implementation)?
{
Ok(MaybeInputsOwned { psbt: self.psbt, params: self.params })
} else {
Err(InternalPayloadError::OriginalPsbtNotBroadcastable.into())
Expand Down Expand Up @@ -136,7 +140,7 @@ impl MaybeInputsOwned {
/// An attacker could try to spend receiver's own inputs. This check prevents that.
pub fn check_inputs_not_owned(
self,
is_owned: impl Fn(&Script) -> Result<bool, ReplyableError>,
is_owned: impl Fn(&Script) -> Result<bool, ImplementationError>,
) -> Result<MaybeInputsSeen, ReplyableError> {
let mut err: Result<(), ReplyableError> = Ok(());
if let Some(e) = self
Expand All @@ -152,7 +156,7 @@ impl MaybeInputsOwned {
.find_map(|script| match is_owned(&script) {
Ok(false) => None,
Ok(true) => Some(InternalPayloadError::InputOwned(script).into()),
Err(e) => Some(ReplyableError::Implementation(e.into())),
Err(e) => Some(ReplyableError::Implementation(e)),
})
{
return Err(e);
Expand All @@ -177,7 +181,7 @@ impl MaybeInputsSeen {
/// proposes a Payjoin PSBT as a new Original PSBT for a new Payjoin.
pub fn check_no_inputs_seen_before(
self,
is_known: impl Fn(&OutPoint) -> Result<bool, ReplyableError>,
is_known: impl Fn(&OutPoint) -> Result<bool, ImplementationError>,
) -> Result<OutputsUnknown, ReplyableError> {
self.psbt.input_pairs().try_for_each(|input| {
match is_known(&input.txin.previous_output) {
Expand All @@ -186,7 +190,7 @@ impl MaybeInputsSeen {
log::warn!("Request contains an input we've seen before: {}. Preventing possible probing attack.", input.txin.previous_output);
Err(InternalPayloadError::InputSeen(input.txin.previous_output))?
},
Err(e) => Err(ReplyableError::Implementation(e.into()))?,
Err(e) => Err(ReplyableError::Implementation(e))?,
}
})?;

Expand All @@ -208,7 +212,7 @@ impl OutputsUnknown {
/// Find which outputs belong to the receiver
pub fn identify_receiver_outputs(
self,
is_receiver_output: impl Fn(&Script) -> Result<bool, ReplyableError>,
is_receiver_output: impl Fn(&Script) -> Result<bool, ImplementationError>,
) -> Result<WantsOutputs, ReplyableError> {
let owned_vouts: Vec<usize> = self
.psbt
Expand All @@ -221,7 +225,8 @@ impl OutputsUnknown {
Ok(false) => None,
Err(e) => Some(Err(e)),
})
.collect::<Result<Vec<_>, _>>()?;
.collect::<Result<Vec<_>, _>>()
.map_err(ReplyableError::Implementation)?;

if owned_vouts.is_empty() {
return Err(InternalPayloadError::MissingPayment.into());
Expand Down Expand Up @@ -742,7 +747,7 @@ impl ProvisionalProposal {
/// wallet_process_psbt should sign and finalize receiver inputs
pub fn finalize_proposal(
mut self,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, ReplyableError>,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, ImplementationError>,
min_fee_rate: Option<FeeRate>,
max_effective_fee_rate: Option<FeeRate>,
) -> Result<PayjoinProposal, ReplyableError> {
Expand All @@ -754,7 +759,7 @@ impl ProvisionalProposal {
psbt.inputs[i].final_script_witness = None;
psbt.inputs[i].tap_key_sig = None;
}
let psbt = wallet_process_psbt(&psbt)?;
let psbt = wallet_process_psbt(&psbt).map_err(ReplyableError::Implementation)?;
let payjoin_proposal = self.prepare_psbt(psbt);
Ok(payjoin_proposal)
}
Expand Down
17 changes: 8 additions & 9 deletions payjoin/src/receive/v2/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use url::Url;

use super::error::{Error, InputContributionError};
use super::{
v1, InternalPayloadError, JsonError, OutputSubstitutionError, ReplyableError, SelectionError,
v1, ImplementationError, InternalPayloadError, JsonError, OutputSubstitutionError,
ReplyableError, SelectionError,
};
use crate::hpke::{decrypt_message_a, encrypt_message_b, HpkeKeyPair, HpkePublicKey};
use crate::ohttp::{ohttp_decapsulate, ohttp_encapsulate, OhttpEncapsulationError, OhttpKeys};
Expand Down Expand Up @@ -235,7 +236,7 @@ impl UncheckedProposal {
pub fn check_broadcast_suitability(
self,
min_fee_rate: Option<FeeRate>,
can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, ReplyableError>,
can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, ImplementationError>,
) -> Result<MaybeInputsOwned, ReplyableError> {
let inner = self.v1.check_broadcast_suitability(min_fee_rate, can_broadcast)?;
Ok(MaybeInputsOwned { v1: inner, context: self.context })
Expand Down Expand Up @@ -307,7 +308,7 @@ impl MaybeInputsOwned {
/// An attacker could try to spend receiver's own inputs. This check prevents that.
pub fn check_inputs_not_owned(
self,
is_owned: impl Fn(&Script) -> Result<bool, ReplyableError>,
is_owned: impl Fn(&Script) -> Result<bool, ImplementationError>,
) -> Result<MaybeInputsSeen, ReplyableError> {
let inner = self.v1.check_inputs_not_owned(is_owned)?;
Ok(MaybeInputsSeen { v1: inner, context: self.context })
Expand All @@ -329,7 +330,7 @@ impl MaybeInputsSeen {
/// proposes a Payjoin PSBT as a new Original PSBT for a new Payjoin.
pub fn check_no_inputs_seen_before(
self,
is_known: impl Fn(&OutPoint) -> Result<bool, ReplyableError>,
is_known: impl Fn(&OutPoint) -> Result<bool, ImplementationError>,
) -> Result<OutputsUnknown, ReplyableError> {
let inner = self.v1.check_no_inputs_seen_before(is_known)?;
Ok(OutputsUnknown { inner, context: self.context })
Expand All @@ -350,7 +351,7 @@ impl OutputsUnknown {
/// Find which outputs belong to the receiver
pub fn identify_receiver_outputs(
self,
is_receiver_output: impl Fn(&Script) -> Result<bool, ReplyableError>,
is_receiver_output: impl Fn(&Script) -> Result<bool, ImplementationError>,
) -> Result<WantsOutputs, ReplyableError> {
let inner = self.inner.identify_receiver_outputs(is_receiver_output)?;
Ok(WantsOutputs { v1: inner, context: self.context })
Expand Down Expand Up @@ -455,7 +456,7 @@ pub struct ProvisionalProposal {
impl ProvisionalProposal {
pub fn finalize_proposal(
self,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, ReplyableError>,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, ImplementationError>,
min_fee_rate: Option<FeeRate>,
max_effective_fee_rate: Option<FeeRate>,
) -> Result<PayjoinProposal, ReplyableError> {
Expand Down Expand Up @@ -607,9 +608,7 @@ mod test {

let server_error = proposal
.clone()
.check_broadcast_suitability(None, |_| {
Err(ReplyableError::Implementation("mock error".into()))
})
.check_broadcast_suitability(None, |_| Err("mock error".into()))
.err()
.unwrap();
assert_eq!(
Expand Down
Loading

0 comments on commit 1358c50

Please sign in to comment.