From e67d91a59901d6ca55c4a951cbc7fd5f1c51c49b Mon Sep 17 00:00:00 2001 From: Lucas Soriano del Pino Date: Fri, 8 Mar 2024 11:43:24 +0100 Subject: [PATCH 1/3] refactor(ln-dlc-node): Move estimate_fee method to wallet module --- crates/ln-dlc-node/src/node/mod.rs | 14 -------------- crates/ln-dlc-node/src/node/wallet.rs | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/ln-dlc-node/src/node/mod.rs b/crates/ln-dlc-node/src/node/mod.rs index 8117e92de..726323b34 100644 --- a/crates/ln-dlc-node/src/node/mod.rs +++ b/crates/ln-dlc-node/src/node/mod.rs @@ -29,7 +29,6 @@ use bitcoin::address::NetworkUnchecked; use bitcoin::secp256k1::PublicKey; use bitcoin::secp256k1::XOnlyPublicKey; use bitcoin::Address; -use bitcoin::Amount; use bitcoin::Network; use bitcoin::Txid; use dlc_messages::message_handler::MessageHandler as DlcMessageHandler; @@ -538,19 +537,6 @@ impl, - amount_sats: u64, - fee: ConfirmationTarget, - ) -> Result { - let address = address.require_network(self.network)?; - - self.wallet.estimate_fee(&address, amount_sats, fee) - } - /// Send the given `amount_sats` sats to the given unchecked, on-chain `address`. pub async fn send_to_address( &self, diff --git a/crates/ln-dlc-node/src/node/wallet.rs b/crates/ln-dlc-node/src/node/wallet.rs index b937ad047..e70d497b3 100644 --- a/crates/ln-dlc-node/src/node/wallet.rs +++ b/crates/ln-dlc-node/src/node/wallet.rs @@ -8,11 +8,14 @@ use crate::storage::TenTenOneStorage; use anyhow::Context; use anyhow::Result; use bdk_esplora::EsploraAsyncExt; +use bitcoin::address::NetworkUnchecked; use bitcoin::secp256k1::SecretKey; use bitcoin::Address; +use bitcoin::Amount; use bitcoin::OutPoint; use bitcoin::ScriptBuf; use bitcoin::TxOut; +use lightning::chain::chaininterface::ConfirmationTarget; use std::sync::Arc; use tokio::task::spawn_blocking; @@ -60,6 +63,19 @@ impl Nod self.wallet.is_mine(script_pubkey) } + /// Estimate the fee for sending the given `amount_sats` to the given `address` on-chain with + /// the given `fee`. + pub fn estimate_fee( + &self, + address: Address, + amount_sats: u64, + fee: ConfirmationTarget, + ) -> Result { + let address = address.require_network(self.network)?; + + self.wallet.estimate_fee(&address, amount_sats, fee) + } + /// Sync the state of the on-chain wallet against the blockchain. pub async fn sync_on_chain_wallet(&self) -> Result<()> { let client = &self.blockchain.esplora_client_async; From 9a10d5da6d151b5bc07d649a7b4b2a46daa1de9c Mon Sep 17 00:00:00 2001 From: Lucas Soriano del Pino Date: Fri, 8 Mar 2024 12:24:32 +0100 Subject: [PATCH 2/3] chore: Do not check address network when estimating fee It's completely unnecessary because a fee estimate is purely informational. --- crates/ln-dlc-node/src/node/wallet.rs | 5 +---- mobile/native/src/ln_dlc/mod.rs | 4 +++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/ln-dlc-node/src/node/wallet.rs b/crates/ln-dlc-node/src/node/wallet.rs index e70d497b3..1a4e1a042 100644 --- a/crates/ln-dlc-node/src/node/wallet.rs +++ b/crates/ln-dlc-node/src/node/wallet.rs @@ -8,7 +8,6 @@ use crate::storage::TenTenOneStorage; use anyhow::Context; use anyhow::Result; use bdk_esplora::EsploraAsyncExt; -use bitcoin::address::NetworkUnchecked; use bitcoin::secp256k1::SecretKey; use bitcoin::Address; use bitcoin::Amount; @@ -67,12 +66,10 @@ impl Nod /// the given `fee`. pub fn estimate_fee( &self, - address: Address, + address: Address, amount_sats: u64, fee: ConfirmationTarget, ) -> Result { - let address = address.require_network(self.network)?; - self.wallet.estimate_fee(&address, amount_sats, fee) } diff --git a/mobile/native/src/ln_dlc/mod.rs b/mobile/native/src/ln_dlc/mod.rs index a17d0814c..a0d940ca4 100644 --- a/mobile/native/src/ln_dlc/mod.rs +++ b/mobile/native/src/ln_dlc/mod.rs @@ -980,7 +980,9 @@ pub fn estimated_funding_tx_fee() -> Result { } pub async fn estimate_payment_fee(amount: u64, address: &str, fee: Fee) -> Result { - let address = address.parse()?; + let address: Address = address.parse().context("Failed to parse address")?; + // This is safe to do because we are only using this address to estimate a fee. + let address = address.assume_checked(); let fee = match fee { Fee::Priority(target) => state::get_node() From 46fea4da0446fe27ffd65643d75d9df230e856c6 Mon Sep 17 00:00:00 2001 From: Lucas Soriano del Pino Date: Fri, 8 Mar 2024 12:31:04 +0100 Subject: [PATCH 3/3] fix(app): Don't error when user is typing send amount When the user is about to send on-chain and is typing the send amount, we dynamically calculate a fee estimate so that we can update the screen as they update the value. This is nice, but the user will inevitably start by typing in small amounts for which calculating a fee does not make economical sense. We enhance the library code to handle this case explicitly by using a error enum variant `EstimateFeeError::SendAmountBelowDust`. We can then leverage this in the app backend to simply ignore this kind of error. --- Cargo.lock | 1 + crates/ln-dlc-node/Cargo.toml | 1 + crates/ln-dlc-node/src/lib.rs | 1 + crates/ln-dlc-node/src/node/wallet.rs | 3 ++- crates/ln-dlc-node/src/on_chain_wallet.rs | 15 ++++++++++++++- mobile/native/src/api.rs | 6 +++++- mobile/native/src/ln_dlc/mod.rs | 23 ++++++++++++++++------- 7 files changed, 40 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4791d4577..180907bad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2378,6 +2378,7 @@ dependencies = [ "serde", "serde_with 3.2.0", "sha2", + "thiserror", "time", "tokio", "tokio-tungstenite-wasm", diff --git a/crates/ln-dlc-node/Cargo.toml b/crates/ln-dlc-node/Cargo.toml index ce752787e..e9ca08359 100644 --- a/crates/ln-dlc-node/Cargo.toml +++ b/crates/ln-dlc-node/Cargo.toml @@ -42,6 +42,7 @@ secp256k1-zkp = { version = "0.7.0", features = ["global-context"] } serde = "1.0.147" serde_with = "3.1.0" sha2 = "0.10" +thiserror = "1" time = "0.3" tokio = { version = "1", default-features = false, features = ["io-util", "macros", "rt", "rt-multi-thread", "sync", "time", "tracing"] } tracing = "0.1.37" diff --git a/crates/ln-dlc-node/src/lib.rs b/crates/ln-dlc-node/src/lib.rs index af8b1a6db..1412acb00 100644 --- a/crates/ln-dlc-node/src/lib.rs +++ b/crates/ln-dlc-node/src/lib.rs @@ -46,6 +46,7 @@ pub use ln::DlcChannelDetails; pub use ln::EventHandlerTrait; pub use ln::EventSender; pub use on_chain_wallet::ConfirmationStatus; +pub use on_chain_wallet::EstimateFeeError; pub use on_chain_wallet::TransactionDetails; #[cfg(test)] diff --git a/crates/ln-dlc-node/src/node/wallet.rs b/crates/ln-dlc-node/src/node/wallet.rs index 1a4e1a042..399ea6557 100644 --- a/crates/ln-dlc-node/src/node/wallet.rs +++ b/crates/ln-dlc-node/src/node/wallet.rs @@ -1,6 +1,7 @@ use crate::bitcoin_conversion::to_secp_sk_30; use crate::node::Node; use crate::node::Storage; +use crate::on_chain_wallet; use crate::on_chain_wallet::BdkStorage; use crate::on_chain_wallet::OnChainWallet; use crate::on_chain_wallet::TransactionDetails; @@ -69,7 +70,7 @@ impl Nod address: Address, amount_sats: u64, fee: ConfirmationTarget, - ) -> Result { + ) -> Result { self.wallet.estimate_fee(&address, amount_sats, fee) } diff --git a/crates/ln-dlc-node/src/on_chain_wallet.rs b/crates/ln-dlc-node/src/on_chain_wallet.rs index f41794209..056e71f59 100644 --- a/crates/ln-dlc-node/src/on_chain_wallet.rs +++ b/crates/ln-dlc-node/src/on_chain_wallet.rs @@ -13,6 +13,7 @@ use bdk::chain::Append; use bdk::chain::ChainPosition; use bdk::chain::PersistBackend; use bdk::psbt::PsbtUtils; +use bdk::wallet::IsDust; use bdk::KeychainKind; use bdk::LocalOutput; use bdk::SignOptions; @@ -377,7 +378,11 @@ where recipient: &Address, amount_sat_or_drain: u64, confirmation_target: ConfirmationTarget, - ) -> Result { + ) -> Result { + if amount_sat_or_drain.is_dust(&recipient.script_pubkey()) { + return Err(EstimateFeeError::SendAmountBelowDust); + } + let psbt = self.build_psbt( recipient, amount_sat_or_drain, @@ -449,6 +454,14 @@ impl ConfirmationStatus { } } +#[derive(thiserror::Error, Debug)] +pub enum EstimateFeeError { + #[error("Cannot estimate fee for output below dust")] + SendAmountBelowDust, + #[error(transparent)] + Other(#[from] anyhow::Error), +} + pub trait BdkStorage: PersistBackend + Send + Sync + 'static {} #[derive(Default)] diff --git a/mobile/native/src/api.rs b/mobile/native/src/api.rs index e09058e56..4b614bb27 100644 --- a/mobile/native/src/api.rs +++ b/mobile/native/src/api.rs @@ -674,7 +674,11 @@ pub fn calculate_all_fees_for_on_chain(address: String, amount: u64) -> Result fee, + None => Amount::ZERO, + }; fees.push(FeeEstimation { sats_per_vbyte: fee_rate.ceil() as u64, diff --git a/mobile/native/src/ln_dlc/mod.rs b/mobile/native/src/ln_dlc/mod.rs index a0d940ca4..7a749b6be 100644 --- a/mobile/native/src/ln_dlc/mod.rs +++ b/mobile/native/src/ln_dlc/mod.rs @@ -979,20 +979,29 @@ pub fn estimated_funding_tx_fee() -> Result { Ok(fee) } -pub async fn estimate_payment_fee(amount: u64, address: &str, fee: Fee) -> Result { +pub async fn estimate_payment_fee(amount: u64, address: &str, fee: Fee) -> Result> { let address: Address = address.parse().context("Failed to parse address")?; // This is safe to do because we are only using this address to estimate a fee. let address = address.assume_checked(); let fee = match fee { - Fee::Priority(target) => state::get_node() - .inner - .estimate_fee(address, amount, target.into())? - .to_sat(), - Fee::FeeRate { sats } => sats, + Fee::Priority(target) => { + match state::get_node() + .inner + .estimate_fee(address, amount, target.into()) + { + Ok(fee) => Some(fee), + // It's not sensible to calculate the fee for an amount below dust. + Err(ln_dlc_node::EstimateFeeError::SendAmountBelowDust) => None, + Err(e) => { + bail!("Failed to estimate payment fee: {e:#}") + } + } + } + Fee::FeeRate { sats } => Some(Amount::from_sat(sats)), }; - Ok(Amount::from_sat(fee)) + Ok(fee) } pub async fn trade(