-
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
Conversation
.addOption(accountOption()) | ||
.addOption(endpointOption()) |
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
selectedAccount: account | ||
}) | ||
} | ||
.argParser(addressOrName => { |
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 cannot be async!
See comments
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 | ||
} | ||
|
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.
🔥 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 withname
- 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...
@mixmix you able to update this PR and rebase off of the current version of dev? can complete my full review after that's done. |
@rh0delta merged in |
tiny PRs!