Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Sum for amount types #615

Merged
merged 3 commits into from Sep 10, 2021

Conversation

sgeisler
Copy link
Contributor

Currently iterators of amounts can't be summed. This PR fixes this and also adds a checked variant.

@sgeisler sgeisler force-pushed the 2021-06-sum-amounts branch 2 times, most recently from 0e2893f to 3643564 Compare June 12, 2021 15:21
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.
@sgeisler sgeisler marked this pull request as ready for review June 12, 2021 16:22
@sgeisler sgeisler added the minor API Change This PR should get a minor version bump label Jun 12, 2021
src/util/amount.rs Outdated Show resolved Hide resolved
dr-orlovsky
dr-orlovsky previously approved these changes Jun 16, 2021
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sgeisler
Copy link
Contributor Author

@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.

Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this API design!

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack 95aa3bf

Comment on lines +529 to +530
let sats: u64 = iter.map(|amt| amt.0).sum();
Amount::from_sat(sats)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4dae569

@sanket1729
Copy link
Member

ack 95aa3bf

@apoelstra, you ACKED an unrelated commit head here :P

@dr-orlovsky dr-orlovsky merged commit b2c8a7e into rust-bitcoin:master Sep 10, 2021
@apoelstra
Copy link
Member

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.

@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Sep 25, 2021
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor API Change This PR should get a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants