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. |
- Also hardcoded fee_rate and updated dependent fn in taker and server
| SendAmount::Max => total_input_value - fee, | ||
| let txout = TxOut { | ||
| script_pubkey: dest_addr.script_pubkey(), | ||
| value: Amount::ZERO, //Temporary value |
There was a problem hiding this comment.
This doesn't have to be 0. You can set it to the actual value needed here only.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah I see, you need this to handle the max spend case. Cool then it can stay.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
| if remaining > minimal_nondust { | ||
| tx.output[1].value = remaining; | ||
| } | ||
| log::info!( | ||
| "Adding change output with {} sats (fee: {})", | ||
| remaining, | ||
| fee_wchange | ||
| ); |
There was a problem hiding this comment.
There is one edge case to consider here. because feewchange will be higher than the previous calculated fee, if the remaining is close to dust limit, this updated remaining value may be below dust after recalculation. Which will make the added change below dust limit.
This can be handled by creating an hypothetical remianing_with_change value first, and then check if that is less than dust. Only add change out if true, else carry on with the non-change calculation.
| 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.
closes #309
The code doesn't compile and needs more work.
I'm drafting this PR to get a
ConceptACK.