Implement Sum for amount types#615
Conversation
0e2893f to
3643564
Compare
To be able to sum up iterators of amounts it is not sufficient that these implement `Add`, they also need to implement `Sum`.
It's just `Sum` with checked arithmetic.
3643564 to
6f7da5f
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
Code duplication may be completely avoided if a BitcoinAmount trait was introduced covering both amount types. This will also work well for external API so the functions may be generalized by the amount type.
|
@dr-orlovsky I did experiment with your idea a bit, I think it's definitely worth a separate PR. It's not as easy as I expected and we might consider it not worth it in the end. See ea7c185 for my attempt, still failing tests because I probably missed some nuances. |
9a90f3b to
e991b0c
Compare
e991b0c to
4dae569
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
I really like this API design!
| let sats: u64 = iter.map(|amt| amt.0).sum(); | ||
| Amount::from_sat(sats) |
There was a problem hiding this comment.
nit, maybe either use as_sat here, or use Amount(sats) instead of calling from_sat, so that it will be consistent in case from_sat or the internal representation ever changes
Or another option: iter.fold(Self::ZERO, ops::Add::add) (which is similar to how Sum is implemented in libstd)
@apoelstra, you ACKED an unrelated commit head here :P |
|
I really wish github would not accept ACKs for things that are not the current PR tip. In this case I'm sure it's fine. I will try to be more careful in the future. |
7eccfdb clippy: whitelist iter_kv lint since it is broken (Andrew Poelstra) 6de288f cargo fmt (Andrew Poelstra) f9c4e53 clippy: fix PartialOrd impl on an Ord type (Andrew Poelstra) 1f8dfe2 clippy: clean up iterator (Andrew Poelstra) 5ab7afe clippy: whitelist inapplicable lints (Andrew Poelstra) 2237906 clippy: use try_fold in place of fold in several places (Andrew Poelstra) 4f7782f clippy: minor things (Andrew Poelstra) a61f71a clippy: eliminate a bunch of redundant `into_iter` calls (Andrew Poelstra) Pull request description: Supercedes rust-bitcoin#615. ACKs for top commit: tcharding: ACK 7eccfdb sanket1729: ACK 7eccfdb Tree-SHA512: ea4f14f754b07c9ae127684e6ca1d9f121361a7be617d45a19af0269af735d2f420f59abdf15b943052b34cc9afba31cddc87d75a86694f8eefa0d672d4b031b
Currently iterators of amounts can't be summed. This PR fixes this and also adds a checked variant.