Feat: don't include dust btc amounts on rotation

[msgmaxim]

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have updated documentation where appropriate.

Summary

Noticed that we used "all or nothing" approach when sweeping utxos on rotation, when it is better to skip dust utxos to avoid paying more for their fees than they are worth. Another potential benefit of this is reducing the number of payloads we would need to sign on rotation. (Already discussed this with @ramizhasan111 to make sure that we want this change.)

[codecov]

Codecov Report

Merging #4063 (7a6fda7) into main (b3adda0) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##            main   #4063   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        376     376           
  Lines      59662   59672   +10     
  Branches   59662   59672   +10     
=====================================
+ Hits       42554   42563    +9     
- Misses     14961   14962    +1     
  Partials    2147    2147           

Files	Coverage Δ
state-chain/pallets/cf-environment/src/lib.rs	74% <100%> (+1%)	⬆️
state-chain/pallets/cf-environment/src/tests.rs	100% <100%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dandanlen]

We want to return None if this is empty, right?

Could we not just do the same as before except:

let available_utxos = BitcoinAvailableUtxos::<T>::take()
	.into_iter()
	.filter(|utxo| utxo.amount > fee_per_input_utxo)
	.collect();

// Same as before
(!available_utxos.is_empty()).then_some(available_utxos).and_then(
//etc.

[msgmaxim]

We want to return None if this is empty, right?

New code also returns None if the list is empty, and we even have a test for that. Is your suggestion purely due to style preference?

I'm not convinced that "no utxo" needs to be handled separately. Conceptually it is really not any different from "not enough utxo". If you want, I could inline total_fee and combine the first two expressions, but personally don't really mind giving names to these intermediate expressions.

[dandanlen]

Ok I see that now. I guess it's stylistic thing then.

I think I missed it because the new code reads to me like "Return None if the utxo sum minus the total fee saturates", which (indirectly, because we filter the list above) implies that the list is empty. This seems less obvious than simply "return None if there are no utxos above the dust amount".

I don't mind breaking this up in some way / adding intermediate expressions, but it would be easier to follow if we handle the empty case explicitly (if x.is_empty() or whatever, doesn't have to be !available_utxos.is_empty()).then_some( which I admit is not easy to read either).

[msgmaxim]

I would say the unnecessary check for is_empty() is the cause of the initial confusion. It made it seem like it was a case that needed a special treatment (different from the more general "not enough funds" case) when it doesn't.

Anyway, it doesn't make a big difference to me, so I will make the change you are suggesting before merging.

[dandanlen]

Ok I see that now. I guess it's stylistic thing then.

I think I missed it because the new code reads to me like "Return None if the utxo sum minus the total fee saturates", which (indirectly, because we filter the list above) implies that the list is empty. This seems less obvious than simply "return None if there are no utxos above the dust amount".

I don't mind breaking this up in some way / adding intermediate expressions, but it would be easier to follow if we handle the empty case explicitly (if x.is_empty() or whatever, doesn't have to be !available_utxos.is_empty()).then_some( which I admit is not easy to read either).