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

Add stake_flags to stake state #32524

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Conversation

HaoranYi
Copy link
Contributor

@HaoranYi HaoranYi commented Jul 18, 2023

Problem

Fix a stake redelegation issue.

Part of #31876

Summary of Changes

Add StakeFlags to StakeState

NOTE: This change just adds StakeFlags to StakeState. The Stake behavior should not change.
The actual fix will come in a following PR once this PR is merged.

Fixes #

@HaoranYi HaoranYi changed the title add stake_flags to stake state Add stake_flags to stake state Jul 18, 2023
@HaoranYi
Copy link
Contributor Author

This PR will break downstream spl repo.

Here is the following up PR to fix SPL repo, once this PR is merged:
solana-labs/solana-program-library#4504

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm. gonna give spl folks the final approval here though since you'll need to workout deployment strategy with them

@mvines
Copy link
Member

mvines commented Jul 19, 2023

We probably should bump the major version of the solana-program crate with this PR (once releng folks figure out how to do this!) since it's a breaking change

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Jul 19, 2023

We probably should bump the major version of the solana-program crate with this PR (once releng folks figure out how to do this!) since it's a breaking change

I am not familiar with the version of solana-program. Do you mean to bump the following version?

./stake-pool/program/Cargo.toml:version = "0.7.0"

@joncinque

@t-nelson
Copy link
Contributor

I am not familiar with the version of solana-program. Do you mean to bump the following version?

./stake-pool/program/Cargo.toml:version = "0.7.0"

no. the version field in sdk/program/Cargo.toml (monorepo). there's an initiate to move both this crate and solana-sdk out of the monorepo, so they can be more easily independently versioned

@HaoranYi
Copy link
Contributor Author

diff --git a/ci/downstream-projects/func-spl.sh b/ci/downstream-projects/func-spl.sh
index 2eaf9baae3..d525e04949 100755
--- a/ci/downstream-projects/func-spl.sh
+++ b/ci/downstream-projects/func-spl.sh
@@ -20,10 +20,14 @@ spl() {
     )
     set -x
     rm -rf spl
-    git clone https://github.com/solana-labs/solana-program-library.git spl
+    git clone https://github.com/HaoranYi/solana-program-library.git spl
     # copy toolchain file to use solana's rust version
     cp "$SOLANA_DIR"/rust-toolchain.toml spl/
     cd spl || exit 1
+   git checkout redelegation_fix

     project_used_solana_version=$(sed -nE 's/solana-sdk = \"[>=<~]*(.*)\"/\1/p' <"token/program/Cargo.toml")
     echo "used solana version: $project_used_solana_version"

I have manually built and tested spl PR together with this change. All tests passed.

@t-nelson
Copy link
Contributor

I have manually built and tested spl PR together with this change. All tests passed.

less concerned about the tests and more about what needs to happen to get the program redeployed with this patch

@joncinque
Copy link
Contributor

joncinque commented Jul 20, 2023

less concerned about the tests and more about what needs to happen to get the program redeployed with this patch

The current deserializers work properly with the change, so we don't actually need to update the program. They just ignore the new stuff at the end of the struct

@t-nelson
Copy link
Contributor

less concerned about the tests and more about what needs to happen to get the program redeployed with this patch

The current deserializers work properly with the change, so we don't actually need to update the program. They just ignore the new stuff at the end of the struct

put yer money where yer mouth is and approve? 😉

Comment on lines +22 to 25
StakeState::Stake(meta, stake, _) => StakeAccountType::Delegated(UiStakeAccount {
meta: meta.into(),
stake: Some(stake.into()),
}),
Copy link
Member

@2501babe 2501babe Jul 22, 2023

Choose a reason for hiding this comment

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

im assuming the intent of flags (if more flags are ever added) is only for transient internal state tracking, so doesnt need to show up in UiStakeAccount?

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need to impose any use-restrictions on the flags. future additions that make sense to display will likely need handling that we can't predict today. for instance i'll claim one when i get around proposing vote account bonds, but ui display would probably be something like

flags.contains(FLAG_VOTER_BOND).then_some(delegation.vote_account)

Copy link
Contributor Author

@HaoranYi HaoranYi Jul 22, 2023

Choose a reason for hiding this comment

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

#32033

^^ This is a follow up PR, which adds the stake flags to UiStakeAccount.

@2501babe
Copy link
Member

lgtm, also note i tested my single pool cli tests, with a cli built against this pr, and a program built against 1.16, and everything worked fine. i wasnt able to run any sbf tests for an issue unrelated to this pr (ie they fail on monorepo master too, i think because of the tokio upgrade)

i am curious what the upgrade story here is tho, will you cut a new monorepo release after this pr? or is this blocked on splitting sdk/program out of monorepo? since once this is merged, we would need something to update to, and in the mean time monorepo ci will break on the downstream projects tests

@HaoranYi HaoranYi merged commit 17af3ab into solana-labs:master Jul 24, 2023
@HaoranYi HaoranYi deleted the stake_flags2 branch July 24, 2023 14:09
@HaoranYi
Copy link
Contributor Author

Merged.
solana-labs/solana-program-library#4504 was also merged.

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.

Late review, but just wanted to say that the stake program changes look good!

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.

5 participants