TxBuilder: Implement CIP-0002 Coin Selection #232
Conversation
https://github.com/cardano-foundation/CIPs/blob/master/CIP-0002/CIP-0002.md key differences 1) we take into account adjuting for free (which is out of CIP2's scope) 2) LargestFirst supports multiassets (naively)
| }, | ||
| CoinSelectionStrategyCIP2::RandomImprove => { | ||
| use rand::Rng; | ||
| if self.outputs.0.iter().any(|output| output.amount.multiasset.is_some()) { |
There was a problem hiding this comment.
CIP-0002 was written before multiassets and I think trying to handle mutliassets with random improve would be non-trivial. We can do it for largest first but with possible unintended consequences of huge TXs if the multiasset UTXO you need has a very low ADA value, causing this algorithm to stuff in many others.
Do we want to handle multiasset separately? Or as an extra stage at the end if there are any that just adds in inputs containing enough without much concern which should be fine as long as people aren't stuffing in many different multiassets into one tx and need to optimize the selection in a better way.
| @@ -222,7 +316,7 @@ impl TransactionBuilder { | |||
| } | |||
|
|
|||
| /// calculates how much the fee would increase if you added a given output | |||
| pub fn fee_for_input(&mut self, address: &Address, input: &TransactionInput, amount: &Value) -> Result<Coin, JsError> { | |||
There was a problem hiding this comment.
This wasn't strictly needed for this PR I think (add_inputs_from is mut), but this shouldn't have been mut in the first place.
| for utxo in available_inputs.0.iter() { | ||
| input_values.insert(utxo.input.transaction_id(), utxo.output.amount.clone()); | ||
| } | ||
| let mut encountered = std::collections::HashSet::new(); |
There was a problem hiding this comment.
due to the RNG I'm not sure what else we can test
|
Thank you, @rooooooooob, this is great. Resolve conflicts, please, when possible |
|
@vsubhuman I fixed the merge conflict. |
|
@BlakeBrown please see #264 which implements multiasset for both. The current (i.e. this PR) implementation for multiasset (largest-first) is very sub-optimal and will almost certainly add too many inputs in unless you have a lot of inputs with those desired multiassets. I should have thrown an error for the CIP-2 largest-first if there were multiassets too, and I do now in #264 if you're using the non-multiasset enum variants. Keep in mind CIP-2 only targets ADA-only since it was written before Cardano had multiassets. I tried to adapt both to multiasset in the linked PR, but I didn't before as it was additional work and how well they would work when applied (and even how you would adapt them) is/was unknown. |
https://github.com/cardano-foundation/CIPs/blob/master/CIP-0002/CIP-0002.md
key differences