RandomImprove : Test covering fees correctly #254
Conversation
…h funds to cover the fees
|
Fails with insufficient funds at the moment:
Waiting for #252 to get resolved to fix that. The conditions in the test are:
|
vsubhuman
requested review from
rooooooooob and
pedromtcosta
and removed request for
SebastienGllmt
* we expect 3 inputs in this case, which should give us the 264 fee
|
@vsubhuman I made the following changes:
|
rust/src/tx_builder.rs
Outdated
| // Phase 3: add extra inputs needed for fees | ||
| // We do this at the end because this new inputs won't be associated with | ||
| // a specific output, so the improvement algorithm we do above does not apply here. | ||
| if input_total.partial_cmp(&output_total).unwrap().eq(&Ordering::Less) { |
There was a problem hiding this comment.
- Why using
partial_cmphere? - Unchecked unwrap doesn't look good.
If these two types are both Value I think you can even just use <, e.g. see how while added < needed is used above
There was a problem hiding this comment.
Ah, that was a bad copy paste from my end 😅
Just pushed a version with a regular < comparison
|
@rooooooooob , check the solution plz if you have a chance |
There was a problem hiding this comment.
I guess this is a good enough fix to do it at the end. CIP 2 actually leaves out the fees entirely and says it's out of scope of the CIP so this detail is not covered which is why I tried to do it during the adding phase.
We're also trying to figure out what we want to do for multiassets since those also weren't covered but we can cover that at a later point.
|
/check |
|
Thank you, @pedromtcosta ! Looks good! |
Added test that consistently fails on random improve not adding enough funds to cover the fees