-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix cli opts bug with multiple accountOptions #250
Changes from 3 commits
fb4691b
6eccbba
ea23881
ebaf352
6aa5c15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ function getConfigOrNull () { | |
|
||
export function endpointOption () { | ||
return new Option( | ||
'-e, --endpoint <endpoint>', | ||
'-e, --endpoint <url>', | ||
[ | ||
'Runs entropy with the given endpoint and ignores network endpoints in config.', | ||
'Can also be given a stored endpoint name from config eg: `entropy --endpoint test-net`.' | ||
|
@@ -44,36 +44,43 @@ export function endpointOption () { | |
|
||
export function passwordOption (description?: string) { | ||
return new Option( | ||
'-p, --password <password>', | ||
'-p, --password <string>', | ||
description || 'Password for the account' | ||
) | ||
.hideHelp(true) | ||
} | ||
|
||
export function accountOption () { | ||
const storedConfig = getConfigOrNull() | ||
|
||
return new Option( | ||
'-a, --account <accountAddressOrName>', | ||
'-a, --account <name|address>', | ||
[ | ||
'Sets the account for the session.', | ||
'Defaults to the last set account (or the first account if one has not been set before).' | ||
].join(' ') | ||
) | ||
.env('ENTROPY_ACCOUNT') | ||
.argParser(async (account) => { | ||
if (storedConfig && storedConfig.selectedAccount !== account) { | ||
// Updated selected account in config with new address from this option | ||
await config.set({ | ||
...storedConfig, | ||
selectedAccount: account | ||
}) | ||
} | ||
.argParser(addressOrName => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐛 - this cannot be async! See comments |
||
// We try to map addressOrName to an account we have stored | ||
if (!storedConfig) return addressOrName | ||
|
||
return account | ||
const account = findAccountByAddressOrName(storedConfig.accounts, addressOrName) | ||
if (!account) return addressOrName | ||
|
||
// If we find one, we set this account as the future default | ||
config.setSelectedAccount(account) | ||
// NOTE: argParser cannot be an async function, so we cannot await this call | ||
// WARNING: this will lead to a race-condition if functions are called in quick succession | ||
// and assume the selectedAccount has been persisted | ||
// | ||
// RISK: doesn't seem likely as most of our functions will await at slow other steps.... | ||
// SOLUTION: write a scynchronous version? | ||
|
||
// We finally return the account name to be as consistent as possible (using name, not address) | ||
return account.name | ||
}) | ||
.default(storedConfig?.selectedAccount) | ||
// TODO: display the *name* not address | ||
// TODO: standardise whether selectedAccount is name or address. | ||
} | ||
|
||
export async function loadEntropy (addressOrName: string, endpoint: string, password?: string): Promise<Entropy> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import envPaths from 'env-paths' | |
|
||
import allMigrations from './migrations' | ||
import { serialize, deserialize } from './encoding' | ||
import { EntropyConfig } from './types' | ||
import { EntropyConfig, EntropyAccountConfig } from './types' | ||
|
||
const paths = envPaths('entropy-cryptography', { suffix: '' }) | ||
const CONFIG_PATH = join(paths.config, 'entropy-cli.json') | ||
|
@@ -73,14 +73,28 @@ export async function set (config: EntropyConfig, configPath = CONFIG_PATH) { | |
await writeFile(configPath, serialize(config)) | ||
} | ||
|
||
export async function setSelectedAccount (account: EntropyAccountConfig, configPath = CONFIG_PATH) { | ||
const storedConfig = await get(configPath) | ||
|
||
if (storedConfig.selectedAccount === account.name) return storedConfig | ||
// no need for update | ||
|
||
const newConfig = { | ||
...storedConfig, | ||
selectedAccount: account.name | ||
} | ||
await set(newConfig, configPath) | ||
return newConfig | ||
} | ||
|
||
Comment on lines
+76
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 one bug I saw was inconsistent setting of I decided to fix it to be I then changed
Things I'm not happy about1. repeated `config.get`... 2. this is all gonna be a pain when we want to add `-c, --config ` 3. `src/account/utils.js` still has hard-coded `config.set` for `selectedAccount` because... we need to persist the whole config anyway... |
||
/* util */ | ||
function noop () {} | ||
function assertConfigPath (configPath) { | ||
function assertConfigPath (configPath: string) { | ||
if (!configPath.endsWith('.json')) { | ||
throw Error(`configPath must be of form *.json, got ${configPath}`) | ||
} | ||
} | ||
export function isDangerousReadError (err) { | ||
export function isDangerousReadError (err: any) { | ||
// file not found: | ||
if (err.code === 'ENOENT') return false | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THIS was a squirrely bug - tj/commander.js#2260