-
Notifications
You must be signed in to change notification settings - Fork 337
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: bank transaction counts #2382
Conversation
dce1a4f
to
621a7a1
Compare
621a7a1
to
1141ded
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only nits, changes look fine to me. think the new struct(s) help readability
// All transactions that were executed but then failed record because the | ||
// slot ended | ||
pub failed_commit_count: usize, | ||
// Total transaction counts tracked for reporting `LeaderSlotMetrics`. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistent with other comments so fine to leave as is, but why would all the comments on theese fields not be doc-comments?
@@ -60,6 +50,48 @@ pub(crate) struct ProcessTransactionsSummary { | |||
pub max_prioritization_fees: u64, | |||
} | |||
|
|||
#[derive(Debug, Default, PartialEq)] | |||
pub struct ProcessTransactionsCounts { | |||
// Total number of transactions that were passed as candidates for execution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// doc-comments here too?
Problem
Hard to follow how transaction counts are used throughout the runtime
Summary of Changes
ExecuteAndCommitTransactionsCounts
andProcessTransactionsCounts
u64
fromusize
for a number of countsFixes #