Skip to content

Refactor fee calculation code#3813

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
tcharding:12-27-cacl-module
Jan 9, 2025
Merged

Refactor fee calculation code#3813
apoelstra merged 2 commits intorust-bitcoin:masterfrom
tcharding:12-27-cacl-module

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Dec 27, 2024

We have a bunch of functions and impl blocks scattered around the place for calculating fee from fee rate and weight.

In an effort to make the code easier to read/understand and also easier to audit introduce a private fee module and move all the code that is related to this calculation into it.

This is in internal change only.

@github-actions github-actions bot added the C-units PRs modifying the units crate label Dec 27, 2024
@tcharding tcharding changed the title Refactor fee calculation code Introduce fee calculation module Dec 27, 2024
@tcharding tcharding changed the title Introduce fee calculation module Refactor fee calculation module Dec 27, 2024
@tcharding tcharding changed the title Refactor fee calculation module Refactor fee calculation code Dec 27, 2024
@tcharding
Copy link
Copy Markdown
Member Author

I very intentionally did not include improvements in this PR to assist in review and reduced the need for bikeshedding. The new module is private and no changes are made, code is just moved.

Copy link
Copy Markdown
Contributor

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

All of the code moves look good, but I find the name of the module misleading since it doesn't have all of the calculations for units. Maybe fee or fee_calc is more descriptive?

@tcharding
Copy link
Copy Markdown
Member Author

ooo nice, I like fee.rs.

Copy link
Copy Markdown
Contributor

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK 6bc590fbf0c7a3659004dd789a1a395d2d55df45

Copy link
Copy Markdown
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 6bc590fbf0c7a3659004dd789a1a395d2d55df45; successfully ran local tests; though the commit message calls this the calc module still

@tcharding
Copy link
Copy Markdown
Member Author

Force push is change to commit log only, no code changes.

sanket1729
sanket1729 previously approved these changes Jan 2, 2025
Copy link
Copy Markdown
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 cadbc11aaed65acae43dc05fcb37e31f30fa49b5

apoelstra
apoelstra previously approved these changes Jan 3, 2025
Copy link
Copy Markdown
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 cadbc11aaed65acae43dc05fcb37e31f30fa49b5; successfully ran local tests

@apoelstra
Copy link
Copy Markdown
Member

Damn, needs rebase now.

@tcharding tcharding dismissed stale reviews from apoelstra and sanket1729 via 54a8fcd January 4, 2025 04:06
@tcharding
Copy link
Copy Markdown
Member Author

Weird, no merge conflicts.

@apoelstra
Copy link
Copy Markdown
Member

Needs rebase again

We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.

In preparation for adding a `calc` module use getters and setters in
code that will move to the `calc` module.

(Remember, the `FeeRate` uses an inner sats per kwu value.)

Internal change only.
We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.

In an effort to make the code easier to read/understand and also easier
to audit introduce a private `fee` module and move all the code that is
related to this calculation into it.

This is in internal change only.
Copy link
Copy Markdown
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 9d1cba4; successfully ran local tests

@apoelstra apoelstra merged commit 52f0586 into rust-bitcoin:master Jan 9, 2025
@tcharding tcharding deleted the 12-27-cacl-module branch January 19, 2025 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-units PRs modifying the units crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants