Skip to content
This repository has been archived by the owner. It is now read-only.

Create transaction register second passhrase - Closes #249 #303

Merged
merged 14 commits into from Oct 26, 2017

Conversation

2 participants
@Tosch110
Copy link
Contributor

commented Oct 24, 2017

Closes #249

Tobias Schwarz added some commits Oct 20, 2017

Tobias Schwarz
Tobias Schwarz
Tobias Schwarz
Tobias Schwarz
Tobias Schwarz
@willclarktech
Copy link
Contributor

left a comment

Looking good!

@@ -11,6 +11,15 @@ const passphraseDescription = `Specifies a source for your secret passphrase. Li
- --passphrase stdin (takes the first line only)
`;

const secondPassphraseDescription = `Specifies a source for your secret second passphrase. Lisky will prompt you for input if this option is not set. Source must be one of \`env\`, \`file\` or \`stdin\`. Except for \`stdin\`, a corresponding identifier must also be provided.

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

for your second secret passphrase is more natural.

Examples:
- --second-passphrase 'pass:my secret passphrase' (should only be used where security is not important)
- --second-passphrase env:SECRET_PASSPHRASE
- --second-passphrase file:/path/to/my/passphrase.txt (takes the first line only)

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Examples should be specific to second passphrase.

export const getPassphrase = async (vorpal, passphraseSource, passphrase, options) => {
const optionsWithDefaults = Object.assign({ displayName: 'your secret passphrase' }, options);
export const getPassphrase = async (
vorpal, passphraseSource, passphrase, options, displayPrefix,

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Why not just pass a displayName option instead of adding a whole extra parameter?

@@ -33,6 +42,7 @@ const options = {
json: ['-j, --json', jsonDescription],
noJson: ['-t, --no-json', noJsonDescription],
passphrase: ['-p, --passphrase <source>', passphraseDescription],
secondPassphrase: ['-s, --second-passphrase <source>', secondPassphraseDescription],

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Do you think this is a helpful alias?

This comment has been minimized.

Copy link
@Tosch110

Tosch110 Oct 25, 2017

Author Contributor

yes :)

@@ -17,3 +17,11 @@ import lisk from 'lisk-js';
import config from './env';

export default lisk.api(config.liskJS);

export const createLiskTransaction = Object.assign({},

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Let's rename this module api.js and move transactions into transactions.js?


export function itShouldResolveToTheCreatedTransaction() {
const { returnValue, createdTransaction } = this.test.ctx;
(returnValue).should.be.fulfilledWith(createdTransaction);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

This is secretly async so return is actually critical here to avoid a race condition.


describe('Given a lisk transaction object', () => {
beforeEach(given.aLiskTransactionObject);
it('Then the lisk transaction object should have transaction creation functions', then.theLiskTransactionObjectShouldHaveTransactionCreationFunctions);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

This is a little obscure. Is there a reason not to specify which ones (in separate tests)?

return (_aliases).should.be.eql([alias]);
}

export function theLiskTransactionObjectShouldHaveTransactionCreationFunctions() {

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Step says they're functions but this isn't tested.

import commonOptions from '../utils/options';
import { createLiskTransaction } from '../utils/liskInstance';

const description = `Creates a register second passphrase transaction for an existing account.

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Creates a transaction which will register a second passphrase for an existing account if broadcast to the network.

commonOptions.passphrase,
commonOptions.secondPassphrase,
],
errorPrefix: 'Could not create transaction',

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 24, 2017

Contributor

Should be specific to the action

@willclarktech willclarktech added this to Pull Requests in Sprint Board 23-10-17 Oct 25, 2017

@willclarktech willclarktech added this to Pull Requests in Version 0.3.0 Oct 25, 2017

Tobias Schwarz added some commits Oct 25, 2017

Tobias Schwarz
Tobias Schwarz

Tobias Schwarz added some commits Oct 25, 2017

Tobias Schwarz
Tobias Schwarz

@Tosch110 Tosch110 force-pushed the 249-create_transaction_register_second_passhrase branch from a4a9c41 to bd1f373 Oct 25, 2017

@willclarktech
Copy link
Contributor

left a comment

A few minor comments, and a general point that will affect the main command test suite - the structure of the command is like this:

  1. Maybe get inputs of two sorts from StdIn and return one or either/neither of them.
  2. Get the passphrase now that we have all the possible sources available.
  3. Get the second passphrase now that we have all the possible sources available.
  4. Create the transaction.
  5. Resolve.

Steps 2 and 3 are not being reliably tested as things stand.

- --second-passphrase 'pass:my second secret passphrase' (should only be used where security is not important)
- --second-passphrase env:SECOND_SECRET_PASSPHRASE
- --second-passphrase file:/path/to/my/secondPassphrase.txt (takes the first line only)
- --second-passphrase stdin (takes the first line only)

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

takes one line only - this might have to change in the passphraseDescription too.

@@ -13,7 +13,7 @@
* Removal or modification of this copyright notice is prohibited.
*
*/
import lisk from './liskInstance';
import lisk from './api';

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

import client from './api' or import liskAPIInstance from './api'

import lisk from 'lisk-js';

// eslint-disable-next-line import/prefer-default-export
export const liskTransaction = Object.assign({},

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

export default const transactions

it('Then it should get the passphrase using the passphrase from stdin', then.itShouldGetThePassphraseUsingThePassphraseFromStdIn);
it('Then it should get the second passphrase using the vorpal instance', then.itShouldGetTheSecondPassphraseUsingTheVorpalInstance);
it('Then it should get the second passphrase with a repeated prompt', then.itShouldGetTheSecondPassphraseWithARepeatedPrompt);
it('Then it should create a register second passphrase transaction using the passphrase and the second passphrase', then.itShouldHaveCreatedARegisterSecondPassphraseTransactionUsingThePassphraseAndTheSecondPassphrase);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

I prefer this description to the should have created version, but the function name diverges currently.

@@ -16,9 +16,9 @@
import * as given from '../../steps/1_given';
import * as then from '../../steps/3_then';

describe('liskInstance util', () => {
describe('api util', () => {
describe('Given a lisk instance', () => {

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

Description diverges from function name

@@ -19,15 +19,15 @@ import * as then from '../../steps/3_then';

describe('Query class', () => {
describe('Given a lisk instance', () => {

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

Description diverges from function name

describe('transactions util', () => {
describe('Given a lisk transaction object', () => {
beforeEach(given.aLiskTransactionObject);
it('Then it should have the transaction create transaction', then.itShouldHaveTheTransactionCreateTransaction);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

This might be more a problem with lisk-js 0.5.0 (and solved via the renaming in 1.0.0) but I don't understand what these tests mean by reading this specification.

Tobias Schwarz
@willclarktech
Copy link
Contributor

left a comment

Two minor points

it('Then it should have the transaction create vote', then.itShouldHaveTheTransactionCreateVote);
describe('Given a transaction object', () => {
beforeEach(given.aTransactionsObject);
it('Then it should have the transaction transfer', then.itShouldHaveTheTransactionTransfer);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

Still not totally obvious to me what these mean. How about Then it should have a function to create a "transfer" transaction, with a step definition which can be reused for all of these? (But in that case it should also test that they are functions.)

@@ -72,9 +73,9 @@ describe('create transaction register second passphrase command', () => {
beforeEach(when.theActionIsCalledWithTheOptions);
it('Then it should get the passphrase from stdin', then.itShouldGetThePassphraseFromStdIn);
it('Then it should get the passphrase using the passphrase from stdin', then.itShouldGetThePassphraseUsingThePassphraseFromStdIn);
it('Then it should not get the second passphrase from stdin', then.itShouldNotGetTheSecondPassphraseFromStdIn);

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

We need a test for repeated prompt here too.

Tobias Schwarz
case 2: return 'createDelegate';
case 3: return 'createVote';
case 4: return 'createMultisignature';
default: return 'notIncludedType';

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

Prefer throwing an error otherwise we'll be making nonsensical assertions.

@@ -145,3 +145,14 @@ export const createStreamStub = on => ({
close: () => {},
on,
});

export function getTransactionFunctionNameByType(transactionType) {

This comment has been minimized.

Copy link
@willclarktech

willclarktech Oct 26, 2017

Contributor

getTransactionCreatorFunctionNameByType

Tobias Schwarz
@willclarktech
Copy link
Contributor

left a comment

Nice work

@willclarktech willclarktech merged commit 3236c22 into 0.3.0 Oct 26, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@willclarktech willclarktech deleted the 249-create_transaction_register_second_passhrase branch Oct 26, 2017

@willclarktech willclarktech moved this from Pull Requests to Merged Pull Requests in Sprint Board 23-10-17 Oct 26, 2017

@willclarktech willclarktech moved this from Pull Requests to Merged Pull Requests in Version 0.3.0 Oct 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.