Add multiplication operator to CFeeRate #29037
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
src/policy/feerate.h
Outdated
| @@ -71,6 +71,8 @@ class CFeeRate | |||
| friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; } | |||
| CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; } | |||
| std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const; | |||
| friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.nSatoshisPerK); } | |||
| friend CFeeRate operator*(int a, const CFeeRate& f) { return f * a; } | |||
There was a problem hiding this comment.
nit: To me it confused me for a moment cause I didn't realize that f was using the operator defined on the line above, I think it would make more sense for a reader to just see the same thing above defined backwards like so
friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(f.nSatoshisPerK * a); }
feel free to ignore this suggestion though
There was a problem hiding this comment.
Sure, I updated it to use the same formula in both lines, so that only the parameters appear in either order
murchandamus
force-pushed
the
2023-12-multiply-operator-CFeeRate
branch
from
7dacc17
to
1553c80
Compare
|
ACK 1553c80 |
|
tACK 1553c80 I compiled from source, ran the test suite, and added some additional BOOST_CHECK tests on my local machine that all passed |
|
Thanks both of you! @kashifs: If you open a PR against my PR or send me a patch by email, I’d be happy to add your commit with the additional tests to the PR. |
|
Hi @murchandamus, |
murchandamus
force-pushed
the
2023-12-multiply-operator-CFeeRate
branch
2 times, most recently
from
9a6dd54
to
1553c80
Compare
|
Thanks @kashifs, I’ve merged your pull request into my pull request and pushed it. |
|
Okay, great! I'm slowly working my way through #27877 |
| @@ -71,6 +71,8 @@ class CFeeRate | |||
| friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; } | |||
| CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; } | |||
| std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::BTC_KVB) const; | |||
| friend CFeeRate operator*(const CFeeRate& f, int a) { return CFeeRate(a * f.nSatoshisPerK); } | |||
| friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(a * f.nSatoshisPerK); } | |||
There was a problem hiding this comment.
Seems strange to have operator+= and operator* but no operator*= or operator+.
DrahtBot
requested review from
kevkevinpal
and removed request for
kevkevinpal
|
reACK 1757452 |
| @@ -87,10 +87,30 @@ BOOST_AUTO_TEST_CASE(GetFeeTest) | |||
| CFeeRate(MAX_MONEY, std::numeric_limits<uint32_t>::max()).GetFeePerK(); | |||
There was a problem hiding this comment.
nit: will be nice to squashed this 1757452 to the first commit
|
ACK 1757452 |
Allows us to use
coin_selection_params.m_long_term_feerate * 3or
3 * coin_selection_params.m_long_term_feerateinstead of
CFeeRate{coin_selection_params.m_long_term_feerate.GetFee(3000)}inspired by #27877 (comment)