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

fix cli opts bug with multiple accountOptions #250

Merged
merged 5 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/account/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { EntropyAccount } from "./main";
import { selectAndPersistNewAccount, addVerifyingKeyToAccountAndSelect } from "./utils";
import { ACCOUNTS_CONTENT } from './constants'
import * as config from '../config'
import { cliWrite, accountOption, endpointOption, loadEntropy, passwordOption } from "../common/utils-cli";
import { accountOption, endpointOption, passwordOption, cliWrite, loadEntropy } from "../common/utils-cli";

export function entropyAccountCommand () {
return new Command('account')
Expand Down Expand Up @@ -84,9 +84,9 @@ function entropyAccountList () {
function entropyAccountRegister () {
return new Command('register')
.description('Register an entropy account with a program')
.addOption(passwordOption())
.addOption(endpointOption())
.addOption(accountOption())
.addOption(endpointOption())
.addOption(passwordOption())
// Removing these options for now until we update the design to accept program configs
// .addOption(
// new Option(
Expand Down
5 changes: 1 addition & 4 deletions src/account/interaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,7 @@ export async function entropyAccount (endpoint: string, storedConfig: EntropyCon
return
}
const { selectedAccount } = await inquirer.prompt(accountSelectQuestions(accounts))
await config.set({
...storedConfig,
selectedAccount: selectedAccount.address
})
await config.setSelectedAccount(selectedAccount)

print('Current selected account is ' + selectedAccount)
return
Expand Down
6 changes: 3 additions & 3 deletions src/account/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ export async function selectAndPersistNewAccount (newAccount: EntropyAccountConf
throw Error(`An account with address "${newAccount.address}" already exists.`)
}

accounts.push(newAccount)
// persist to config, set selectedAccount
accounts.push(newAccount)
await config.set({
...storedConfig,
selectedAccount: newAccount.address
Expand All @@ -28,9 +29,8 @@ export async function addVerifyingKeyToAccountAndSelect (verifyingKey: string, a
const account = findAccountByAddressOrName(storedConfig.accounts, accountNameOrAddress)
if (!account) throw Error(`Unable to persist verifyingKey "${verifyingKey}" to unknown account "${accountNameOrAddress}"`)

account.data.registration.verifyingKeys.push(verifyingKey)

// persist to config, set selectedAccount
account.data.registration.verifyingKeys.push(verifyingKey)
await config.set({
...storedConfig,
selectedAccount: account.address
Expand Down
4 changes: 1 addition & 3 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { Command, Option } from 'commander'

import { EntropyTuiOptions } from './types'
import { accountOption, endpointOption, loadEntropy } from './common/utils-cli'
import { loadEntropy } from './common/utils-cli'
import * as config from './config'

import launchTui from './tui'
Expand All @@ -20,8 +20,6 @@ const program = new Command()
program
.name('entropy')
.description('CLI interface for interacting with entropy.xyz. Running this binary without any commands or arguments starts a text-based interface.')
.addOption(accountOption())
.addOption(endpointOption())
Comment on lines -23 to -24
Copy link
Contributor Author

@mixmix mixmix Oct 3, 2024

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

.addOption(
new Option(
'-d, --dev',
Expand Down
35 changes: 21 additions & 14 deletions src/common/utils-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`.'
Expand All @@ -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 => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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> {
Expand Down
20 changes: 17 additions & 3 deletions src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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
Copy link
Contributor Author

@mixmix mixmix Oct 3, 2024

Choose a reason for hiding this comment

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

🔥 one bug I saw was inconsistent setting of config.selectedAccount as account.name OR account.address

I decided to fix it to be name because then in the --help you will see e.g (default: naynay) instead of (default: 0x12312231231232) 💀

I then changed

  • accountOption to deal with name
  • added a special method setSelectedAccount to handle the persistence of the right thing

Things I'm not happy about 1. 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

Expand Down
9 changes: 9 additions & 0 deletions src/config/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export interface EntropyConfig {
dev: string;
'test-net': string
}
// selectedAccount is account.name (alias) for the account
selectedAccount: string
'migration-version': string
}
Expand All @@ -14,6 +15,14 @@ export interface EntropyAccountConfig {
data: EntropyAccountData
}

// Safe output format
export interface EntropyAccountConfigFormatted {
name: string
address: string
verifyingKeys: string[]
}

// TODO: document this whole thing
export interface EntropyAccountData {
debug?: boolean
seed: string
Expand Down
6 changes: 3 additions & 3 deletions src/sign/command.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { Command, /* Option */ } from 'commander'
import { cliWrite, accountOption, endpointOption, loadEntropy, passwordOption } from '../common/utils-cli'
import { accountOption, endpointOption, passwordOption, cliWrite, loadEntropy } from '../common/utils-cli'
import { EntropySign } from './main'

export function entropySignCommand () {
const signCommand = new Command('sign')
.description('Sign a message using the Entropy network. Output is a JSON { verifyingKey, signature }')
.argument('msg', 'Message you would like to sign (string)')
.addOption(passwordOption('Password for the source account (if required)'))
.addOption(endpointOption())
.addOption(accountOption())
.addOption(endpointOption())
.addOption(passwordOption('Password for the source account (if required)'))
// .addOption(
// new Option(
// '-r, --raw',
Expand Down
6 changes: 3 additions & 3 deletions src/transfer/command.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Command } from "commander"
import { accountOption, endpointOption, loadEntropy, passwordOption } from "src/common/utils-cli"
import { accountOption, endpointOption, passwordOption, loadEntropy } from "../common/utils-cli"
import { EntropyTransfer } from "./main"

export function entropyTransferCommand () {
Expand All @@ -8,9 +8,9 @@ export function entropyTransferCommand () {
.description('Transfer funds between two Entropy accounts.') // TODO: name the output
.argument('destination', 'Account address funds will be sent to')
.argument('amount', 'Amount of funds to be moved')
.addOption(passwordOption('Password for the source account (if required)'))
.addOption(endpointOption())
.addOption(accountOption())
.addOption(endpointOption())
.addOption(passwordOption('Password for the source account (if required)'))
.action(async (destination, amount, opts) => {
const entropy = await loadEntropy(opts.account, opts.endpoint)
const transferService = new EntropyTransfer(entropy, opts.endpoint)
Expand Down
6 changes: 1 addition & 5 deletions src/tui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ async function setupConfig () {

// set selectedAccount if we can
if (!storedConfig.selectedAccount && storedConfig.accounts.length) {
await config.set({
...storedConfig,
selectedAccount: storedConfig.accounts[0].address
})
storedConfig = await config.get()
storedConfig = await config.setSelectedAccount(storedConfig.accounts[0])
}

return storedConfig
Expand Down
2 changes: 1 addition & 1 deletion tests/account.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ test('Account - list', async t => {
dev: 'ws://127.0.0.1:9944',
'test-net': 'wss://testnet.entropy.xyz',
},
selectedAccount: account.address,
selectedAccount: account.name,
'migration-version': '0'
}

Expand Down
Loading