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

refactor: remove feature gate from total_fee function #1463

Merged
merged 1 commit into from
May 23, 2024

Conversation

jstarry
Copy link

@jstarry jstarry commented May 22, 2024

Problem

In PR #1462 I would like to calculate fees details once and reuse them downstream in places where the active feature set might not available. This makes calculating the total_fee for a transaction difficult because it requires a feature flag.

Summary of Changes

Minor refactor which stores the remove_rounding_in_fee_calculation feature gate value in FeeDetails so that it isn't required when calling total_fee

Fixes #

@jstarry jstarry requested a review from tao-stones May 22, 2024 23:23
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 97.05882% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 82.7%. Comparing base (e227d25) to head (3b09eda).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1463   +/-   ##
=======================================
  Coverage    82.7%    82.7%           
=======================================
  Files         871      871           
  Lines      370326   370393   +67     
=======================================
+ Hits       306523   306584   +61     
- Misses      63803    63809    +6     

Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

Changes look good, and serve the purpose well. Just want to clarify before sign it off: the cached gate status will not be reused cross epoch boundary?

pub struct FeeDetails {
transaction_fee: u64,
prioritization_fee: u64,
remove_rounding_in_fee_calculation: bool,
Copy link

Choose a reason for hiding this comment

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

nit: FeeDetails is public and implements default, would making remove_rounding_in_fee_calculation explicit as option<bool> safer than just bool?

Copy link
Author

Choose a reason for hiding this comment

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

Well you can't set tx or pri fee to a non-zero value via a public method so if the default impl is used, the round feature wouldn't have any effect anyways

@jstarry
Copy link
Author

jstarry commented May 23, 2024

Good point, I wasn't planning for it to be used across epoch boundaries. Also, fee details in general can't be used across epoch boundaries so this PR doesn't make the situation any worse.

Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

agree on both points, ship it

@jstarry jstarry merged commit cd7710b into anza-xyz:master May 23, 2024
51 checks passed
@jstarry jstarry deleted the refactor/fee-details branch May 23, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants