Migrate account command - Closes #557 #568

shuse2 · 2018-06-29T14:07:48Z

Description

Migration from

lisk create account
lisk show account

npm t
./bin/run account:create
./bin/run account:show
./bin/run account:get

Review checklist

It should be merged after Migrate config command - Closes #556 #567
The PR solves Migrate command account #557
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

willclarktech · 2018-07-30T08:10:50Z

package.json

@@ -57,7 +57,8 @@
 			"@oclif/plugin-help"
 		],
 		"topics": {
-			"config": { "description": "Manages Lisk Commander configuration." }
+			"config": { "description": "Manages Lisk Commander configuration." },
+			"account": { "description": "Manages account." }


"Manages Lisk accounts."?

I was thinking ahead that it should be able to create non-lisk accounts too, but i guess it's too ahead now

willclarktech · 2018-07-30T08:12:49Z

package.json

@@ -57,7 +57,8 @@
 			"@oclif/plugin-help"
 		],
 		"topics": {
-			"config": { "description": "Manages Lisk Commander configuration." }
+			"config": { "description": "Manages Lisk Commander configuration." },
+			"account": { "description": "Manages account." }


These are formatted differently to the message for the help command (try ./bin/run).

true, maybe something like

"config": { "description": "manage configuration for lisk" }, "account": { "description": "manage account for lisk" }

I'd rather change the help message to conform to the pattern we have here.

willclarktech · 2018-07-30T08:19:26Z

src/commands/account/create.js

+};
+
+CreateCommand.description = `
+Returns a randomly-generated mnemonic passphrase with its corresponding public key and address.


its corresponding public/private key pair and Lisk address

willclarktech · 2018-07-30T08:21:52Z

src/commands/account/get.js

+		name: 'addresses',
+		required: true,
+		description:
+			'Comma separated address(es) which you want to get the information of.',


Comma-separated address(es) to get information about.?

willclarktech · 2018-07-30T08:26:54Z

src/commands/account/get.js

+						address,
+					}));
+		const client = getAPIClient(this.userConfig.api);
+		const results = await query(client, 'accounts', req);


If I try an address which has a balance and one which doesn't I just get the error No accounts found using specified parameters. This is pretty unhelpful, but shouldn't I get information about the existing addresses without having to try again anyway?

I can think of three way of solving this, but it's not really good UX either...

Ignore errors, unless all error

create another key in the response like { data: [], errors: []}

print error and returned value

1 will be hard to tell what went wrong to the user, and
considering it will be used in script, 2 and 3 will be harder to use.

any other idea to solve this? 😢

I think the case where an address is valid but not in the blockchain is a different case to other errors. We could wrap that specific error and return an object that has no public key and no balance (therefore clear that it's never been used). With other errors it's more complicated. I guess it's more acceptable in that case to return an error if anything failed.

In case of empty response, we are creating the error on the commander side.
but im not sure it's a good idea to return an object that has no public key and no balance.
Any API change will affect this command to break, while currently it's pretty flexible

What did we do for list before?

no public key and no balance

I just meant an empty string and 0s.

current behavior is the same as before.
if we need to initialize them, we need to be fixed on those field.
maybe what we can do is to define empty object from key?
for example, we call query with object created with key. In account case, it would be
{account: '123L'} and in cause of empty response, return this

willclarktech · 2018-07-30T08:28:58Z

src/commands/account/show.js

+import BaseCommand from '../../base';
+import getInputsFromSources from '../../utils/input';
+import cryptography from '../../utils/cryptography';
+import commonOptions from '../../utils/options';


We might want to rename this commonFlags and utils/flags to be more herouku-consistent?

willclarktech · 2018-07-30T08:41:32Z

test/commands/account/get.test.js

+	});
+
+	describe('account:get account', () => {
+		const account = '3520445367460290306L';


willclarktech · 2018-07-30T08:43:57Z

test/commands/account/get.test.js

+				]);
+				return expect(printMethodStub).to.be.calledWithExactly(queryResult);
+			});
+	});


Error case? Is there a maximum number of addresses?

Currently there is no maximum (each request is separated)

willclarktech · 2018-07-30T08:54:19Z

test/commands/account/get.test.js

+		const queryResult = {
+			address: account,
+			name: 'i am owner',
+		};


Now I have to know whether I'm calling this command with one address or more than one in order to process it properly (or do some format check). If I'm using this command as part of a script that generates a variable number of addresses, that might not be obvious.

mmm maybe it will have better UX by always returning the array?

I think so.

willclarktech · 2018-07-30T09:15:03Z

test/commands/account/show.test.js

+					...defaultAddress,
+				});
+			});
+	});


What about warning if the passphrase is not a valid mnemonic? Beyond the scope of this issue?

I think #337 is the issue

willclarktech · 2018-07-30T11:45:04Z

package.json

@@ -57,8 +57,11 @@
 			"@oclif/plugin-help"
 		],
 		"topics": {
+			"help": { "description": "Displays help for Lisk Commander." },
+			"warranty": { "description": "Displays warranty notice for Lisk Commander." },
+			"copyright": { "description": "Displays copyright notice for Lisk Commander." },


All these for Lisk Commanders seem a bit redundant.

willclarktech · 2018-07-30T11:49:44Z

src/commands/account/get.js

+						address,
+					}));
+		const client = getAPIClient(this.userConfig.api);
+		const results = await query(client, 'accounts', req);


I think the case where an address is valid but not in the blockchain is a different case to other errors. We could wrap that specific error and return an object that has no public key and no balance (therefore clear that it's never been used). With other errors it's more complicated. I guess it's more acceptable in that case to return an error if anything failed.

willclarktech · 2018-07-30T11:51:54Z

test/commands/account/get.test.js

@@ -44,53 +44,57 @@ describe('account:get command', () => {
 	});

 	describe('account:get account', () => {


account:get address

willclarktech · 2018-07-30T11:53:28Z

package.json

 			"config": { "description": "Manages Lisk Commander configuration." },
-			"account": { "description": "Manages account." }
+			"account": { "description": "Manages Lisk accounts." }


Prefer alphabetical order (to match order displayed by oclif).

Addressed

willclarktech · 2018-08-01T08:24:59Z

src/commands/account/get.js

+			},
+			placeholder: {
+				address,
+				message: 'No data was returned.',


Suggest "Address not found."

willclarktech

Looking good, would just be nice to align the import style of lisk-elements/cryptography in config:set.

shuse2 added the 👁️ pending review label Jun 29, 2018

shuse2 assigned willclarktech Jun 29, 2018

shuse2 requested a review from willclarktech June 29, 2018 14:07

shuse2 assigned shuse2 and unassigned willclarktech Jun 30, 2018

shuse2 changed the base branch from 2.0.0 to 556_migrate_config_command June 30, 2018 09:39

shuse2 force-pushed the 557-migrate_account_command branch 2 times, most recently from ae21d46 to c0597d5 Compare June 30, 2018 11:12

shuse2 force-pushed the 556_migrate_config_command branch 2 times, most recently from 4669cd4 to 865d40c Compare July 16, 2018 13:22

shuse2 force-pushed the 557-migrate_account_command branch from c0597d5 to abc9bdf Compare July 18, 2018 09:35

shuse2 changed the base branch from 556_migrate_config_command to 2.0.0 July 27, 2018 11:36

shuse2 requested a review from yatki July 27, 2018 11:36

shuse2 force-pushed the 557-migrate_account_command branch from abc9bdf to 492729b Compare July 27, 2018 11:50

willclarktech suggested changes Jul 30, 2018

View reviewed changes

willclarktech previously requested changes Jul 30, 2018

View reviewed changes

willclarktech added 🏗️ in progress and removed 👁️ pending review labels Jul 30, 2018

shuse2 force-pushed the 557-migrate_account_command branch from 328ff60 to c5ba1c4 Compare July 30, 2018 14:21

shuse2 added 👁️ pending review and removed 🏗️ in progress labels Jul 30, 2018

shuse2 force-pushed the 557-migrate_account_command branch from c5ba1c4 to 9dbe2e3 Compare July 31, 2018 13:14

willclarktech reviewed Aug 1, 2018

View reviewed changes

shuse2 and others added 4 commits August 1, 2018 13:34

🌱 Migrate account commands

2599b49

🌱 Migrate account get function

31dcb88

🐛 Fix output of the address and arg name

7721eb2

♻️ Update to latest config branch

97d9e43

shuse2 added 6 commits August 1, 2018 13:34

♻️ Update name and remove object assign

5f92f3b

♻️ Update get to return array and change phrases in src and test

a6ecaa7

🌱 Add query placeholder and fix test phrases

9a8430d

🔥 Remove migrated files

5bde694

🔥 Remove usage of utils/cryptography

bc49ed7

♻️ Update placeholder message

bcf4416

shuse2 force-pushed the 557-migrate_account_command branch from 9dbe2e3 to bcf4416 Compare August 1, 2018 11:38

willclarktech suggested changes Aug 1, 2018

View reviewed changes

♻️ Update to consistent import

db6143e

willclarktech approved these changes Aug 1, 2018

View reviewed changes

willclarktech added ✅ ready and removed 👁️ pending review labels Aug 1, 2018

shuse2 merged commit 104f5f3 into 2.0.0 Aug 1, 2018

shuse2 deleted the 557-migrate_account_command branch August 1, 2018 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate account command - Closes #557 #568

Migrate account command - Closes #557 #568

shuse2 commented Jun 29, 2018 •

edited

Loading

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

shuse2 Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Jul 30, 2018

willclarktech Aug 1, 2018

willclarktech left a comment

		@@ -44,53 +44,57 @@ describe('account:get command', () => {
		});

		describe('account:get account', () => {

Migrate account command - Closes #557 #568

Migrate account command - Closes #557 #568

Conversation

shuse2 commented Jun 29, 2018 • edited Loading

Description

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willclarktech left a comment

Choose a reason for hiding this comment

shuse2 commented Jun 29, 2018 •

edited

Loading