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.

Sign up for GitHub

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

Jump to bottom

Add GetMinimumDelegation stake program instruction #24020

Merged
merged 1 commit into from
Apr 2, 2022
Merged

Add GetMinimumDelegation stake program instruction #24020

merged 1 commit into from
Apr 2, 2022

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Mar 31, 2022 •
edited

Problem

There is not a way to query the minimum stake delegation.

Summary of Changes

  • Add new GetMinimumDelegation instruction to the stake program (and feature-gate)
  • Add get_minimum_delegation() helper function
  • Deprecate MINIMUM_STAKE_DELEGATION constant and update clients to use get_minimum_delegation() instead

Feature gate issue: #24057

@brooksprumo brooksprumo mentioned this pull request Mar 31, 2022
Closed
5 tasks
@brooksprumo brooksprumo force-pushed the minimum-stake/new-fn branch 2 times, most recently from 2aab6cb to c4ef47b Compare March 31, 2022 18:28
@brooksprumo brooksprumo marked this pull request as ready for review March 31, 2022 22:15
@brooksprumo
Copy link
Contributor Author

Optionally I could also split this PR up into the three points listed up Summary of Changes.

jstarry
jstarry previously approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Looks good just a few small things

programs/stake/src/lib.rs Outdated Show resolved Hide resolved
sdk/program/src/stake/mod.rs Show resolved Hide resolved
@mergify mergify bot dismissed jstarry’s stale review April 1, 2022 18:29

Pull request has been modified.

@brooksprumo brooksprumo added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 1, 2022
@brooksprumo brooksprumo linked an issue Apr 1, 2022 that may be closed by this pull request
Feature Gate: Add GetMinimumDelegation instruction to stake program #24057
Closed
brooksprumo
brooksprumo commented Apr 1, 2022
View reviewed changes
sdk/program/src/stake/mod.rs
/// NOTE: This is also used to calculate the minimum balance of a stake account, which is the
/// rent exempt reserve _plus_ the minimum stake delegation.
#[deprecated(
since = "1.10.6",
Copy link
Contributor Author

@brooksprumo brooksprumo Apr 1, 2022 •
edited

Choose a reason for hiding this comment

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

What's the process for deciding which version a feature should land in? I was originally thinking that I would backport this PR to v1.10, but is that right? And since this PR could be split up into three separate pieces, maybe the different pieces have different backport answers. @jstarry wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

i always use the current version of the oldest branch i intend to backport to

Copy link
Member

Choose a reason for hiding this comment

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

i always use the current version of the oldest branch i intend to backport to

Nice, makes sense!

And since this PR could be split up into three separate pieces, maybe the different pieces have different backport answers

This particular deprecation is specific to this PR so should be fine to follow trent's suggestion

@codecov
Copy link

codecov bot commented Apr 1, 2022 •
edited

Codecov Report

Merging #24020 (9a3fa32) into master (2da4e3e) will decrease coverage by 0.1%.
The diff coverage is 82.0%.

❗ Current head 9a3fa32 differs from pull request most recent head 15b6a96. Consider uploading reports for the commit 15b6a96 to get more accurate results

@@            Coverage Diff            @@
##           master   #24020     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.2%     
=========================================
  Files         581      589      +8     
  Lines      158312   160700   +2388     
=========================================
+ Hits       129518   131296   +1778     
- Misses      28794    29404    +610

jstarry
jstarry reviewed Apr 2, 2022
View reviewed changes
sdk/program/src/stake/mod.rs
/// rent exempt reserve _plus_ the minimum stake delegation.
#[deprecated(
since = "1.10.6",
note = "This constant may be outdated, please use `solana_stake_program::get_minimum_delegation` instead"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the directive here is quite right because solana_stake_program::get_minimum_delegation isn't public. Can we deprecate this later when you add the convenience method for doing the CPI + get_return_data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do. Since the PR is already merged, I'll put up another commit to remove the deprecation.

@brooksprumo brooksprumo removed the v1.10 label Apr 2, 2022
@brooksprumo brooksprumo added the automerge Merge this Pull Request automatically once CI passes label Apr 2, 2022
@mergify mergify bot merged commit 2af6753 into solana-labs:master Apr 2, 2022
@brooksprumo brooksprumo deleted the minimum-stake/new-fn branch April 3, 2022 16:23
@brooksprumo brooksprumo mentioned this pull request Apr 3, 2022
Merged
brooksprumo added a commit to brooksprumo/solana that referenced this pull request May 2, 2022
@brooksprumo
Add GetMinimumDelegation stake program instruction (solana-labs#24020)
1d18bfd
@brooksprumo brooksprumo mentioned this pull request May 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-nelson t-nelson t-nelson left review comments

@jstarry jstarry jstarry left review comments

Assignees
No one assigned
Labels
automerge Merge this Pull Request automatically once CI passes feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Gate: Add GetMinimumDelegation instruction to stake program
3 participants
@brooksprumo @t-nelson @jstarry