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 Vote Program Account setup #2992

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

sagar-solana
Copy link
Contributor

@sagar-solana sagar-solana commented Feb 28, 2019

Summary of Changes

  • Split new_account() into fund_vote_account(), initialize_account(), and delegate_account().
  • Ensure rewards are paid into the staker account from the rewards account
  • Re-introduce stake sorting when generating the leader schedule. This is needed because without it, the leader schedule is dependent on the order in which vote accounts were inserted.

Fixes #2513

Hopeful alternative to #2973

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #2992 into master will decrease coverage by <.1%.
The diff coverage is 79.5%.

@@           Coverage Diff            @@
##           master   #2992     +/-   ##
========================================
- Coverage    76.9%   76.8%   -0.1%     
========================================
  Files         130     130             
  Lines       19888   19976     +88     
========================================
+ Hits        15294   15350     +56     
- Misses       4594    4626     +32

Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

This program is so important and pretty trick to debug. It needs loads of tests! See neighboring file programs/native/rewards/tests/rewards.rs for how to do that easily.

sdk/src/vote_program.rs Outdated Show resolved Hide resolved
sdk/src/vote_program.rs Outdated Show resolved Hide resolved
sdk/src/vote_program.rs Outdated Show resolved Hide resolved
/// identified by keys[0] for voting
RegisterAccount,
/// Initialize the VoteState for this "vote account"
/// * Transaction::keys[0] - the staker id that is also assumed to be the delegate by default
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs some work. While the transaction containing this instruction may have two accounts, this instruction should only point to the second one (the vote account). The transaction would be a SystemInstruction::CreateAccount and VoteInstruction::InitializeAccount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garious, I will need both accounts being passed in to the VoteProgram. To assign the staker as the delegate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garious, maybe I'm missing something. I need to assign the "staker" as the delegate by default. I cannot do that if I don't have the staker's pubkey (provided by accounts[0] right now). I suppose I could take it as an argument to Initialize?

sdk/src/vote_program.rs Outdated Show resolved Hide resolved
sdk/src/vote_program.rs Outdated Show resolved Hide resolved
sdk/src/vote_transaction.rs Outdated Show resolved Hide resolved
sdk/src/vote_transaction.rs Outdated Show resolved Hide resolved
@@ -50,11 +55,29 @@ impl VoteTransaction {
vec![system_program::id(), vote_program::id()],
vec![
Instruction::new(0, &create_tx, vec![0, 1]),
Instruction::new(1, &VoteInstruction::RegisterAccount, vec![0, 1]),
Instruction::new(1, &VoteInstruction::InitAccount, vec![0, 1]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Per previous comments, this can be vec![1].

sdk/src/vote_transaction.rs Show resolved Hide resolved
@sagar-solana sagar-solana force-pushed the vote_program_cleanup branch 2 times, most recently from 1fc132c to ea0767c Compare February 28, 2019 21:57
@sagar-solana
Copy link
Contributor Author

@garious, I was able to address most of your comments.

@garious
Copy link
Contributor

garious commented Feb 28, 2019

Can you rebase and merge?

@sagar-solana sagar-solana force-pushed the vote_program_cleanup branch from 7efcda5 to e035123 Compare March 1, 2019 00:05
@sagar-solana sagar-solana merged commit 20e4ede into solana-labs:master Mar 1, 2019
@sagar-solana sagar-solana deleted the vote_program_cleanup branch March 1, 2019 01:08
ryoqun pushed a commit to ryoqun/solana that referenced this pull request Sep 28, 2024
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