-
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
refactor entropy register so CLI/TUI are similar #242
refactor entropy register so CLI/TUI are similar #242
Conversation
const entropy: Entropy = await loadEntropy(accountToRegister.address, endpoint) | ||
const accountService = new EntropyAccount(entropy, endpoint) | ||
const updatedAccount = await accountService.registerAccount(accountToRegister) | ||
// NOTE: loadEntropy throws if it can't find opts.account |
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.
// NOTE: loadEntropy throws if it can't find opts.account | |
// NOTE: loadEntropy throws if it can't find opts.account | |
// This is important because we don't want to register if we don't have a place to persist later |
@@ -75,28 +76,6 @@ export class EntropyAccount extends EntropyBase { | |||
}) | |||
} | |||
|
|||
// WATCH: should this be extracted to interaction.ts? | |||
async registerAccount (account: EntropyAccountConfig, registerParams?: AccountRegisterParams): Promise<EntropyAccountConfig> { |
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.
All this function is is registerAndPushKeysIntoAccount
I have pulled this out into the verbosely named util which
- pushes into account
- persists config
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.
it's also the lgger correct?
export async function addVerifyingKeyToAccountAndSelect (verifyingKey: string, accountNameOrAddress: string) { | ||
const storedConfig = await config.get() | ||
const account = findAccountByAddressOrName(storedConfig.accounts, accountNameOrAddress) | ||
if (!account) throw Error(`Unable to persist verifyingKey "${verifyingKey}" to unknown account "${accountNameOrAddress}"`) |
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 is very unlikely because we have earlier checked the account exists...but we put a nice message out anyway 🤷
Alternative.. make a function with an uglier signature (storeConfig, verifyingKey, account)
? 😢 this could still fail if account
is not within storedConfig.accounts
tho ...
await config.set({ | ||
...storedConfig, | ||
selectedAccount: account.address | ||
}) |
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.
Check my working.
We don't need to any of the fancy slicing or inserting accounts
here because:
- on line 28 : we grab the ref for target
account
instoredConfig.accounts
- on line 31 : we push a key into that account's data (mutating it)
- on line 35 : we say "persist that mutated storedConfig"
During QA discussion, notice that entropy register was coded the same in CLI + TUI.
This PR:
main
does to only be registering on-chainutil
command
andinteraction
to use the same 2 functionscommand
action