-
Notifications
You must be signed in to change notification settings - Fork 201
Add tuple support in interactive prompts #1333
Changes from 10 commits
cc4c392
52c2376
8b77707
96371aa
d431c7f
cf380a6
6d2924a
aa53001
ab91d3d
216bfd2
d179790
70afef8
b0eeba0
20f5c19
c4e3105
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"; | ||
|
||
|
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 { | ||||||
|
@@ -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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the entire 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, in what cases is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, 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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found it weird 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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 | ||||||
|
@@ -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`); | ||||||
|
@@ -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 { | ||||||
|
@@ -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); | ||||||
} | ||||||
} | ||||||
|
@@ -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 { | ||||||
|
@@ -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 { | ||||||
|
@@ -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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think "placeholder" is very accurate. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there cases when |
||||||
return `(${components.map(c => getPlaceholder(c)).join(', ')})`; | ||||||
} else if (type === 'tuple') { | ||||||
return `(Hello world, 42)`; | ||||||
} else { | ||||||
return null; | ||||||
} | ||||||
} |
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 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).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.
Good catch!