Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use feerate instead of fee while sending a transaction #343

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+106 −42
Open

use feerate instead of fee while sending a transaction #343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b1819d
update spend_from_wallet to use feerate instead of fee
claddyy Jan 18, 2025
1d94e2c
Update fee if we have change output
claddyy Jan 19, 2025
dfd67f8
Check calculated vsize against vsize via API
claddyy Jan 19, 2025
aec4eec
refractor: abstract spend_coins as an API
claddyy Jan 27, 2025
d0e8a5b
Handle edge case for dust limit while creating change o/p
claddyy Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/bin/taker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,10 @@ fn main() -> Result<(), TakerError> {
let destination =
Destination::Address(Address::from_str(&address).unwrap().assume_checked());

let fee_rate = 3.0; // sats/vByte, Written as a temporary fix until issue #199 is solved.

let tx = taker.get_wallet_mut().spend_from_wallet(
fee,
fee_rate,
SendAmount::Amount(amount),
destination,
&coins_to_spend,
Expand Down
7 changes: 3 additions & 4 deletions src/maker/rpc/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,15 @@ fn handle_request(maker: &Arc<Maker>, socket: &mut TcpStream) -> Result<(), Make

let coins_to_send = maker.get_wallet().read()?.coin_select(amount + fee)?;

let fee_rate = 3.0; // sats/vByte, Written as a temporary fix until issue #199 is solved.

let tx = maker.get_wallet().write()?.spend_from_wallet(
fee,
fee_rate,
SendAmount::Amount(amount),
destination,
&coins_to_send,
)?;

let calculated_fee_rate = fee / (tx.weight());
log::info!("Calculated FeeRate : {:#}", calculated_fee_rate);

let txid = maker.get_wallet().read()?.send_tx(&tx)?;

RpcMsgResp::SendToAddressResp(txid.to_string())
Expand Down
91 changes: 59 additions & 32 deletions src/wallet/direct_send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ use crate::wallet::api::UTXOSpendInfo;

use super::{error::WalletError, Wallet};

const P2PWPKH_WITNESS_SIZE: usize = 108;
const P2WSH_MULTISIG_2OF2_WITNESS_SIZE: usize = 222;
mojoX911 marked this conversation as resolved.
Show resolved Hide resolved

/// Represents options for specifying the amount to be sent in a transaction.
#[derive(Debug, Clone, PartialEq)]
pub enum SendAmount {
Expand Down Expand Up @@ -83,7 +86,7 @@ impl Wallet {
/// are held in a change address, if applicable.
pub fn spend_from_wallet(
&mut self,
fee: Amount,
fee_rate: f64,
send_amount: SendAmount,
destination: Destination,
coins_to_spend: &[(ListUnspentResultEntry, UTXOSpendInfo)],
Expand All @@ -102,38 +105,38 @@ impl Wallet {
};

let mut total_input_value = Amount::ZERO;
let mut total_witness_size = 0;
let mut valid_coins = Vec::new();

for (utxo_data, spend_info) in coins_to_spend {
// filter all contract and fidelity utxos.
if let UTXOSpendInfo::FidelityBondCoin { .. }
| UTXOSpendInfo::HashlockContract { .. }
| UTXOSpendInfo::TimelockContract { .. } = spend_info
{
log::warn!("Skipping Fidelity Bond or Contract UTXO.");
continue;
match spend_info {
UTXOSpendInfo::SeedCoin { .. } => {
total_witness_size += P2PWPKH_WITNESS_SIZE;
valid_coins.push((utxo_data, spend_info));
total_input_value += utxo_data.amount;
}
UTXOSpendInfo::SwapCoin { .. } => {
total_witness_size += P2WSH_MULTISIG_2OF2_WITNESS_SIZE;
valid_coins.push((utxo_data, spend_info));
total_input_value += utxo_data.amount;
}
UTXOSpendInfo::FidelityBondCoin { .. }
| UTXOSpendInfo::HashlockContract { .. }
| UTXOSpendInfo::TimelockContract { .. } => {
log::warn!("Skipping Fidelity Bond or Contract UTXO: {:?}", spend_info);
continue;
}
Comment on lines 131 to +148
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic can be moved to the call sites, or better, create wrapper APIs on top of a common send API. Weather a utxo is valid for spend should be determined at the call site.

For ex: if you wanna spend only fidelity bonds in API like spend_fildiety() you filter out the fidelity utxo, and then feed to the common spend_coins() API.

This will also allow us to use a common logic for all kind of spendings and use that everywhere, instead of current individual design of each spend functions. This refactor is much needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Partially addressed via aec4eec.
I'll try once to get the size of tx using the descriptor instead of current workaround(hardcoding witness sizes). Once that's done, the API would be lot cleaner.

}
}

for (utxo_data, _) in &valid_coins {
tx.input.push(TxIn {
previous_output: OutPoint::new(utxo_data.txid, utxo_data.vout),
sequence: Sequence::ZERO,
witness: Witness::new(),
script_sig: ScriptBuf::new(),
});

total_input_value += utxo_data.amount;
}

if let SendAmount::Amount(a) = send_amount {
if a + fee > total_input_value {
return Err(WalletError::InsufficientFund {
available: total_input_value.to_btc(),
required: (a + fee).to_btc(),
});
}
}

log::info!("Total Input Amount: {} | Fees: {}", total_input_value, fee);

let dest_addr = match destination {
Destination::Wallet => self.get_next_internal_addresses(1)?[0].clone(),
Destination::Address(a) => {
Expand All @@ -154,20 +157,44 @@ impl Wallet {
}
};

let txout = {
let value = match send_amount {
SendAmount::Max => total_input_value - fee,
let txout = TxOut {
script_pubkey: dest_addr.script_pubkey(),
value: Amount::ZERO, //Temporary value
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have to be 0. You can set it to the actual value needed here only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm validating send_amount against fee parameters later, that's why I kept it 0, and the value is added later. Does keeping it this way makes more sense, or shall I change it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change it, please. There is no ambiguity about the send amount. Just set the value here.

This approach makes sense for change amount where you don't know the value before hand.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, you need this to handle the max spend case. Cool then it can stay.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second thought. Lets remove this SendAmount::Max thing. It's not working as expected. It will only create a non-change spend from with the given inputs. That doesn't mean thats the max wallet amount. The current design is not ideal too. Specifying Max and selected coin list in the input together is redundant and circular. Makes it almost unusable for the sweeping purpose.

Instead there should a be dedicated API called seep_wallet(dest_addr), which doesn't need any other input from the user, finds the total spendable utxos, create one tx with no change to sweep the whole wallet balance to dest-addr. This can be done in a followup PR. For now we can simplify the spend logic much more if we remove the Max enum.

Copy link

@mojoX911 mojoX911 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One good way of doing it would be to have a SpendType enum.

pun enum SpendType {
    Amount(u64),
    NoChange
}

When NoChange is given, it will create a change less tx, sending the whole total_input - fee to the dest_addr. This will be useful for fidelity, and contract spendings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One good way of doing it would be to have a SpendType enum.

pun enum SpendType {
    Amount(u64),
    NoChange
}

When NoChange is given, it will create a change less tx, sending the whole total_input - fee to the dest_addr. This will be useful for fidelity, and contract spendings.

https://github.com/claddyy/coinswap/blob/d0e8a5b165d26a0e8a4c34a5b126f20814e51044/src/wallet/direct_send.rs#L201-L204

SpendType seems to be just same as the SendAmount enum. Currently when Max is given, the change less tx is created.

};

tx.output.push(txout);

SendAmount::Amount(a) => a,
};
log::info!("Sending {} to {}.", value, dest_addr);
TxOut {
script_pubkey: dest_addr.script_pubkey(),
value,
let base_size = tx.base_size();
let vsize = (base_size * 4 + total_witness_size) / 4;
claddyy marked this conversation as resolved.
Show resolved Hide resolved
let fee = Amount::from_sat((fee_rate * vsize as f64).ceil() as u64);
log::info!("Total Input Amount: {} | Fees: {}", total_input_value, fee);

if let SendAmount::Amount(a) = send_amount {
if a + fee > total_input_value {
return Err(WalletError::InsufficientFund {
available: total_input_value.to_btc(),
required: (a + fee).to_btc(),
});
}
}

//Validate and set amounts
if let SendAmount::Amount(a) = send_amount {
if a + fee > total_input_value {
return Err(WalletError::InsufficientFund {
available: total_input_value.to_btc(),
required: (a + fee).to_btc(),
});
}
}
claddyy marked this conversation as resolved.
Show resolved Hide resolved

let value = match send_amount {
SendAmount::Max => total_input_value - fee,
SendAmount::Amount(a) => a,
};

tx.output.push(txout);
tx.output[0].value = value;
log::info!("Sending {} to {}", value, dest_addr);

// Only include change if remaining > dust
if let SendAmount::Amount(amount) = send_amount {
mojoX911 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading