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

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 <[email protected]>
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 <[email protected]>
@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