Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Add show account - Closes #540 #543

Merged
merged 7 commits into from
Jun 13, 2018
Merged

Add show account - Closes #540 #543

merged 7 commits into from
Jun 13, 2018

Conversation

shuse2
Copy link
Contributor

@shuse2 shuse2 commented Jun 7, 2018

Description

Add show account command

Review checklist

@shuse2 shuse2 added this to the Version 1.0.0 milestone Jun 7, 2018
@shuse2 shuse2 self-assigned this Jun 7, 2018
@shuse2 shuse2 requested a review from willclarktech June 7, 2018 10:32
import commonOptions from '../utils/options';
import cryptography from '../utils/cryptography';

const description = `Shows an account information with the given passphrase.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shows account information for a given passphrase


const description = `Shows an account information with the given passphrase.

Examples: show account
Copy link
Contributor

Choose a reason for hiding this comment

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

Example:

`;

const processInput = ({ passphrase }) =>
cryptography.liskCrypto.getAddressAndPublicKeyFromPassphrase(passphrase);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to provide the private key? We do when creating an account.

given.aPassphrase,
() => {
Given(
'an object with the public key "314852d7afb0d4c283692fef8a2cb40e30c7a5df2ed79994178c10ac168d6d977ef45cd525e95b7a86244bbd4eb4550914ad06301013958f4dd64d32ef7bc588" and the address "2167422481642255385L" derived from the passphrase',
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider reusing the given step which defines a passphrase and these additional items together?

when.theActionIsCalledWithTheOptions,
() => {
Then(
'it should resolve to an object with the public key and the address',
Copy link
Contributor

Choose a reason for hiding this comment

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

How did the action know about the passphrase in this scenario?

given.anEmptyOptionsObject,
() => {
When(
'the action is called with the options',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this branch?

given.anObjectWithThePublicKeyAndTheAddressDerivedFromThePassphrase,
() => {
Given(
'an empty options object',
Copy link
Contributor

Choose a reason for hiding this comment

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

What about with the passphrase passed as an option?

@@ -216,3 +216,18 @@ export function anEncryptedMessageWithANonce() {

this.test.ctx.cipherAndNonce = cipherAndNonce;
}

export function anObjectWithThePublicKeyAndTheAddressDerivedFromThePassphrase() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This step doesn't define an object at all.

@shuse2 shuse2 force-pushed the 540-add_show_account branch from 2d8bebb to 6701798 Compare June 11, 2018 12:10

const description = `Shows account information for a given passphrase.

Example: show account
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be tab-indented.

given.aVorpalInstanceWithAUIAndAnActiveCommandThatCanPrompt,
() => {
Given('an action "show account"', given.anAction, () => {
Given(
Copy link
Contributor

Choose a reason for hiding this comment

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

This step is now redundant.

() => {
Then(
'it should resolve to an object with the public key, the private key and the address',
then.itShouldResolveToAnObjectWithThePublicKeyThePrivateKeyAndTheAddress,
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a consistent ordering reflecting the derivation direction: in general passphrase -> privateKey -> publicKey -> address.

() => {
Given(
'an options object with passphrase set to "passphraseSource"',
given.anOptionsObjectWithPassphraseSetTo,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the passphrase be retrieved? What if it can? What if it can't?

Copy link
Contributor

@willclarktech willclarktech left a comment

Choose a reason for hiding this comment

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

Just a small typo, otherwise LGTM.

then.itShouldGetTheInputsFromSourcesUsingThePassphraseSourceWithARepeatingPrompt,
);
Then(
'it should resolve to an object withthe private key, the public key and the address',
Copy link
Contributor

Choose a reason for hiding this comment

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

with the

then.itShouldGetTheInputsFromSourcesUsingThePassphraseSourceWithARepeatingPrompt,
);
Then(
'it should resolve to an object withthe private key, the public key and the address',
Copy link
Contributor

Choose a reason for hiding this comment

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

with the

@shuse2 shuse2 force-pushed the 540-add_show_account branch from 2711d07 to 201d7fb Compare June 13, 2018 15:29
@shuse2 shuse2 merged commit efab1d9 into 1.0.0 Jun 13, 2018
@shuse2 shuse2 deleted the 540-add_show_account branch June 13, 2018 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants