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.

Sign up for GitHub

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

Jump to bottom

separate priority fee and transaction fee from fee calculation #34757

Merged
merged 2 commits into from
Jan 26, 2024
Merged

separate priority fee and transaction fee from fee calculation #34757

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2f33f5
add function calculate_fee_details() that returns FeeDetails with bas…
tao-stones Jan 25, 2024
f84b315
Merge branch 'master' into calculate-fee-returns-details
tao-stones Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions sdk/src/fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,19 @@ pub struct FeeStructure {
pub compute_fee_bins: Vec<FeeBin>,
}

/// Return type of calculate_fee(...)
#[derive(Debug, Default, Clone, Eq, PartialEq)]
pub struct FeeDetails {
transaction_fee: u64,
prioritization_fee: u64,
}

impl FeeDetails {
pub fn total_fee(&self) -> u64 {
self.transaction_fee.saturating_add(self.prioritization_fee)
}
}

pub const ACCOUNT_DATA_COST_PAGE_SIZE: u64 = 32_u64.saturating_mul(1024);

impl FeeStructure {
Expand Down Expand Up @@ -75,15 +88,32 @@ impl FeeStructure {
.saturating_mul(heap_cost)
}

/// Calculate fee for `SanitizedMessage`
#[cfg(not(target_os = "solana"))]
pub fn calculate_fee(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep this function live & active. @apfitzge has a good point that most use case just want total_fee. Doesn't need to deprecate it.

&self,
message: &SanitizedMessage,
_lamports_per_signature: u64,
lamports_per_signature: u64,
budget_limits: &FeeBudgetLimits,
include_loaded_account_data_size_in_fee: bool,
) -> u64 {
self.calculate_fee_details(
message,
lamports_per_signature,
budget_limits,
include_loaded_account_data_size_in_fee,
)
.total_fee()
}

/// Calculate fee details for `SanitizedMessage`
#[cfg(not(target_os = "solana"))]
pub fn calculate_fee_details(
&self,
message: &SanitizedMessage,
_lamports_per_signature: u64,
budget_limits: &FeeBudgetLimits,
include_loaded_account_data_size_in_fee: bool,
) -> FeeDetails {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is changing the signature of a public interface, which is technically against semver. who's consuming this?

Copy link
Contributor Author

@tao-stones tao-stones Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Direct callers (besides tests) are bank (calculate tx fee for distribution) and accounts (to validate payer account), both are updated in this PR. Don't think it is exposed to external 🤞🏼

Copy link
Contributor

Choose a reason for hiding this comment

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

well it's exposed for sure 'cause sdk is a publicly consumable crate and this method is publicly exported. question is whether it's actually consumed by anyone, which we really ought not find out the hard way.

i suppose the most pedantic thing to do here would be to move this logic to a new method calculate_fee_details(...) -> FeeDetails, then deprecate calculate_fee(...) -> u64 replacing its body with a call to calculate_fee_details(...).total_fees()

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Trent here, it's quite possible 3rd party code usees this function.
Though I'm not sure we need to actually deprecate calculate_fee, so long as the impl is just calculate_fee_details(...).total_fees().

Seems reasonable there are many call-sites that don't care about the details, just the total fee. Might simplify this PR as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree too. But does calculate_fee() need to be exposed to external? Users use simulator to get tx fee, are they? github says this function was moved from runtime::bank to sdk::fee_structure not too long ago. Maybe entire fee_structure belong to solana_runtime instead of sdk?

Copy link
Contributor

Choose a reason for hiding this comment

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

the ship has sailed on whether it needs to be or not. we've already shipped releases with it exposed in the public api of a publicly consumable crate that we've promised to follow semver on. removing that symbol for public availability anytime but a major release will be a violation of semver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

033c98b for the sailed ship

let signature_fee = message
.num_signatures()
.saturating_mul(self.lamports_per_signature);
Expand Down Expand Up @@ -115,12 +145,13 @@ impl FeeStructure {
.unwrap_or_default()
});

(budget_limits
.prioritization_fee
.saturating_add(signature_fee)
.saturating_add(write_lock_fee)
.saturating_add(compute_fee) as f64)
.round() as u64
FeeDetails {
transaction_fee: (signature_fee
.saturating_add(write_lock_fee)
.saturating_add(compute_fee) as f64)
.round() as u64,
Comment on lines +149 to +152
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated, but why do we round here even after removing congestion? these are all integers, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll patch it separately

prioritization_fee: budget_limits.prioritization_fee,
}
}
}

Expand Down
Loading