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

Rework #17486 #17566

Merged
merged 3 commits into from
May 29, 2021
Merged

Rework #17486 #17566

merged 3 commits into from
May 29, 2021

Conversation

t-nelson
Copy link
Contributor

Problem

#17486 introduced several issues

  1. Easy to misuse and require a default wallet to exist for commands that didn't need it. This was the case for at least solana validators
  2. Assumed the path was a filepath, when it could be a bunch of other signer types which don't exist in a filesystem
  3. Made the assumption that "keypair" was always the arg name, which it is not for spl-token CLI

Summary of Changes

Review hints:

  • Ignore the first commit
  • Do a better job than I did on the original 😅

@jackcmay
Copy link
Contributor

There was a suggestion that we just force cli users to setup a default keypair because they will eventually need it anyway and might as well have them address it from the start.

@t-nelson t-nelson force-pushed the restrict-filepath branch 2 times, most recently from 6c4d80a to b457f6f Compare May 28, 2021 07:48
@t-nelson
Copy link
Contributor Author

Ok, I re-reworked this so that it defers the stat() call until the first attempt to query and remains isolated to the default signer rather than any other queried signers

@t-nelson t-nelson force-pushed the restrict-filepath branch from b457f6f to f7b300d Compare May 28, 2021 19:04
@t-nelson t-nelson requested a review from jackcmay May 28, 2021 19:27
jackcmay
jackcmay previously approved these changes May 28, 2021
CriesofCarrots
CriesofCarrots previously approved these changes May 28, 2021
@t-nelson t-nelson added the automerge Merge this Pull Request automatically once CI passes label May 28, 2021
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label May 28, 2021
@mergify
Copy link
Contributor

mergify bot commented May 28, 2021

automerge label removed due to a CI failure

@mergify mergify bot dismissed stale reviews from jackcmay and CriesofCarrots May 28, 2021 22:16

Pull request has been modified.

@t-nelson t-nelson merged commit d01b4f8 into solana-labs:master May 29, 2021
@t-nelson t-nelson deleted the restrict-filepath branch May 29, 2021 02:10
mergify bot added a commit that referenced this pull request May 31, 2021
* Revert "Improve missing default signer error messaging (#17486)"

This reverts commit 6d40d0d.

(cherry picked from commit ca8c1c6)

* Improve missing default filepath signer error messaging

(cherry picked from commit 06a926f)

* CI: temporarily skip spl downstream build

(cherry picked from commit d01b4f8)

Co-authored-by: Trent Nelson <trent@solana.com>
mergify bot added a commit that referenced this pull request May 31, 2021
* Revert "Improve missing default signer error messaging (#17486)"

This reverts commit 6d40d0d.

(cherry picked from commit ca8c1c6)

* Improve missing default filepath signer error messaging

(cherry picked from commit 06a926f)

* CI: temporarily skip spl downstream build

(cherry picked from commit d01b4f8)

Co-authored-by: Trent Nelson <trent@solana.com>
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
@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.

3 participants