Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Upgrade VoteState#8348

Merged
solana-grimes merged 1 commit intosolana-labs:masterfrom
carllin:UpgradePath
Feb 26, 2020
Merged

Upgrade VoteState#8348
solana-grimes merged 1 commit intosolana-labs:masterfrom
carllin:UpgradePath

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Feb 20, 2020

Problem

No way to upgrade to new VoteState without breaking ABI

Depends on and includes changes from #8303

Summary of Changes

Convert VoteState's between different releases using an enum to differentiate the types

TODO: convert snapshots to include new VoteStates

Fixes #

@carllin carllin force-pushed the UpgradePath branch 20 times, most recently from a6d24df to dcd425c Compare February 21, 2020 00:14
self.lamports()?.saturating_sub(meta.rent_exempt_reserve), // can't stake the rent ;)
vote_account.unsigned_key(),
&vote_account.state()?,
&State::<VoteStateVersions>::state(vote_account)?.convert_to_current(),
Copy link
Copy Markdown
Contributor Author

@carllin carllin Feb 21, 2020

Choose a reason for hiding this comment

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

@mvines, I dislike this state() function because it requires whoever modifies this code to know that vote accounts are special/need special handling, and can't directly be deserialized into a VoteState object like its other counterparts(stake, storage, nonce, etc.). Ideally the Account type carries the type of the object it's storing so we can avoid this footgun (even in this PR, hopefully I haven't missed any conversions directly to VoteState from account.data), but that's a more sweeping change I can add as a follow-up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fn state(&self) -> Result<T, InstructionError> {
self.try_account_ref()?.state()
}

We could add a trait that requires an implementation of fn convert_to_current() then make it a bound on T for State<T> and StateMut<T> hide it in the fn state() implementations

@carllin carllin force-pushed the UpgradePath branch 4 times, most recently from 7521422 to e013d49 Compare February 21, 2020 08:11
@carllin carllin mentioned this pull request Feb 21, 2020
@carllin carllin force-pushed the UpgradePath branch 3 times, most recently from 6bf0d2d to 15656c7 Compare February 24, 2020 11:26
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2020

Codecov Report

Merging #8348 into master will decrease coverage by <.1%.
The diff coverage is 90.3%.

@@           Coverage Diff            @@
##           master   #8348     +/-   ##
========================================
- Coverage    80.3%   80.3%   -0.1%     
========================================
  Files         254     256      +2     
  Lines       56235   56341    +106     
========================================
+ Hits        45173   45254     +81     
- Misses      11062   11087     +25

@carllin carllin force-pushed the UpgradePath branch 3 times, most recently from f4ac0dc to 25ec429 Compare February 25, 2020 08:43
@carllin carllin marked this pull request as ready for review February 25, 2020 23:49
Comment thread programs/vote/src/vote_state/vote_state_0_23_5.rs
@carllin carllin added the automerge Merge this Pull Request automatically once CI passes label Feb 26, 2020
@solana-grimes solana-grimes merged commit d821fd2 into solana-labs:master Feb 26, 2020
@carllin carllin added the v1.0 label Feb 28, 2020
mergify Bot pushed a commit that referenced this pull request Feb 28, 2020
automerge

(cherry picked from commit d821fd2)
@mergify mergify Bot mentioned this pull request Feb 28, 2020
solana-grimes pushed a commit that referenced this pull request Feb 28, 2020
carllin pushed a commit to carllin/solana that referenced this pull request Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants