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

Cli: Support checked stake and vote operations #18449

Merged
merged 6 commits into from
Jul 15, 2021

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Jul 6, 2021

Problem

New checked stake and vote instructions aren't exposed in the solana-cli

Summary of Changes

Add new checked subcommands

  • Vote AuthorizeChecked
  • Stake InitializeChecked
  • Stake AuthorizeChecked
  • Stake SetLockupChecked
  • Integration tests
  • Try with hardware wallet

Needs rebase on #18345

@CriesofCarrots CriesofCarrots added the noCI Suppress CI on this Pull Request label Jul 6, 2021
@CriesofCarrots CriesofCarrots force-pushed the cli-checked-stake branch 3 times, most recently from 6c4c5c5 to 8c00cf3 Compare July 8, 2021 03:18
@CriesofCarrots CriesofCarrots removed the noCI Suppress CI on this Pull Request label Jul 8, 2021
@CriesofCarrots CriesofCarrots marked this pull request as ready for review July 8, 2021 03:19
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Jul 8, 2021
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 8, 2021
@CriesofCarrots CriesofCarrots force-pushed the cli-checked-stake branch 2 times, most recently from 4597166 to fbdb592 Compare July 8, 2021 03:54
@CriesofCarrots CriesofCarrots requested review from t-nelson and mvines July 8, 2021 03:56
@CriesofCarrots
Copy link
Contributor Author

I probably won't merge this until I get the Ledger work done and tested, but ready for review.
Incidentally, I think I would have preferred to add a --checked parameter to the affected commands instead of duplicate commands, but I wanted the help text to be very clear about the type of authority required (pubkey vs. keypair).

@mvines
Copy link
Contributor

mvines commented Jul 8, 2021

I probably won't merge this until I get the Ledger work done and tested, but ready for review.
Incidentally, I think I would have preferred to add a --checked parameter to the affected commands instead of duplicate commands, but I wanted the help text to be very clear about the type of authority required (pubkey vs. keypair).

Makes sense to me, I don't have a better suggestion. PR looks good to me overall as well

@stale
Copy link

stale bot commented Jul 15, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 15, 2021
@CriesofCarrots CriesofCarrots removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 15, 2021
@CriesofCarrots
Copy link
Contributor Author

This changeset + hardware wallet signers = looks good

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #18449 (c1299c4) into master (abe5a0a) will increase coverage by 0.0%.
The diff coverage is 88.1%.

@@           Coverage Diff            @@
##           master   #18449    +/-   ##
========================================
  Coverage    82.7%    82.7%            
========================================
  Files         438      438            
  Lines      123760   124276   +516     
========================================
+ Hits       102440   102879   +439     
- Misses      21320    21397    +77     

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.

Looks great!

It's a shame clap doesn't support conditional arg validation. These could've been a --use-checked-instruction instead of new subcommands 🤕

@CriesofCarrots
Copy link
Contributor Author

It's a shame clap doesn't support conditional arg validation. These could've been a --use-checked-instruction instead of new subcommands

Agreed, I even liked just a --checked flag.
But even with conditional arg validation, making useful help text for those PUBKEY or KEYPAIR args would be a bit of a bear, so oh well, hopefully clarity++

@CriesofCarrots CriesofCarrots merged commit aeb30fa into solana-labs:master Jul 15, 2021
mergify bot pushed a commit that referenced this pull request Jul 15, 2021
* Refactor VoteAuthorize to use SignerIndex and support vote-authorize-*-checked

* Add checked bool const and use in command parsing

* Add create-stake-account-checked handling

* Add stake-set-lockup-checked handling

* Remove unnecessary mut

* Add stake-authorized-checked handling

(cherry picked from commit aeb30fa)
CriesofCarrots added a commit that referenced this pull request Jul 16, 2021
* Refactor VoteAuthorize to use SignerIndex and support vote-authorize-*-checked

* Add checked bool const and use in command parsing

* Add create-stake-account-checked handling

* Add stake-set-lockup-checked handling

* Remove unnecessary mut

* Add stake-authorized-checked handling

(cherry picked from commit aeb30fa)
CriesofCarrots added a commit that referenced this pull request Jul 16, 2021
* Refactor VoteAuthorize to use SignerIndex and support vote-authorize-*-checked

* Add checked bool const and use in command parsing

* Add create-stake-account-checked handling

* Add stake-set-lockup-checked handling

* Remove unnecessary mut

* Add stake-authorized-checked handling

(cherry picked from commit aeb30fa)

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
@CriesofCarrots CriesofCarrots deleted the cli-checked-stake branch July 28, 2021 22:29
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants