Add get-recent-prioritization-fees command to cli #805

sakridge · 2024-04-15T02:30:10Z

Problem

No method to get recent prioritization fees in the CLI

Summary of Changes

Add a command

Fixes #

codecov-commenter · 2024-04-15T04:14:20Z

Codecov Report

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

Project coverage is 81.9%. Comparing base (e7452b7) to head (afeee2a).
Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #805     +/-   ##
=========================================
- Coverage    82.0%    81.9%   -0.1%     
=========================================
  Files         859      859             
  Lines      232906   232965     +59     
=========================================
- Hits       191079   191028     -51     
- Misses      41827    41937    +110

tao-stones

We might need to reconsider how we present RPC call results to the user

cli/src/cluster_query.rs

tao-stones

lgtm

CriesofCarrots

I haven't looked at the math here yet, but building a String to return with if verbose cases doesn't feel great to me, since we already have solana-cli-output handling for regular, quiet, and verbose displays. Can you please put the data in a display struct instead? That will also provide JSON output support by default, which I'm sure people are going to want for this subcommand.

sakridge · 2024-04-24T17:20:18Z

I haven't looked at the math here yet, but building a String to return with if verbose cases doesn't feel great to me, since we already have solana-cli-output handling for regular, quiet, and verbose displays. Can you please put the data in a display struct instead? That will also provide JSON output support by default, which I'm sure people are going to want for this subcommand.

Ok, it should support JSON now with cli-output, please check my work.

tao-stones

lgtm, pending @CriesofCarrots

cli/src/cluster_query.rs

t-nelson · 2024-04-25T05:19:52Z

cli/src/cluster_query.rs

+        .unwrap_or(fees.len() as u64)
+        .min(fees.len() as u64)
+        .max(1) as usize;


this can all be enforced with clap, which hands the user nice guidance instead of doing things silently behind their back

I have to parse again in an .validator() right? kind of annoying to do, but not perf sensitive and gives better feedback. It's just for the 0 case.

I have to parse again in an .validator() right?

Yeah. And it is kind of annoying 😅 Honestly, if someone is foolish enough to pass --limit-num-slots 0, I'm okay with just returning empty/default data. But Trent may feel differently

CriesofCarrots

Math/logic looks good. Just a couple more suggestions on using cli-output, plus Trent's clap suggestions.

cli-output/src/cli_output.rs

CriesofCarrots · 2024-04-25T18:04:19Z

cli-output/src/cli_output.rs

+        writeln!(f, "{:<11} {}", self.slot, self.prioritization_fee)?;
+        Ok(())


Suggested change

writeln!(f, "{:<11} {}", self.slot, self.prioritization_fee)?;

Ok(())

writeln!(f, "{:<11} {}", self.slot, self.prioritization_fee)

nit: just a little more concise

I don't see this change (nit)

cli/src/cluster_query.rs

changes have been made

CriesofCarrots

Wfm, but happy to restamp if you add the clap validator

sakridge force-pushed the get-recent-prioritization-fee-cli branch 2 times, most recently from 0887bd8 to d6b652b Compare April 19, 2024 08:44

sakridge requested review from CriesofCarrots and tao-stones April 19, 2024 08:45

sakridge marked this pull request as ready for review April 19, 2024 08:45

tao-stones requested changes Apr 19, 2024

View reviewed changes

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

sakridge force-pushed the get-recent-prioritization-fee-cli branch from d6b652b to 9ad59fb Compare April 22, 2024 01:33

sakridge requested a review from tao-stones April 22, 2024 01:34

sakridge force-pushed the get-recent-prioritization-fee-cli branch from 9ad59fb to b0e3eaa Compare April 22, 2024 02:54

tao-stones reviewed Apr 22, 2024

View reviewed changes

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

sakridge force-pushed the get-recent-prioritization-fee-cli branch from b0e3eaa to d4ad745 Compare April 22, 2024 15:55

tao-stones reviewed Apr 22, 2024

View reviewed changes

cli/src/cluster_query.rs Outdated Show resolved Hide resolved

sakridge force-pushed the get-recent-prioritization-fee-cli branch 2 times, most recently from 87cb300 to 1754296 Compare April 22, 2024 17:05

tao-stones previously approved these changes Apr 22, 2024

View reviewed changes

CriesofCarrots previously requested changes Apr 23, 2024

View reviewed changes

sakridge dismissed tao-stones’s stale review via c7c7b11 April 24, 2024 17:14

sakridge force-pushed the get-recent-prioritization-fee-cli branch 2 times, most recently from c7c7b11 to e2e3caa Compare April 24, 2024 17:16

sakridge requested review from CriesofCarrots and tao-stones April 24, 2024 17:19

sakridge force-pushed the get-recent-prioritization-fee-cli branch from e2e3caa to 72b0480 Compare April 24, 2024 17:40

tao-stones previously approved these changes Apr 25, 2024

View reviewed changes

t-nelson reviewed Apr 25, 2024

View reviewed changes

CriesofCarrots reviewed Apr 25, 2024

View reviewed changes

sakridge dismissed tao-stones’s stale review via 098c727 April 25, 2024 18:22

sakridge force-pushed the get-recent-prioritization-fee-cli branch 2 times, most recently from 098c727 to 1993ff0 Compare April 25, 2024 18:27

sakridge force-pushed the get-recent-prioritization-fee-cli branch from 1993ff0 to d7903dd Compare April 26, 2024 01:46

Add get-recent-prioritization-fees command to cli

afeee2a

sakridge force-pushed the get-recent-prioritization-fee-cli branch from d7903dd to afeee2a Compare April 26, 2024 01:47

CriesofCarrots approved these changes Apr 26, 2024

View reviewed changes

sakridge merged commit e2f361e into anza-xyz:master Apr 30, 2024
38 checks passed

sakridge deleted the get-recent-prioritization-fee-cli branch April 30, 2024 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add get-recent-prioritization-fees command to cli #805

Add get-recent-prioritization-fees command to cli #805

sakridge commented Apr 15, 2024 •

edited

Loading

codecov-commenter commented Apr 15, 2024 •

edited

Loading

tao-stones left a comment

tao-stones left a comment

CriesofCarrots left a comment

sakridge commented Apr 24, 2024

tao-stones left a comment

t-nelson Apr 25, 2024

sakridge Apr 25, 2024

CriesofCarrots Apr 25, 2024

CriesofCarrots left a comment

CriesofCarrots Apr 25, 2024

sakridge Apr 25, 2024

CriesofCarrots Apr 25, 2024 •

edited

Loading

CriesofCarrots left a comment

		writeln!(f, "{:<11} {}", self.slot, self.prioritization_fee)?;
		Ok(())

Add get-recent-prioritization-fees command to cli #805

Add get-recent-prioritization-fees command to cli #805

Conversation

sakridge commented Apr 15, 2024 • edited Loading

Problem

Summary of Changes

codecov-commenter commented Apr 15, 2024 • edited Loading

Codecov Report

tao-stones left a comment

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

sakridge commented Apr 24, 2024

tao-stones left a comment

Choose a reason for hiding this comment

t-nelson Apr 25, 2024

Choose a reason for hiding this comment

sakridge Apr 25, 2024

Choose a reason for hiding this comment

CriesofCarrots Apr 25, 2024

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots Apr 25, 2024

Choose a reason for hiding this comment

sakridge Apr 25, 2024

Choose a reason for hiding this comment

CriesofCarrots Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

sakridge commented Apr 15, 2024 •

edited

Loading

codecov-commenter commented Apr 15, 2024 •

edited

Loading

CriesofCarrots Apr 25, 2024 •

edited

Loading