Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Add tuple support in interactive prompts #1333

Merged
merged 15 commits into from
Dec 17, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/cli/contracts/mocks/ImplV1.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pragma solidity ^0.5.0;
pragma experimental ABIEncoderV2;

import "./Libraries.sol";
import "mock-stdlib/contracts/GreeterImpl.sol";
Expand All @@ -9,6 +10,8 @@ contract ImplV1 {

event InitializeEvent(uint256 value);

struct MyTuple { uint256 value; string text; }

function initialize(uint256 _value) public {
value = _value;
}
Expand Down Expand Up @@ -39,6 +42,10 @@ contract ImplV1 {
return numbers;
}

function echoTuple(MyTuple memory t) public pure returns (MyTuple memory) {
return t;
}

function doesNotReturn() public pure {
uint256 num = 42;
num = num + 41;
Expand Down
1 change: 1 addition & 0 deletions packages/cli/contracts/mocks/ImplV2.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pragma solidity ^0.5.0;
pragma experimental ABIEncoderV2;

import "./ImplV1.sol";

Expand Down
3 changes: 2 additions & 1 deletion packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"scripts": {
"copy-files": "./scripts/copy-files.sh",
"compile-ts": "rm -rf lib && tsc",
"compile-contracts": "rm -rf build/contracts && ../../bootstrap/node_modules/.bin/oz compile --solc-version 0.5.3 --evm-version constantinople",
"compile-contracts": "rm -rf build/contracts && ../../bootstrap/node_modules/.bin/oz compile --solc-version 0.5.14 --evm-version constantinople",
"prepare": "npm run compile-contracts && npm run compile-ts && npm run copy-files && chmod 755 ./lib/bin/oz-cli.js",
"test": "TS_NODE_PROJECT='tsconfig.test.json' mocha --require ts-node/register --recursive test",
"gen-docs": "./scripts/gen-docs.sh",
Expand Down Expand Up @@ -116,6 +116,7 @@
"lodash.uniq": "^4.5.0",
"lodash.uniqby": "^4.7.0",
"lodash.uniqwith": "^4.5.0",
"lodash.zipwith": "^4.2.0",
"npm-programmatic": "0.0.12",
"rlp": "^2.2.3",
"semver": "^5.5.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/models/network/TransactionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import ProjectFile from '../files/ProjectFile';
import NetworkFile from '../files/NetworkFile';
import { describeEvents } from '../../utils/events';

const { buildCallData, callDescription } = ABI;
const { getABIFunction, callDescription } = ABI;

interface ERC20TokenInfo {
balance?: string;
Expand Down Expand Up @@ -137,7 +137,7 @@ export default class TransactionController {
const { package: packageName, contract: contractName } = this.networkFile.getProxy(address);
const contractManager = new ContractManager(this.projectFile);
const contract = contractManager.getContractClass(packageName, contractName).at(address);
const { method } = buildCallData(contract, methodName, methodArgs);
const method = getABIFunction(contract, methodName, methodArgs);

return { contract, method };
}
Expand Down
43 changes: 9 additions & 34 deletions packages/cli/src/prompts/method-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import pickBy from 'lodash.pickby';
import isUndefined from 'lodash.isundefined';
import negate from 'lodash.negate';

import { parseMethodParams, parseArg } from '../utils/input';
import { promptIfNeeded, argsList, methodsList, InquirerQuestions } from './prompt';
import { parseMethodParams, parseArg, getPlaceholder } from '../utils/input';
import { promptIfNeeded, argsList, methodsList, InquirerQuestions, argLabel } from './prompt';

type PropsFn = (any) => InquirerQuestions;

Expand Down Expand Up @@ -59,18 +59,20 @@ function getCommandProps(
return {
...accum,
[arg.name]: {
message: arg.name ? `${arg.name} (${arg.type}):` : `(${arg.type}):`,
message: argLabel(arg),
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the previous string had a colon at the end, and argLabel doesn't (which is okay because in the other place where it's used there was no colon).

Suggested change
message: argLabel(arg),
message: argLabel(arg) + ':',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

type: 'input',
when: () => !methodArgs || !methodArgs[index],
validate: input => {
validate: (input: string) => {
try {
parseArg(input, arg.type);
parseArg(input, arg);
return true;
} catch (err) {
return `${err.message}. Enter a valid ${arg.type} such as: ${getPlaceholder(arg.type)}.`;
const placeholder = getPlaceholder(arg);
const msg = placeholder ? `Enter a valid ${arg.type} such as: ${placeholder}` : `Enter a valid ${arg.type}`;
return `${err.message}. ${msg}.`;
}
},
normalize: input => parseArg(input, arg.type),
normalize: input => parseArg(input, arg),
},
};
}, {});
Expand Down Expand Up @@ -98,30 +100,3 @@ function getCommandProps(
...args,
};
}

function getPlaceholder(type: string): string {
const ARRAY_TYPE_REGEX = /(.+)\[\d*\]$/; // matches array type identifiers like uint[] or byte[4]

if (type.match(ARRAY_TYPE_REGEX)) {
const arrayType = type.match(ARRAY_TYPE_REGEX)[1];
const itemPlaceholder = getPlaceholder(arrayType);
return `[${itemPlaceholder}, ${itemPlaceholder}]`;
} else if (
type.startsWith('uint') ||
type.startsWith('int') ||
type.startsWith('fixed') ||
type.startsWith('ufixed')
) {
return '42';
} else if (type === 'bool') {
return 'true';
} else if (type === 'bytes') {
return '0xabcdef';
} else if (type === 'address') {
return '0x1df62f291b2e969fb0849d99d9ce41e2f137006e';
} else if (type === 'string') {
return 'Hello world';
} else {
return null;
}
}
17 changes: 14 additions & 3 deletions packages/cli/src/prompts/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import isEmpty from 'lodash.isempty';
import groupBy from 'lodash.groupby';
import difference from 'lodash.difference';
import inquirer from 'inquirer';
import { contractMethodsFromAbi, ContractMethodMutability as Mutability } from '@openzeppelin/upgrades';
import { contractMethodsFromAbi, ContractMethodMutability as Mutability, ABI } from '@openzeppelin/upgrades';

import Session from '../models/network/Session';
import ConfigManager from '../models/config/ConfigManager';
Expand Down Expand Up @@ -52,6 +52,13 @@ interface MethodOptions {
constant?: boolean;
}

export interface MethodArg {
name: string;
type: string;
internalType?: string;
components?: MethodArg[];
}

export let DISABLE_INTERACTIVITY: boolean =
!process.stdin.isTTY ||
!!process.env.OPENZEPPELIN_NON_INTERACTIVE ||
Expand Down Expand Up @@ -173,7 +180,7 @@ export function methodsList(
return contractMethods(contractFullName, constant, projectFile)
.map(({ name, hasInitializer, inputs, selector }) => {
const initializable = hasInitializer ? '* ' : '';
const args = inputs.map(({ name: inputName, type }) => (inputName ? `${inputName}: ${type}` : type));
const args = inputs.map(argLabel);
const label = `${initializable}${name}(${args.join(', ')})`;

return { name: label, value: { name, selector } };
Expand All @@ -189,13 +196,17 @@ export function methodsList(
});
}

export function argLabel(arg: MethodArg): string {
return arg.name ? `${arg.name}: ${ABI.getABIType(arg)}` : ABI.getABIType(arg);
}

// Returns an inquirer question with a list of arguments for a particular method
export function argsList(
contractFullName: string,
methodIdentifier: string,
constant?: Mutability,
projectFile?: ProjectFile,
): { name: string; type: string }[] {
): MethodArg[] {
const method = contractMethods(contractFullName, constant, projectFile).find(
({ name, selector }): any => selector === methodIdentifier || name === methodIdentifier,
);
Expand Down
64 changes: 54 additions & 10 deletions packages/cli/src/utils/input.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import BN from 'bignumber.js';
import flattenDeep from 'lodash.flattendeep';
import { encodeParams, Loggy, ZWeb3 } from '@openzeppelin/upgrades';
import { MethodArg } from '../prompts/prompt';
import zipWith from 'lodash.zipwith';

// TODO: Deprecate in favor of a combination of parseArg and parseArray
export function parseArgs(args: string): string[] | never {
Expand Down Expand Up @@ -46,18 +48,24 @@ function quoteArguments(args: string) {
return args;
}

export function parseArg(input: string | string[], type: string): any {
export function parseArg(input: string | string[], { type, components }: Pick<MethodArg, 'type' | 'components'>): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the entire MethodArg type here? I imagine that using Pick simplifies the tests? Is there another reason?

If we insist on not using the entire type, I think we should define a type alias in this file. It represents an important concept, and it's used twice in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was in the recursive call, since the types for an array (for example) are not named. I agree with giving it a proper name, just created MethodArgType for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, in what cases is input an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, parseArray does not parse just the top-level arrays, but it returns arrays of arrays (as many levels deep as needed). So, the parseArg recursive call in the case of arrays may be done with an array instead of a string.

And writing this I'm thinking that maybe arrays of tuples (or viceversa) may not work...

const TRUE_VALUES = ['y', 'yes', 't', 'true', '1'];
const FALSE_VALUES = ['n', 'no', 'f', 'false', '0'];
const ARRAY_TYPE_REGEX = /(.+)\[\d*\]$/; // matches array type identifiers like uint[] or byte[4]

// TODO: Handle tuples in ABI specification
// Tuples: recursively parse
if (type === 'tuple') {
const inputs = typeof input === 'string' ? parseArray(stripParens(input), '(', ')') : input;
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it weird that parseArray wouldn't handle the delimiters itself. What do you think about doing that?

Also, why do we allow the input to not be surrounded by delimiters? In what cases is that used? Does that apply to tuples in the same way that it applies to arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found it weird that parseArray wouldn't handle the delimiters itself. What do you think about doing that?

Good point, I think it could be refactored. That said, given that this routine is pretty isolated (and working!), I wouldn't risk changing it now.

Also, why do we allow the input to not be surrounded by delimiters? In what cases is that used? Does that apply to tuples in the same way that it applies to arrays?

When the user is prompted to enter an array, they can just enter a comma separated list, without the brackets, for simplicity. Same applies for tuples.

if (inputs.length !== components.length)
throw new Error(`Expected ${components.length} values but got ${input.length}`);
return zipWith(inputs, components, parseArg);
frangio marked this conversation as resolved.
Show resolved Hide resolved
}

// Arrays: recursively parse
if (type.match(ARRAY_TYPE_REGEX)) {
else if (type.match(ARRAY_TYPE_REGEX)) {
const arrayType = type.match(ARRAY_TYPE_REGEX)[1];
const inputs = typeof input === 'string' ? parseArray(stripBrackets(input)) : input;
return inputs.map(input => parseArg(input, arrayType));
return inputs.map(input => parseArg(input, { type: arrayType }));
}

// Integers: passed via bignumber to handle signs and scientific notation
Expand Down Expand Up @@ -110,6 +118,10 @@ export function stripBrackets(inputMaybeWithBrackets: string): string {
return `${inputMaybeWithBrackets.replace(/^\s*\[/, '').replace(/\]\s*$/, '')}`;
}

export function stripParens(inputMaybeWithParens: string): string {
return `${inputMaybeWithParens.replace(/^\s*\(/, '').replace(/\)\s*$/, '')}`;
}

function requireInputString(arg: string | string[]): arg is string {
if (typeof arg !== 'string') {
throw new Error(`Expected ${flattenDeep(arg).join(',')} to be a scalar value but was an array`);
Expand All @@ -126,7 +138,7 @@ function requireInputString(arg: string | string[]): arg is string {
* with arbitrarily nested string arrays, but ts has a hard time handling that,
* so we're fooling it into thinking it's just one level deep.
*/
export function parseArray(input: string): (string | string[])[] {
export function parseArray(input: string, open = '[', close = ']'): (string | string[])[] {
let i = 0; // index for traversing input

function innerParseQuotedString(quoteChar: string): string {
Expand All @@ -148,9 +160,9 @@ export function parseArray(input: string): (string | string[])[] {
return input.slice(start, i - 1);
} else if (char === '"' || char === "'") {
throw new Error(`Unexpected quote at position ${i}`);
} else if (char === '[' || char === "'") {
} else if (char === open || char === "'") {
throw new Error(`Unexpected opening bracket at position ${i}`);
} else if (char === ']') {
} else if (char === close) {
return input.slice(start, --i);
}
}
Expand All @@ -164,7 +176,7 @@ export function parseArray(input: string): (string | string[])[] {
continue;
} else if (char === ',') {
return;
} else if (char === ']') {
} else if (char === close) {
i--;
return;
} else {
Expand All @@ -183,11 +195,11 @@ export function parseArray(input: string): (string | string[])[] {
} else if (char === '"' || char === "'") {
result.push(innerParseQuotedString(char));
requireCommaOrClosing();
} else if (char === '[') {
} else if (char === open) {
const innerArray = innerParseArray();
result.push(innerArray);
requireCommaOrClosing();
} else if (char === ']') {
} else if (char === close) {
if (!requireClosingBracket) throw new Error(`Unexpected closing array at position ${i + 1} in ${input}`);
return result;
} else {
Expand Down Expand Up @@ -238,3 +250,35 @@ export function validateSalt(salt: string, required = false) {
throw new Error(`Invalid salt ${salt}, must be an uint256 value.`);
}
}

export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think "placeholder" is very accurate. What about getSampleInput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like it! The original name was placeholder because the idea was to use it as a placeholder when presenting the question, but it was after that we noted that inquirer does not support placeholders (it only displays them when they are the default value).

Copy link
Contributor

Choose a reason for hiding this comment

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

The compiler isn't catching this because we're not running it in strict mode. It will be a bit of an undertaking to enable it, but I would highly recommend we do.

Suggested change
export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string {
export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string | null {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one. And agree on the strict mode!

const ARRAY_TYPE_REGEX = /(.+)\[\d*\]$/; // matches array type identifiers like uint[] or byte[4]
const { type, components } = arg;

if (type.match(ARRAY_TYPE_REGEX)) {
const arrayType = type.match(ARRAY_TYPE_REGEX)[1];
const itemPlaceholder = getPlaceholder({ type: arrayType });
return `[${itemPlaceholder}, ${itemPlaceholder}]`;
} else if (
type.startsWith('uint') ||
type.startsWith('int') ||
type.startsWith('fixed') ||
type.startsWith('ufixed')
) {
return '42';
} else if (type === 'bool') {
return 'true';
} else if (type === 'bytes') {
return '0xabcdef';
} else if (type === 'address') {
return '0x1df62f291b2e969fb0849d99d9ce41e2f137006e';
} else if (type === 'string') {
return 'Hello world';
} else if (type === 'tuple' && components) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there cases when components is undefined?

return `(${components.map(c => getPlaceholder(c)).join(', ')})`;
} else if (type === 'tuple') {
return `(Hello world, 42)`;
} else {
return null;
}
}
4 changes: 2 additions & 2 deletions packages/cli/test/prompts/prompt.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,15 +257,15 @@ describe('prompt', function() {
it('returns an array of method arguments names', function() {
const args = argsList('Greeter', 'greet(string)', Mutability.NotConstant, this.projectFile);
args.should.be.an('array');
args[0].should.deep.equal({ name: 'who', type: 'string' });
args[0].should.deep.include({ name: 'who', type: 'string' });
});
});

context('when the argument has no name', function() {
it('returns an array of method arguments names', function() {
const args = argsList('Greeter', 'greetings(uint256)', Mutability.Constant, this.projectFile);
args.should.be.an('array');
args[0].should.deep.equal({ name: '#0', type: 'uint256' });
args[0].should.deep.include({ name: '#0', type: 'uint256' });
});
});
});
Expand Down
20 changes: 20 additions & 0 deletions packages/cli/test/scripts/call.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,26 @@ describe('call script', function() {
this.logs.infos[this.logs.infos.length - 1].should.eq(`Method 'sayNumbers' returned: [1,2,3]`);
});
});

context('when the method takes and returns a struct', function() {
it('calls the method and logs the struct', async function() {
const proxyAddress = this.networkFile.getProxies({
contract: 'Impl',
})[0].address;
await call({
network,
txParams,
networkFile: this.networkFile,
proxyAddress,
methodName: 'echoTuple((uint256,string))',
methodArgs: [['42', 'Hello']],
});

this.logs.infos[this.logs.infos.length - 1].should.eq(
`Method 'echoTuple((uint256,string))' returned: [42,Hello]`,
);
});
});
});
});
});
Loading