use feerate instead of fee while sending a transaction
#343
Conversation
|
@claddyy this is medium priority, and we wanna get this in 1.1 release. So if you can resume work on this we can conclude it as soon as possible. |
There was a problem hiding this comment.
Looking good, one more edge case to cover.
Also there are other spend APIs, where we need this, for ex: fidelity and contract timelock spending. Those fees are hardcoded too. A really good design would be to create one common spend API, and use that everywhere. Lets get that done in this PR too.
I would also like to see a IT covering the change address logic. Ensuring that uneconomical change below dust limits are not being created. But that can be done in a followup PR.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Also hardcoded fee_rate and updated dependent fn in taker and server
| @@ -218,8 +218,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. | |||
There was a problem hiding this comment.
Take the fee_rate as an input of the app. Replace fee with fee_rate, as f32. Default value 2.0 sats/vb.
| @@ -98,16 +98,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. | |||
There was a problem hiding this comment.
Same here. Fee rate will be an input via the maker-cli command.
| let mut total_witness_weight = 0; | ||
| for (_, spend_info) in &selected_utxo { | ||
| match spend_info { | ||
| UTXOSpendInfo::FidelityBondCoin { .. } => { | ||
| total_witness_weight += FIDELITY_BOND_WITNESS_SIZE; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| let base_size = tx.base_size(); | ||
| let vsize = (base_size * 4 + total_witness_weight) / 4; | ||
| let mut fee = Amount::from_sat((fee_rate * (vsize as f64)).ceil() as u64); |
There was a problem hiding this comment.
Instead of having this logic duplicated in all the spend functions, we can just use spend_coin function. A fidelity spend is nothing but a regular spend with only one input and one ouput. The same thing for timelock and haslock spend. We don't have an API for hashlock spend, so it's good time to add one (can be done in a followup).
Also as discussed the spend_coin API can be changed to simplify things. I am proposing the following:
- Make the
Destinationan enum like below.
enum Destination {
Sweep(Address),
Multi(Vec<(Address, Amount)>)
}Multi can be used for batch payments. Each entry will create a new TxOut. This will allow us to batch payouts, which will be very handy later on for utxo management.
Sweep will be used for sweeping type transaction and will behave the same as current SendAmount::Max. This can be used for all the internal spends like, fidelity, hashlock and timelock.
-
remove the
SendAmountall together. We don't need this tupple. -
We always add change address if the
sum(inputs) - sum(output) > dust_limit. Except for the sweep case.
Then this same spend_coin function can be used for all different cases.
|
I am taking it up! |
closes #309