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

Implement VoteInstruction::AuthorizeWithSeed & VoteInstruction::AuthorizeWithSeedChecked #25928

Merged
merged 11 commits into from
Jun 14, 2022
Merged

Implement VoteInstruction::AuthorizeWithSeed & VoteInstruction::AuthorizeWithSeedChecked #25928

merged 11 commits into from
Jun 14, 2022

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Jun 12, 2022

Background

Given a base key, a seed (arbitrary data, like the string "VOTER_SEED") and the address of a program, you can create a derived key for that program. This key can never sign for anything, because it has no associated secret key.

The vote program lets you authorize an account to be the ‘withdrawer.’

Problem

There's nothing stopping you from authorizing a derived key to be the ‘withdrawer’ authority on a vote account, but if you do, you'll never be able to withdraw funds, since nobody can sign for the derived key.

Summary of Changes

  • Add an AuthorizeWithSeed instruction/implementation/tests to the vote program. Now someone who can sign for the base key of a derived key can at least change such a ‘withdrawer’ authority to something else.
  • Add an AuthorizeWithSeedChecked instruction/implementation/tests to the vote program. Does the same as AuthorizeWithSeed except that it also checks that the base key of the new authority has signed the transaction.
  • Add a feature gate that governs access to that new instruction
  • Update docs, wherever found
  • Add AuthorizeWithSeed command to web3.js library
  • Add AuthorizeWithSeedChecked command to web3.js library

Fixes #25860.
Feature Gate Issue: #25930.

@steveluscher steveluscher added enhancement New feature or request feature-gate Pull Request adds or modifies a runtime feature gate labels Jun 12, 2022
@steveluscher steveluscher added the rust Pull requests that update Rust code label Jun 12, 2022
@CriesofCarrots
Copy link
Contributor

@steveluscher , looks like clippy has some opinions about some unnecessary clones and immediate dereferences. Can you fix up so that we can see the rest of CI?

@steveluscher
Copy link
Contributor Author

…looks like clippy has some opinions…

Me: furiously Googles why cargo clippy isn't the default setting in the rust-analyzer VS Code plugin…

PR Updated!

@CriesofCarrots
Copy link
Contributor

Try cargo clippy --tests

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Looking good! Just a handful of suggestions, primarily around readability.
Can you please add an implementation for VoteInstruction::AuthorizeCheckedWithSeed? 🙏 🙏 It should look like VoteInstruction::AuthorizeWithSeed, plus checking that the new authority is a signer, as per

if !instruction_context.is_signer(first_instruction_account + 3)? {

programs/vote/src/vote_instruction.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
sdk/src/feature_set.rs Show resolved Hide resolved
programs/vote/src/vote_state/mod.rs Outdated Show resolved Hide resolved
cli/src/cli.rs Outdated Show resolved Hide resolved
programs/vote/src/vote_processor.rs Outdated Show resolved Hide resolved
@steveluscher
Copy link
Contributor Author

Alright! Everything except for AuthorizeWithSeedChecked has been rolled in. I'll work on that next; shall I put it up as a separate PR or just add a couple of commits to this one?

@CriesofCarrots
Copy link
Contributor

shall I put it up as a separate PR or just add a couple of commits to this one?

Let's add it to this one. I'll review the current set of commits in the meantime!

@steveluscher steveluscher changed the title Implement VoteInstruction::AuthorizeWithSeed Implement VoteInstruction::AuthorizeWithSeed & VoteInstruction::AuthorizeWithSeedChecked Jun 13, 2022
CriesofCarrots
CriesofCarrots previously approved these changes Jun 13, 2022
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Current set of commits look great! Nothing more from me on those

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #25928 (f587fae) into master (8caced6) will decrease coverage by 0.0%.
The diff coverage is 78.8%.

❗ Current head f587fae differs from pull request most recent head 481541f. Consider uploading reports for the commit 481541f to get more accurate results

@@            Coverage Diff            @@
##           master   #25928     +/-   ##
=========================================
- Coverage    82.1%    82.0%   -0.1%     
=========================================
  Files         628      631      +3     
  Lines      171471   173158   +1687     
=========================================
+ Hits       140878   142133   +1255     
- Misses      30593    31025    +432     

@mergify mergify bot dismissed CriesofCarrots’s stale review June 13, 2022 23:33

Pull request has been modified.

@steveluscher
Copy link
Contributor Author

Alright! Sprinkled in AuthorizeCheckedWithSeed.

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Nice, lgtm. Thanks, @steveluscher !

@steveluscher steveluscher merged commit 45d11f3 into solana-labs:master Jun 14, 2022
@steveluscher steveluscher deleted the vote-program-authorize-with-seed-instruction branch June 14, 2022 03:36
mergify bot pushed a commit that referenced this pull request Jun 14, 2022
…thorizeWithSeedChecked` (#25928)

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed`

* [vote_authorize_with_seed] You can now update a vote account's authority if it's a derived key for which you control the base key

* [vote_authorize_with_seed] Add test helper to create a vote account whose authorities are derived keys

* [vote_authorize_with_seed] Write tests to assert the behavior of `VoteInstruction::AuthorizeWithSeed`

* [vote_authorize_with_seed] Feature gate the `VoteInstruction::AuthorizeWithSeed` processor

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed` to transaction status parser

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed` to docs

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeCheckedWithSeed`

* [vote_authorize_with_seed] You can now update a vote account's authority (while checking that the new authority has signed) if it's a derived
key for which you control the base key

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeCheckedWithSeed` to transaction status parser

* [vote_authorize_with_seed] Write tests to assert the behavior of `VoteInstruction::AuthorizeCheckedWithSeed`

(cherry picked from commit 45d11f3)

# Conflicts:
#	programs/vote/src/vote_processor.rs
mergify bot added a commit that referenced this pull request Jun 14, 2022
…thorizeWithSeedChecked` (backport #25928) (#25956)

* Implement `VoteInstruction::AuthorizeWithSeed` & `VoteInstruction::AuthorizeWithSeedChecked` (#25928)

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed`

* [vote_authorize_with_seed] You can now update a vote account's authority if it's a derived key for which you control the base key

* [vote_authorize_with_seed] Add test helper to create a vote account whose authorities are derived keys

* [vote_authorize_with_seed] Write tests to assert the behavior of `VoteInstruction::AuthorizeWithSeed`

* [vote_authorize_with_seed] Feature gate the `VoteInstruction::AuthorizeWithSeed` processor

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed` to transaction status parser

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeWithSeed` to docs

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeCheckedWithSeed`

* [vote_authorize_with_seed] You can now update a vote account's authority (while checking that the new authority has signed) if it's a derived
key for which you control the base key

* [vote_authorize_with_seed] Add `VoteInstruction::AuthorizeCheckedWithSeed` to transaction status parser

* [vote_authorize_with_seed] Write tests to assert the behavior of `VoteInstruction::AuthorizeCheckedWithSeed`

(cherry picked from commit 45d11f3)

# Conflicts:
#	programs/vote/src/vote_processor.rs

* Fix conflicts and accommodate v1.10 api

Co-authored-by: Steven Luscher <[email protected]>
Co-authored-by: Tyera Eulberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature-gate Pull Request adds or modifies a runtime feature gate rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Vote program AuthorizeWithSeed instruction
2 participants