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

Rename priority to compute_unit_price #35062

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Feb 2, 2024

Problem

  • Term "priority" is overloaded and used in multiple places to mean different things

Summary of Changes

  • Split out of Scheduler - prioritization fees/cost #34888 to make the overloaded "priority" less overloaded
  • Renames TransactionPriorityDetails priority field to compute_unit_price
  • Renames TransactionPriorityDetails and associated traits to ComputeBudgetDetails

Fixes #

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (dd30175) 81.6% compared to head (ff44269) 81.6%.
Report is 4 commits behind head on master.

❗ Current head ff44269 differs from pull request most recent head e8d6e6e. Consider uploading reports for the commit e8d6e6e to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #35062     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         830      830             
  Lines      224946   224949      +3     
=========================================
- Hits       183674   183670      -4     
- Misses      41272    41279      +7     

@ryoqun ryoqun self-requested a review February 3, 2024 12:20
@@ -208,7 +205,7 @@ impl PrioritizationFeeCache {
}
}

/// Update with a list of non-vote transactions' tx_priority_details and tx_account_locks; Only
/// Update with a list of non-vote transactions' tx_compute_budget_details and tx_account_locks; Only
/// transactions have both valid priority_detail and account_locks will be used to update
Copy link
Member

Choose a reason for hiding this comment

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

nit: priority_detail => compute_budget_details (also, i'd prefer removing the seemingly-unneeded tx_ prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -294,16 +294,16 @@ mod tests {
}

#[test]
fn test_transaction_priority_details() {
fn test_compute_budget_details() {
let priority = 15;
Copy link
Member

@ryoqun ryoqun Feb 3, 2024

Choose a reason for hiding this comment

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

nit: rename to let compute_unit_price = 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let mut priority_details = sanitized_transaction
.get_transaction_priority_details(packet.meta().round_compute_unit_price())
let mut compute_budget_details = sanitized_transaction
.get_compute_budget_details(packet.meta().round_compute_unit_price())
.ok_or(DeserializedPacketError::PrioritizationFailure)?;

// set priority to zero for vote transactions
Copy link
Member

Choose a reason for hiding this comment

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

nit: // set compute_unit_price to zero for vote transactions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apfitzge apfitzge requested a review from ryoqun February 5, 2024 12:13
Copy link
Member

Choose a reason for hiding this comment

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

thanks for not forgetting rename. :)

Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm; perfect. i think touched code is read better a lot. thanks for addressing all my nits.

@apfitzge
Copy link
Contributor Author

apfitzge commented Feb 5, 2024

lgtm; perfect. i think touched code is read better a lot. thanks for addressing all my nits.

Will get @tao-stones to take a look as well, much of this is compute unit stuff so he may also have opinions!

Copy link
Contributor

@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.

lgtm - thanks for renaming

@@ -8,46 +8,46 @@ use {
};

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct TransactionPriorityDetails {
pub priority: u64,
pub struct ComputeBudgetDetails {
Copy link
Contributor

Choose a reason for hiding this comment

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

at first glance, I wanted to nit pick the name ComputeBudgetDetails here, cause it implies to details of ComputeBudget defined program-runtime/src/compute_budget.rs. But then, this file is on its way out, we don't needed it when start to using runtime-transaction. I'm OK with this change for now.

@apfitzge apfitzge merged commit 9dca15a into solana-labs:master Feb 6, 2024
35 checks passed
@apfitzge apfitzge deleted the compute_unit_price_rename branch February 6, 2024 00:41
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.

3 participants