feat: add Amount division by FeeRate#3893
Conversation
Pull Request Test Coverage Report for Build 12886609170Details
💛 - Coveralls |
|
Mad, thanks for your contribution. High level ack from me except that we agreed over here that we should add two functions, one for floor and one for ceiling division (using Also
Thanks again! |
|
Also, perhaps we should add unit tests that test the truncation functionality. Good work on the docs, your description is better than my attempt. |
|
@tcharding Thanks for your feedback!
lmk if anything else needs adjustment |
There was a problem hiding this comment.
Mad! A few comments (I'm basing my review style on the fact that you've been active in bitcoin-dev-project so you're keen to learn).
As far as code logic goes it all looks correct to me. To get this into a state that I can ack it all we need is to squash the commits. In this repo we keep the PRs consisting of discreet patches by rebasing and force pushing to implement review suggestions. This is as opposed to the development style of creating new patches on top of old ones to implement review suggestions. Ask if that explanation is not clear. There is more than one way to do things but if I was doing this PR I would have a single patch that adds the new functions and unit tests, then a patch that runs the check api script. (If it helps I tend to use the command just check-api; ci -a -m 'api: Run just check-api')
If however you want to improve your Rust skills I would suggest attempting to refactor the functions because there are a bunch of lines of code that are unnecessary. (Hint: try thinking what happens in the checked ops when dividing by zero). Since you have pretty good test coverage already you can refactor wildly and just run the tests to see you didn't break anything.
(nit: There are a bunch of trailing whitespaces too. If you can't see them I can share some git config options that might make them show up in your terminal, I'm not exactly sure what in my config file enables that though.)
Good luck and thanks for the contribution!
In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
926e4ab to
9c94acc
Compare
|
Thank you for the detailed review |
9c94acc to
be07dfc
Compare
|
Nice, thanks man. This is looking really good. Sorry to be a nuisance but can re-write the git log of the first commit so it is well formed. Here is a nice blog post if you'd like to learn how (and why) to do so: https://cbea.ms/git-commit/ |
Implement `checked_div_by_fee_rate_floor` and `checked_div_by_fee_rate_ceil` methods to compute the maximum and minimum transaction weights. These methods provide precise weight calculations using both floor and ceiling divisions. - Introduce `checked_div_by_fee_rate_floor` for floor-based weight division. - Add `checked_div_by_fee_rate_ceil` for ceiling-based weight division. - Update `ops::Div` implementation for `Amount` to use floor division by default. - Include unit tests to validate both floor and ceiling division methods.
be07dfc to
7482fcd
Compare
|
Thanks for your continuous feedback |
|
Nice thanks bro!
This made me double take because it made me think the PR changed the current Don't change the PR though, its good to go as is. Appreciate you taking on board my feedback! Hope to see you doing more |
We are trying a new strategy to get to 1.0 more quickly - remove `amount` and `fee` and release everything else. In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
We are trying a new strategy to get to 1.0 more quickly - remove `amount` and `fee` and release everything else. In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
We are trying a new strategy to get to 1.0 more quickly - remove `amount` and `fee` and release everything else. In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
We are trying a new strategy to get to 1.0 more quickly - remove `amount` and `fee` and release everything else. In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
We are trying a new strategy to get to 1.0 more quickly - remove `amount` and `fee` and release everything else. In preparation for release add a changelog entry, bump the version, and update the lock files. `v1.0` here we come. Before we merge this we should add: - rust-bitcoin#3934 or rust-bitcoin#3866 - rust-bitcoin#3933 - rust-bitcoin#3932 - rust-bitcoin#3929 - rust-bitcoin#3926 - rust-bitcoin#3923 - rust-bitcoin#3893 - rust-bitcoin#3866 - rust-bitcoin#3794
Add checked_div_by_fee_rate method to Amount that computes the maximum weight for a transaction with a given fee rate. This complements the existing
fee = fee_rate * weightandfee_rate = fee / weightoperationschecked_div_by_fee_ratemethod that returns OptionDiv<FeeRate>for Amount for operator syntax supportfloordivision to ensure weight doesn't exceed intended feeThis allows calculating the maximum transaction weight possible for a given fee amount and fee rate.
Closes #3814