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 cli-output large functions and add starting epoch for rewards #3085

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

sakridge
Copy link

@sakridge sakridge commented Oct 6, 2024

Problem

Large functions in cli-output make the code hard to read.
No way to offset into the stake and vote rewards in case the user wants past rewards

Summary of Changes

Refactor functions into smaller ones.
Add --starting-epoch <EPOCH_NUM> argument to solana vote-account --with-rewards or stake-account commands.

Fixes #

@sakridge sakridge changed the title refactor cli-output large functions refactor cli-output large functions and add starting epohc Oct 7, 2024
@sakridge sakridge changed the title refactor cli-output large functions and add starting epohc refactor cli-output large functions and add starting epoch for vote rewards Oct 7, 2024
@sakridge sakridge force-pushed the refactor-cli-output branch 2 times, most recently from 86242f3 to d15a944 Compare October 7, 2024 09:32
@sakridge sakridge changed the title refactor cli-output large functions and add starting epoch for vote rewards refactor cli-output large functions and add starting epoch for rewards Oct 7, 2024
Copy link

@KirillLykov KirillLykov left a comment

Choose a reason for hiding this comment

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

left some comments

build_balance_message(active_stake, me.use_lamports_unit, true),
)?;
let activating_stake = me.activating_stake.or_else(|| {
if me.active_stake.is_none() {

Choose a reason for hiding this comment

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

I think might be simplified:

me.active_stake.is_none().then_some(delegated_stake)

Copy link
Author

Choose a reason for hiding this comment

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

I think another PR is best, I just wanted to move the logic as-is to make sure functionality is identical.

rpc_client
.get_epoch_info()?
.epoch
.saturating_sub(num_epochs as u64)

Choose a reason for hiding this comment

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

I don't quite understand why do we subtract num_epochs now (before we didn't). Before we subtract 1 in the loop before calling get_inflation_reward so maybe should be also here subtract 1?

Copy link
Author

Choose a reason for hiding this comment

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

Because the loop was proceeding backwards by epoch before always from the newest epoch. I reversed the direction to now start at the older epoch and then move forward.

Copy link
Author

Choose a reason for hiding this comment

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

num_epochs has to be at least 1. So it will subtract 1 here in the default case.

Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Agree, everything looks fine

@sakridge sakridge merged commit 50d13d1 into anza-xyz:master Oct 8, 2024
38 checks passed
@sakridge sakridge deleted the refactor-cli-output branch October 8, 2024 10:12
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.

2 participants