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

single-pool: add ReactivatePool instruction #5488

Merged
merged 5 commits into from
Oct 13, 2023

Conversation

2501babe
Copy link
Member

handles the issue described in #5439. the first commit is the important one (changes the program itself), the others just add to the cli. also i moved half the commands into a subcommand, 99% of users are only ever going to need deposit and withdraw so im trying to minimize visual clutter

Comment on lines 46 to 47
/// Restake the pool stake account if its validator is force-deactivated and later
/// recreated with the same vote account.
Copy link
Member Author

Choose a reason for hiding this comment

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

feel free to suggest different wording for this, im not familiar enough with the mechanism that causes this to happen to craft proper verbiage

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call out all the cases: "Restake the pool stake account if it was deactivated. This can happen through the stake program's DeactivateDelinquent instruction, or during a cluster restart."

Comment on lines 197 to +199
// make an individual instruction for all program instructions
// the match is just so this will error if new instructions are added
// if you are reading this because of that error, add the case to the `consistent_account_order` test!!!
Copy link
Member Author

Choose a reason for hiding this comment

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

it may amuse you to hear that i added this comment because i, the author of this file, almost added the new instruction to the match without remembering it also needs to be added to the test

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha we all have goldfish-brain moments

@2501babe 2501babe marked this pull request as draft October 10, 2023 13:40
@2501babe 2501babe marked this pull request as ready for review October 10, 2023 13:40
@2501babe
Copy link
Member Author

2501babe commented Oct 10, 2023

ughhhh i marked draft thinking i would merge #5183 first because this will be much easier to rebase. but actually nevermind im going to merge this first because i have to rebase #5183 already anyway because dependabot updates mean ci doesnt run on it until i do. so ill fix the js code properly in there because the js code in master is all getting deleted anyway

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good! Just a few little comments

Comment on lines 46 to 47
/// Restake the pool stake account if its validator is force-deactivated and later
/// recreated with the same vote account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call out all the cases: "Restake the pool stake account if it was deactivated. This can happen through the stake program's DeactivateDelinquent instruction, or during a cluster restart."

/// 5. `[]` Stake history sysvar
/// 6. `[]` Stake config sysvar
/// 7. `[]` Stake program
ReactivatePool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I got a little tripped up on the name, but I see what you use it, since redelegate means something else. The only thing I was able to come up with is DelegatePoolStake which would say literally what's happening, but wouldn't be as clear as to when it would be useful. If you prefer ReactivatePool, I'm good with it

Copy link
Member Author

Choose a reason for hiding this comment

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

ReactivateStake? ReactivatePoolStake?

Copy link
Member Author

Choose a reason for hiding this comment

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

last one seems fine

Comment on lines 197 to +199
// make an individual instruction for all program instructions
// the match is just so this will error if new instructions are added
// if you are reading this because of that error, add the case to the `consistent_account_order` test!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha we all have goldfish-brain moments

single-pool/program/src/processor.rs Outdated Show resolved Hide resolved
Comment on lines +48 to +51
context.set_account(
&accounts.stake_account,
&AccountSharedData::from(stake_account),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I should really use this in my testing too, this was a nice reminder, thanks!

single-pool/cli/src/main.rs Outdated Show resolved Hide resolved
single-pool/cli/src/cli.rs Outdated Show resolved Hide resolved

Ok(format_output(
config,
"Reactivate".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: to be consistent with the instruction name?

Suggested change
"Reactivate".to_string(),
"ReactivatePool".to_string(),

Copy link
Member Author

Choose a reason for hiding this comment

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

theres a mystery convention in my head where InitializePool -> Initialize, DepositStake -> Deposit etc because these have longer "instruction" names and shorter "transaction" names, but since this has no multi-instruction transaction i guess that makes more sense

@2501babe
Copy link
Member Author

normally id merge because the fixes are trivial but wanna get the stamp because i changed the program. also i copied your fix from #5514

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for integrating all the suggestions!

@2501babe 2501babe merged commit b51213d into solana-labs:master Oct 13, 2023
9 checks passed
@2501babe 2501babe deleted the 20231010_svspreactivate branch October 13, 2023 10:56
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