From 4c1cbb6c7ae58169026a94af34f22053099cacd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 10:42:07 +0200 Subject: [PATCH 01/14] Loosen `EventCallback` type * The `EventCallback` type might be used directly to return something; loosen the typing --- packages/colony-js-adapter/interface/EventHandlers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/colony-js-adapter/interface/EventHandlers.js b/packages/colony-js-adapter/interface/EventHandlers.js index ef449465d..5237ddcff 100644 --- a/packages/colony-js-adapter/interface/EventHandlers.js +++ b/packages/colony-js-adapter/interface/EventHandlers.js @@ -2,7 +2,7 @@ import type { Contract } from './Contract'; -export type EventCallback = (...*) => void; +export type EventCallback = (...*) => *; export type EventHandler = { contract: Contract, From 1383db87b9f99d51642da0d248f0b338942c0bf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 10:44:11 +0200 Subject: [PATCH 02/14] Add `CreateTask` Sender --- CHANGELOG.md | 1 + .../src/ColonyClient/index.js | 7 +++- .../src/ColonyClient/senders/CreateTask.js | 34 +++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 packages/colony-js-client/src/ColonyClient/senders/CreateTask.js diff --git a/CHANGELOG.md b/CHANGELOG.md index abaf8830f..8e61ac373 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ **Maintenance** +* Add extra validation for `ColonyClient.createTask`; the given domain ID for the task is now confirmed to exist before proceeding (`@colony/colony-js-client`) * Add a `DomainAdded` event, which is emitted when calling `ColonyClient.addDomain` (`@colony/colony-js-client`) * Add a `PotAdded` event, which is emitted when calling `ColonyClient.addDomain` and `ColonyClient.createTask` (`@colony/colony-js-client`) * Add the `TaskDeliverableSubmitted` and `TaskWorkRatingRevealed` events, which are available to listen on via `ColonyClient.events` (`@colony/colony-js-client`) diff --git a/packages/colony-js-client/src/ColonyClient/index.js b/packages/colony-js-client/src/ColonyClient/index.js index 09f483873..80a944f97 100644 --- a/packages/colony-js-client/src/ColonyClient/index.js +++ b/packages/colony-js-client/src/ColonyClient/index.js @@ -13,6 +13,7 @@ import ColonyNetworkClient from '../ColonyNetworkClient/index'; import TokenClient from '../TokenClient/index'; import AuthorityClient from '../AuthorityClient/index'; import GetTask from './callers/GetTask'; +import CreateTask from './senders/CreateTask'; import { ROLES, WORKER_ROLE, @@ -824,9 +825,13 @@ export default class ColonyClient extends ContractClient { ['token', 'tokenAddress'], ], }); - this.addSender('createTask', { + this.createTask = new CreateTask({ + client: this, + name: 'createTask', functionName: 'makeTask', input: [ + // Flow hates you for using an optional last parameter in a tuple + // $FlowFixMe ['specificationHash', 'ipfsHash'], ['domainId', 'number', DEFAULT_DOMAIN_ID], ], diff --git a/packages/colony-js-client/src/ColonyClient/senders/CreateTask.js b/packages/colony-js-client/src/ColonyClient/senders/CreateTask.js new file mode 100644 index 000000000..381ebb2b6 --- /dev/null +++ b/packages/colony-js-client/src/ColonyClient/senders/CreateTask.js @@ -0,0 +1,34 @@ +/* @flow */ +/* eslint-disable no-underscore-dangle */ + +import ContractClient from '@colony/colony-js-contract-client'; +import type ColonyClient from '../index'; + +type InputValues = { + specificationHash: string, + domainId: number, +}; + +type OutputValues = { + id: number, +}; + +// XXX This is a good use-case for some kind of async validation step, +// but since the underlying method functionality is due to change very soon, +// we're opting to not make big changes to the Sender behaviour, and simply +// extend the `send` method to perform this async validation. +export default class CreateTask extends ContractClient.Sender< + InputValues, + OutputValues, + ColonyClient, +> { + async send(inputValues: InputValues, options: *) { + // Validate that the domain exists before attempting to create a task + if (Object.hasOwnProperty.call(inputValues, 'domainId')) { + const { count } = await this.client.getDomainCount.call(); + if (count < inputValues.domainId) + throw new Error(`Domain ID ${inputValues.domainId} not found`); + } + return super.send(inputValues, options); + } +} From 83a80761aaa72eeb5397f3aedb884ab2ef402209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:20:45 +0200 Subject: [PATCH 03/14] Upgrade flow to 0.75 --- package.json | 4 ++-- yarn.lock | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index c9cb76c58..582aac4f4 100644 --- a/package.json +++ b/package.json @@ -20,9 +20,9 @@ "eslint-plugin-flowtype": "^2.44.0", "eslint-plugin-import": "^2.8.0", "eslint-plugin-prettier": "^2.6.0", - "flow-bin": "^0.72.0", + "flow-bin": "^0.75.0", "flow-copy-source": "^1.2.1", - "flow-parser": "^0.73.0", + "flow-parser": "^0.75.0", "husky": "^0.14.3", "lerna": "^2.5.1", "lint-staged": "^6.1.1", diff --git a/yarn.lock b/yarn.lock index fd6a3942f..d7557fe7d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3038,6 +3038,10 @@ flow-bin@^0.73.0: version "0.73.0" resolved "https://registry.yarnpkg.com/flow-bin/-/flow-bin-0.73.0.tgz#da1b90a02b0ef9c439f068c2fc14968db83be425" +flow-bin@^0.75.0: + version "0.75.0" + resolved "https://registry.yarnpkg.com/flow-bin/-/flow-bin-0.75.0.tgz#b96d1ee99d3b446a3226be66b4013224ce9df260" + flow-copy-source@^1.2.1: version "1.3.0" resolved "https://registry.yarnpkg.com/flow-copy-source/-/flow-copy-source-1.3.0.tgz#591b153f5c01e8fc566c64a97290ea9103b7f1ea" @@ -3062,9 +3066,9 @@ flow-parser@^0.*: version "0.69.0" resolved "https://registry.yarnpkg.com/flow-parser/-/flow-parser-0.69.0.tgz#378b5128d6d0b554a8b2f16a4ca3e1ab9649f00e" -flow-parser@^0.73.0: - version "0.73.0" - resolved "https://registry.yarnpkg.com/flow-parser/-/flow-parser-0.73.0.tgz#525ac0776f743e16b6dca1a3dd6c602260b15773" +flow-parser@^0.75.0: + version "0.75.0" + resolved "https://registry.yarnpkg.com/flow-parser/-/flow-parser-0.75.0.tgz#9a1891c48051c73017b6b5cc07b3681fda3fdfb0" flow-remove-types@^1.2.3: version "1.2.3" From 9d7e7ae07d67dba75f5ba6e8cec37170d91b2599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:21:12 +0200 Subject: [PATCH 04/14] Split up flowtypes --- .../src/flowtypes/index.js | 26 ++++++++++ .../{flowtypes.js => flowtypes/methods.js} | 51 +++---------------- .../src/flowtypes/params.js | 22 ++++++++ .../src/flowtypes/signatures.js | 22 ++++++++ 4 files changed, 76 insertions(+), 45 deletions(-) create mode 100644 packages/colony-js-contract-client/src/flowtypes/index.js rename packages/colony-js-contract-client/src/{flowtypes.js => flowtypes/methods.js} (68%) create mode 100644 packages/colony-js-contract-client/src/flowtypes/params.js create mode 100644 packages/colony-js-contract-client/src/flowtypes/signatures.js diff --git a/packages/colony-js-contract-client/src/flowtypes/index.js b/packages/colony-js-contract-client/src/flowtypes/index.js new file mode 100644 index 000000000..1d05a6f99 --- /dev/null +++ b/packages/colony-js-contract-client/src/flowtypes/index.js @@ -0,0 +1,26 @@ +/* @flow */ + +export type { + ContractClientConstructorArgs, + ContractMethodArgs, + ContractMethodDef, + ContractMethodMultisigSenderArgs, + ContractMethodSenderArgs, + ContractResponse, + ContractResponseMeta, + DefaultValues, + GetRequiredSignees, + MultisigOperationConstructorArgs, + MultisigOperationPayload, + SendOptions, + ValidateEmpty, +} from './methods'; + +export type { Param, Params, ParamTypeDef, ParamTypes } from './params'; + +export type { + SigningMode, + Signature, + Signers, + CombinedSignatures, +} from './signatures'; diff --git a/packages/colony-js-contract-client/src/flowtypes.js b/packages/colony-js-contract-client/src/flowtypes/methods.js similarity index 68% rename from packages/colony-js-contract-client/src/flowtypes.js rename to packages/colony-js-contract-client/src/flowtypes/methods.js index f1728c7ad..a28ca2aef 100644 --- a/packages/colony-js-contract-client/src/flowtypes.js +++ b/packages/colony-js-contract-client/src/flowtypes/methods.js @@ -1,7 +1,6 @@ /* @flow */ import type BigNumber from 'bn.js'; - import type { EventHandlers, IAdapter, @@ -9,34 +8,14 @@ import type { TransactionOptions, TransactionReceipt, } from '@colony/colony-js-adapter'; - import type { Query } from '@colony/colony-js-contract-loader'; -import ContractClient from './classes/ContractClient'; -import { SIGNING_MODES } from './constants'; - -export type ParamTypes = - | 'address' - | 'bigNumber' - | 'boolean' - | 'date' - | 'hexString' - | 'ipfsHash' - | 'number' - | 'tokenAddress' - | 'string'; +import ContractClient from '../classes/ContractClient'; +import type { Params } from './params'; +import type { Signers } from './signatures'; -// [param name, param type, default value (optional)] -export type Param = [string, ParamTypes, *]; -export type EventParam = [string, ParamTypes]; - -export type Params = Array; -export type EventParams = Array; - -export type ParamTypeDef = { - validate: (value: any) => boolean, - convertOutput: (value: any) => *, - convertInput: (value: any) => *, +export type DefaultValues = { + [inputName: string]: *, }; export type SendOptions = { @@ -76,6 +55,7 @@ export type ContractMethodArgs = { input: Params, output?: Params, validateEmpty?: ValidateEmpty, + defaultValues?: DefaultValues, }; export type ContractMethodSenderArgs = { @@ -101,25 +81,6 @@ export type ContractMethodDef = { output?: Params, }; -export type SigningMode = $Values; - -export type Signature = { - sigR: string, - sigS: string, - sigV: number, -}; - -export type Signers = { - [signeeAddress: string]: Signature & { mode: SigningMode }, -}; - -export type CombinedSignatures = { - sigR: Array, - sigS: Array, - sigV: Array, - mode: Array, -}; - export type MultisigOperationPayload = { data: string, destinationAddress: string, diff --git a/packages/colony-js-contract-client/src/flowtypes/params.js b/packages/colony-js-contract-client/src/flowtypes/params.js new file mode 100644 index 000000000..8541107de --- /dev/null +++ b/packages/colony-js-contract-client/src/flowtypes/params.js @@ -0,0 +1,22 @@ +/* @flow */ + +export type ParamTypes = + | 'address' + | 'bigNumber' + | 'boolean' + | 'date' + | 'hexString' + | 'ipfsHash' + | 'number' + | 'tokenAddress' + | 'string'; + +// [param name, param type, default value (optional)] +export type Param = [string, ParamTypes, *]; +export type Params = Array; + +export type ParamTypeDef = { + validate: (value: any) => boolean, + convertOutput: (value: any) => *, + convertInput: (value: any) => *, +}; diff --git a/packages/colony-js-contract-client/src/flowtypes/signatures.js b/packages/colony-js-contract-client/src/flowtypes/signatures.js new file mode 100644 index 000000000..67464fdf8 --- /dev/null +++ b/packages/colony-js-contract-client/src/flowtypes/signatures.js @@ -0,0 +1,22 @@ +/* @flow */ + +import { SIGNING_MODES } from '../constants'; + +export type SigningMode = $Values; + +export type Signature = { + sigR: string, + sigS: string, + sigV: number, +}; + +export type Signers = { + [signeeAddress: string]: Signature & { mode: SigningMode }, +}; + +export type CombinedSignatures = { + sigR: Array, + sigS: Array, + sigV: Array, + mode: Array, +}; From 2473febaaf852f31b0bdd6174e4299dad03f9a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:53:14 +0200 Subject: [PATCH 05/14] Remove param normalisation --- .../src/classes/ContractEvent.js | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/packages/colony-js-contract-client/src/classes/ContractEvent.js b/packages/colony-js-contract-client/src/classes/ContractEvent.js index 626e1b889..c6a7c6591 100644 --- a/packages/colony-js-contract-client/src/classes/ContractEvent.js +++ b/packages/colony-js-contract-client/src/classes/ContractEvent.js @@ -5,18 +5,11 @@ import ContractClient from '../classes/ContractClient'; import { convertOutputValues } from '../modules/paramConversion'; import { validateParams } from '../modules/paramValidation'; -import type { Params, EventParams } from '../flowtypes'; +import type { Params } from '../flowtypes'; type AssertionMethod = (assertion: boolean, reason: string) => any; type TypedEventCallback = (args: ParamTypes) => void; -const normalizeEventParams = (spec: EventParams): Params => - spec.map(([parameterName, parameterType]) => [ - parameterName, - parameterType, - undefined, - ]); - export default class ContractEvent { // The event's name as defined in the contract. eventName: string; @@ -40,11 +33,11 @@ export default class ContractEvent { }: { eventName: string, client: ContractClient, - argsDef: EventParams, + argsDef: Params, }) { this.eventName = eventName; this.client = client; - this.argsDef = normalizeEventParams(argsDef); + this.argsDef = argsDef; this._wrappedHandlers = new Map(); this.assertValid = makeAssert(`Validation failed for event ${eventName}`); From 270359700b7cb6faa004638d24e849182529c7ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:53:33 +0200 Subject: [PATCH 06/14] Pass down assertValid --- .../colony-js-contract-client/src/modules/paramValidation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/colony-js-contract-client/src/modules/paramValidation.js b/packages/colony-js-contract-client/src/modules/paramValidation.js index cdcaa04bb..c99e5d6c9 100644 --- a/packages/colony-js-contract-client/src/modules/paramValidation.js +++ b/packages/colony-js-contract-client/src/modules/paramValidation.js @@ -75,6 +75,6 @@ export function validateParams( ); return spec.every(paramSpec => - validateValue(inputValues[paramSpec[0]], paramSpec), + validateValue(inputValues[paramSpec[0]], paramSpec, assertValid), ); } From 441d569fc831ea033a02d4bdc906022c89f83965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:54:21 +0200 Subject: [PATCH 07/14] Remove applying defaults * Since `Param` no longer contains a default value, don't apply default values when converting input values --- .../src/modules/paramConversion.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/colony-js-contract-client/src/modules/paramConversion.js b/packages/colony-js-contract-client/src/modules/paramConversion.js index f285b3aed..bff4cf320 100644 --- a/packages/colony-js-contract-client/src/modules/paramConversion.js +++ b/packages/colony-js-contract-client/src/modules/paramConversion.js @@ -11,13 +11,8 @@ export function convertInputValues( inputValues: InputTypes, valuesSpec: Params, ) { - return valuesSpec.map(([paramName, paramType, defaultValue]) => - convertInputValue( - Object.hasOwnProperty.call(inputValues, paramName) - ? inputValues[paramName] - : defaultValue, - paramType, - ), + return valuesSpec.map(([paramName, paramType]) => + convertInputValue(inputValues[paramName], paramType), ); } From 98c21c8990a846f0ba945527bc02d24c60b4dbb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:54:51 +0200 Subject: [PATCH 08/14] `EventParams` -> `Params` --- .../colony-js-contract-client/src/classes/ContractClient.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/colony-js-contract-client/src/classes/ContractClient.js b/packages/colony-js-contract-client/src/classes/ContractClient.js index bad118d29..0db822726 100644 --- a/packages/colony-js-contract-client/src/classes/ContractClient.js +++ b/packages/colony-js-contract-client/src/classes/ContractClient.js @@ -17,7 +17,7 @@ import ContractMethodMultisigSender from './ContractMethodMultisigSender'; import type { ContractMethodDef, ContractClientConstructorArgs, - EventParams, + Params, } from '../flowtypes'; export default class ContractClient { @@ -153,7 +153,7 @@ export default class ContractClient { * Add event subscription functionality for a particular event of this * contract to the given ContractClient instance. */ - addEvent(eventName: string, argsDef: EventParams): void { + addEvent(eventName: string, argsDef: Params): void { if (Reflect.has(this.events, eventName)) { throw new Error(`An event named "${eventName}" already exists`); } From f8df252cbe75e7160bece6ce527b2a081948b893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:56:33 +0200 Subject: [PATCH 09/14] Apply default values * Specify an optional `defaultValues` property for `ContractMethod` * Apply the default values in `validate` and `getValidatedArgs` (if params are specified) with `_getDefaultValues` * `convertInputValues` no longer applies default values --- .../src/classes/ContractMethod.js | 63 ++++++++++++++++--- 1 file changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/colony-js-contract-client/src/classes/ContractMethod.js b/packages/colony-js-contract-client/src/classes/ContractMethod.js index 61cf0e8d2..804d872b0 100644 --- a/packages/colony-js-contract-client/src/classes/ContractMethod.js +++ b/packages/colony-js-contract-client/src/classes/ContractMethod.js @@ -10,7 +10,7 @@ import { convertOutputValues, } from '../modules/paramConversion'; import { validateParams } from '../modules/paramValidation'; -import type { ContractMethodArgs, Params } from '../flowtypes'; +import type { ContractMethodArgs, DefaultValues, Params } from '../flowtypes'; /** * Abstract class for interacting with contract methods. @@ -22,13 +22,36 @@ export default class ContractMethod< > { assertValid: Function; client: IContractClient; + defaultValues: DefaultValues; functionName: string; input: Params; name: string; output: Params; + /** + * Given input values, method parameters and default values, iterate through + * the parameters and construct and object with the properties from the + * input values (if they are defined) or default values. + */ + static _getDefaultValues( + inputValues: * = {}, + params: Params, + defaultValues: DefaultValues = {}, + ) { + return params.reduce( + (acc, [name]) => + Object.assign(acc, { + [name]: Object.hasOwnProperty.call(inputValues, name) + ? inputValues[name] + : defaultValues[name], + }), + {}, + ); + } + constructor({ client, + defaultValues, functionName, name, input, @@ -39,6 +62,7 @@ export default class ContractMethod< this.input = input; this.functionName = functionName; this.assertValid = makeAssert(`Validation failed for ${name}`); + if (defaultValues) this.defaultValues = defaultValues; if (output) this.output = output; } @@ -46,7 +70,7 @@ export default class ContractMethod< * Given named input values, transform these with the expected parameters * in order to get an array of arguments expected by the contract function. */ - _getMethodArgs(inputValues?: InputValues, params?: Params): Array { + _getMethodArgs(inputValues?: *, params?: Params): Array { let args = []; if (inputValues == null) return args; @@ -75,16 +99,28 @@ export default class ContractMethod< } /** - * Given input values, validate them against the expected params + * Given input values, apply default values and validate them against the + * expected params */ - validate(inputValues?: any, params: Params = this.input) { + validate( + inputValues?: any, + params: Params = this.input, + defaultValues: DefaultValues = this.defaultValues, + ) { + const values = + params && params.length + ? this.constructor._getDefaultValues(inputValues, params, defaultValues) + : inputValues; + return this._validate(values); + } + + _validate(inputValues?: any, params: Params = this.input) { return validateParams(inputValues, params, this.assertValid); } /** * Given input values, map them against the expected parameters, * with the appropriate conversion for each type. - * Fall back to default values for each parameter. */ convertInputValues( inputValues: InputValues, @@ -114,10 +150,19 @@ export default class ContractMethod< } /** - * Given input values, validate them and return parsed method args. + * Given input values, get default values (if params are given), then + * validate them and return parsed method args. */ - getValidatedArgs(inputValues?: any, params: Params = this.input) { - this.validate(inputValues, params); - return this._getMethodArgs(inputValues, params); + getValidatedArgs( + inputValues: any, + params: Params = this.input, + defaultValues: DefaultValues = this.defaultValues, + ) { + const values = + params && params.length + ? this.constructor._getDefaultValues(inputValues, params, defaultValues) + : inputValues; + this._validate(values, params); + return this._getMethodArgs(values, params); } } From 371313b74ead3eb276ece560e1b3330c0972273b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:56:47 +0200 Subject: [PATCH 10/14] Update tests --- .../src/__tests__/ContractMethod.js | 39 ++++++++++++------- .../__tests__/ContractMethodMultisigSender.js | 4 +- .../src/__tests__/ContractMethodSender.js | 8 ++-- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js index 18c21fca1..a63a345d9 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js @@ -75,13 +75,13 @@ describe('ContractMethod', () => { functionName: 'myFunction', }); - sandbox.spyOn(method, 'validate'); + sandbox.spyOn(method, '_validate'); sandbox.spyOn(method, '_getMethodArgs'); expect(method.getValidatedArgs(inputValues)).toEqual([ 'converted input: 1', ]); - expect(method.validate).toHaveBeenCalledWith(inputValues, method.input); + expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); expect(method._getMethodArgs).toHaveBeenCalledWith( inputValues, method.input, @@ -140,26 +140,37 @@ describe('ContractMethod', () => { const specificationHash = 'QmcNbGg6EVfFn2Z1QxWauR9XY9KhnEcyb5DUXCXHi8pwMJ'; const domainId = 1; - const input = [ - ['specificationHash', 'ipfsHash'], - ['domainId', 'number', domainId], - ]; + const input = [['specificationHash', 'ipfsHash'], ['domainId', 'number']]; + const defaultValues = { domainId }; const inputValues = { specificationHash }; + const withDefaults = { specificationHash, domainId }; const method = new ContractMethod({ client, input, functionName: 'myFunction', + defaultValues, }); - expect(method.convertInputValues(inputValues, input)).toEqual([ - specificationHash, - domainId, - ]); - expect(types.convertInputValue).toHaveBeenCalledWith( - specificationHash, - 'ipfsHash', + sandbox.spyOn(method.constructor, '_getDefaultValues'); + sandbox.spyOn(method, '_validate'); + sandbox.spyOn(method, '_getMethodArgs'); + + const args = method.getValidatedArgs(inputValues); + + expect(args).toEqual([specificationHash, domainId]); + expect(method.constructor._getDefaultValues).toHaveBeenCalledWith( + inputValues, + input, + defaultValues, + ); + expect(method._validate).toHaveBeenCalledWith( + expect.objectContaining(withDefaults), + input, + ); + expect(method._getMethodArgs).toHaveBeenCalledWith( + expect.objectContaining(withDefaults), + input, ); - expect(types.convertInputValue).toHaveBeenCalledWith(domainId, 'number'); }); }); diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js b/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js index 34cf061f3..519bf9b23 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js @@ -189,7 +189,7 @@ describe('ContractMethodMultisigSender', () => { nonceFunctionName, }); - sandbox.spyOn(method, 'validate').mockReturnValue(true); + sandbox.spyOn(method, '_validate').mockReturnValue(true); sandbox.spyOn(method, '_getMethodArgs').mockReturnValue(callArgs); const txData = '0xtxDataGoesHere'; @@ -202,7 +202,7 @@ describe('ContractMethodMultisigSender', () => { const op = await method.startOperation(inputValues); - expect(method.validate).toHaveBeenCalledWith(inputValues, method.input); + expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); expect(method._getMethodArgs).toHaveBeenCalledWith( inputValues, method.input, diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js b/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js index f394de53d..0c4edd0d2 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js @@ -76,14 +76,14 @@ describe('ContractMethodSender', () => { const gasEstimate = new BigNumber(1000); - sandbox.spyOn(method, 'validate').mockImplementation(() => true); + sandbox.spyOn(method, '_validate').mockImplementation(() => true); sandbox.spyOn(method, '_getMethodArgs').mockImplementation(() => callArgs); sandbox .spyOn(client, 'estimate') .mockImplementation(async () => gasEstimate); expect(await method.estimate(inputValues, options)).toBe(gasEstimate); - expect(method.validate).toHaveBeenCalledWith(inputValues, method.input); + expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); expect(method._getMethodArgs).toHaveBeenCalledWith( inputValues, method.input, @@ -102,13 +102,13 @@ describe('ContractMethodSender', () => { eventHandlers, }); - sandbox.spyOn(method, 'validate').mockImplementation(() => true); + sandbox.spyOn(method, '_validate').mockImplementation(() => true); sandbox.spyOn(method, '_getMethodArgs').mockImplementation(() => callArgs); sandbox.spyOn(client, 'send').mockImplementation(async () => transaction); method._send = sandbox.fn(() => contractResponse); expect(await method.send(inputValues, options)).toEqual(contractResponse); - expect(method.validate).toHaveBeenCalledWith(inputValues, method.input); + expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); expect(method._getMethodArgs).toHaveBeenCalledWith( inputValues, method.input, From 237240f64c68e49e9b3f04698fd1b6145b59dd1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 12:57:11 +0200 Subject: [PATCH 11/14] Provide `defaultValues` (`domainId`) --- CHANGELOG.md | 1 + packages/colony-js-client/src/ColonyClient/index.js | 10 ++++------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e61ac373..93750d8dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ **Maintenance** * Add extra validation for `ColonyClient.createTask`; the given domain ID for the task is now confirmed to exist before proceeding (`@colony/colony-js-client`) +* Default values for methods are now specified separately from the parameters; this simplfies the parameter conversion (`@colony/colony-js-contract-client`) * Add a `DomainAdded` event, which is emitted when calling `ColonyClient.addDomain` (`@colony/colony-js-client`) * Add a `PotAdded` event, which is emitted when calling `ColonyClient.addDomain` and `ColonyClient.createTask` (`@colony/colony-js-client`) * Add the `TaskDeliverableSubmitted` and `TaskWorkRatingRevealed` events, which are available to listen on via `ColonyClient.events` (`@colony/colony-js-client`) diff --git a/packages/colony-js-client/src/ColonyClient/index.js b/packages/colony-js-client/src/ColonyClient/index.js index 80a944f97..8ddac2ce7 100644 --- a/packages/colony-js-client/src/ColonyClient/index.js +++ b/packages/colony-js-client/src/ColonyClient/index.js @@ -829,12 +829,10 @@ export default class ColonyClient extends ContractClient { client: this, name: 'createTask', functionName: 'makeTask', - input: [ - // Flow hates you for using an optional last parameter in a tuple - // $FlowFixMe - ['specificationHash', 'ipfsHash'], - ['domainId', 'number', DEFAULT_DOMAIN_ID], - ], + input: [['specificationHash', 'ipfsHash'], ['domainId', 'number']], + defaultValues: { + domainId: DEFAULT_DOMAIN_ID, + }, eventHandlers: { TaskAdded: { contract: this.contract, From 48c6a587d13a16507f8dc73015b91a327a9c87c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Tue, 3 Jul 2018 13:08:48 +0200 Subject: [PATCH 12/14] Remove default param --- .../src/__tests__/paramValidation.js | 43 +++---------------- .../src/flowtypes/params.js | 2 +- .../src/modules/paramValidation.js | 7 +-- 3 files changed, 9 insertions(+), 43 deletions(-) diff --git a/packages/colony-js-contract-client/src/__tests__/paramValidation.js b/packages/colony-js-contract-client/src/__tests__/paramValidation.js index 31059aa4b..59d6a0411 100644 --- a/packages/colony-js-contract-client/src/__tests__/paramValidation.js +++ b/packages/colony-js-contract-client/src/__tests__/paramValidation.js @@ -24,12 +24,13 @@ describe('validateParams', () => { const spec = [ ['taskId', 'number'], ['potId', 'number'], - ['domainId', 'number', 1], + ['domainId', 'number'], ]; const params = { taskId: 6, potId: 420, + domainId: 1, }; sandbox.spyOn(types, 'validateValueType'); @@ -52,7 +53,7 @@ describe('validateParams', () => { // Missing parameter expect(() => { - validation.validateParams({ taskId: 6 }, spec); + validation.validateParams({ taskId: 6, domainId: 1 }, spec); }).toThrowError('Missing parameters: "potId"'); // Wine parameters @@ -80,18 +81,13 @@ describe('validateParams', () => { ); }).not.toThrow(); - // Extra parameter, without the parameter that has a default value + // Wrong type + // validateValue.mockImplementationOnce(() => false); expect(() => { validation.validateParams( - { taskId: 6, potId: 420, somethingElse: 1 }, + { taskId: 'six', domainId: 1, potId: 420 }, spec, ); - }).not.toThrow(); - - // Wrong type - // validateValue.mockImplementationOnce(() => false); - expect(() => { - validation.validateParams({ taskId: 'six', potId: 420 }, spec); }).toThrowError('Parameter "taskId" expected a value of type "number"'); }); }); @@ -136,33 +132,6 @@ describe('validateValue', () => { expect(validation.validateValue('abc', ['id', 'number'], assertValid)); }).toThrowError(failureMessage); }); - - test('Valid default values are reported as valid', () => { - sandbox.spyOn(types, 'validateValueType'); - - expect(validation.validateValue(undefined, ['id', 'number', 1])).toBe(true); - expect(types.validateValueType).toHaveBeenCalledWith(1, 'number'); - }); - - test('Invalid default values are reported as invalid', () => { - sandbox.spyOn(types, 'validateValueType'); - - expect(() => { - expect( - validation.validateValue( - undefined, - ['id', 'number', 'a bad default value'], - assertValid, - ), - ); - }).toThrowError( - `${failureMessage}: Parameter "id" expected a value of type "number"`, - ); - expect(types.validateValueType).toHaveBeenCalledWith( - 'a bad default value', - 'number', - ); - }); }); describe('areParamPairsEmpty', () => { diff --git a/packages/colony-js-contract-client/src/flowtypes/params.js b/packages/colony-js-contract-client/src/flowtypes/params.js index 8541107de..ef88a4c1c 100644 --- a/packages/colony-js-contract-client/src/flowtypes/params.js +++ b/packages/colony-js-contract-client/src/flowtypes/params.js @@ -12,7 +12,7 @@ export type ParamTypes = | 'string'; // [param name, param type, default value (optional)] -export type Param = [string, ParamTypes, *]; +export type Param = [string, ParamTypes]; export type Params = Array; export type ParamTypeDef = { diff --git a/packages/colony-js-contract-client/src/modules/paramValidation.js b/packages/colony-js-contract-client/src/modules/paramValidation.js index c99e5d6c9..0283c6a6d 100644 --- a/packages/colony-js-contract-client/src/modules/paramValidation.js +++ b/packages/colony-js-contract-client/src/modules/paramValidation.js @@ -19,17 +19,14 @@ export const isInputEmpty = (input: any) => export function validateValue( value: any, - [name, type, defaultValue]: Param, + [name, type]: Param, assertValid?: AssertionMethod = defaultAssert, ) { let reason; let isValid = false; try { - isValid = validateValueType( - typeof value !== 'undefined' ? value : defaultValue, - type, - ); + isValid = validateValueType(value, type); } catch (error) { reason = error.message || error.toString(); } From 6e3b263ec22293d665591285e473b46942b038de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Thu, 5 Jul 2018 13:49:03 +0200 Subject: [PATCH 13/14] Simplify `getValidatedArgs` * Remove `_getMethodArgs` method * Improve typing of `_applyDefaultValues` (it now returns `InputValues`) * `_applyDefaultValues` can now be called without params (simply returns a copy of the input) --- .../src/__tests__/ContractMethod.js | 26 +------ .../__tests__/ContractMethodMultisigSender.js | 11 +-- .../src/__tests__/ContractMethodSender.js | 22 ++---- .../src/classes/ContractMethod.js | 75 ++++++++----------- 4 files changed, 46 insertions(+), 88 deletions(-) diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js index a63a345d9..bd9ef9bf5 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js @@ -39,7 +39,7 @@ describe('ContractMethod', () => { sandbox.spyOn(method, 'convertInputValues').mockImplementation(() => [1]); - expect(method._getMethodArgs(inputValues, method.input)).toEqual([1]); + expect(method.getValidatedArgs(inputValues, method.input)).toEqual([1]); expect(method.convertInputValues).toHaveBeenCalledWith( inputValues, method.input, @@ -54,15 +54,7 @@ describe('ContractMethod', () => { sandbox.spyOn(console, 'warn').mockImplementation(() => {}); - expect(method._getMethodArgs()).toEqual([]); - - // There should be a warning if input values are supplied - expect(method._getMethodArgs({ id: 1 })).toEqual([]); - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith( - // eslint-disable-next-line max-len - 'Warning: _getMethodArgs called with parameters for a method that does not accept parameters', - ); + expect(method.getValidatedArgs()).toEqual([]); }); test('Getting validated arguments', () => { @@ -76,16 +68,11 @@ describe('ContractMethod', () => { }); sandbox.spyOn(method, '_validate'); - sandbox.spyOn(method, '_getMethodArgs'); expect(method.getValidatedArgs(inputValues)).toEqual([ 'converted input: 1', ]); expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); - expect(method._getMethodArgs).toHaveBeenCalledWith( - inputValues, - method.input, - ); }); test('Contract return values are mapped to expected output', () => { @@ -152,14 +139,13 @@ describe('ContractMethod', () => { defaultValues, }); - sandbox.spyOn(method.constructor, '_getDefaultValues'); + sandbox.spyOn(method.constructor, '_applyDefaultValues'); sandbox.spyOn(method, '_validate'); - sandbox.spyOn(method, '_getMethodArgs'); const args = method.getValidatedArgs(inputValues); expect(args).toEqual([specificationHash, domainId]); - expect(method.constructor._getDefaultValues).toHaveBeenCalledWith( + expect(method.constructor._applyDefaultValues).toHaveBeenCalledWith( inputValues, input, defaultValues, @@ -168,9 +154,5 @@ describe('ContractMethod', () => { expect.objectContaining(withDefaults), input, ); - expect(method._getMethodArgs).toHaveBeenCalledWith( - expect.objectContaining(withDefaults), - input, - ); }); }); diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js b/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js index 519bf9b23..7d584cba2 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethodMultisigSender.js @@ -189,8 +189,9 @@ describe('ContractMethodMultisigSender', () => { nonceFunctionName, }); - sandbox.spyOn(method, '_validate').mockReturnValue(true); - sandbox.spyOn(method, '_getMethodArgs').mockReturnValue(callArgs); + sandbox + .spyOn(method, 'getValidatedArgs') + .mockImplementation(() => callArgs); const txData = '0xtxDataGoesHere'; sandbox @@ -202,11 +203,7 @@ describe('ContractMethodMultisigSender', () => { const op = await method.startOperation(inputValues); - expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); - expect(method._getMethodArgs).toHaveBeenCalledWith( - inputValues, - method.input, - ); + expect(method.getValidatedArgs).toHaveBeenCalledWith(inputValues); expect(method.client.createTransactionData).toHaveBeenCalledWith( method.functionName, callArgs, diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js b/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js index 0c4edd0d2..59e8e831f 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethodSender.js @@ -76,18 +76,15 @@ describe('ContractMethodSender', () => { const gasEstimate = new BigNumber(1000); - sandbox.spyOn(method, '_validate').mockImplementation(() => true); - sandbox.spyOn(method, '_getMethodArgs').mockImplementation(() => callArgs); + sandbox + .spyOn(method, 'getValidatedArgs') + .mockImplementation(() => callArgs); sandbox .spyOn(client, 'estimate') .mockImplementation(async () => gasEstimate); expect(await method.estimate(inputValues, options)).toBe(gasEstimate); - expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); - expect(method._getMethodArgs).toHaveBeenCalledWith( - inputValues, - method.input, - ); + expect(method.getValidatedArgs).toHaveBeenCalledWith(inputValues); expect(method.client.estimate).toHaveBeenCalledWith( method.functionName, callArgs, @@ -102,17 +99,14 @@ describe('ContractMethodSender', () => { eventHandlers, }); - sandbox.spyOn(method, '_validate').mockImplementation(() => true); - sandbox.spyOn(method, '_getMethodArgs').mockImplementation(() => callArgs); + sandbox + .spyOn(method, 'getValidatedArgs') + .mockImplementation(() => callArgs); sandbox.spyOn(client, 'send').mockImplementation(async () => transaction); method._send = sandbox.fn(() => contractResponse); expect(await method.send(inputValues, options)).toEqual(contractResponse); - expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); - expect(method._getMethodArgs).toHaveBeenCalledWith( - inputValues, - method.input, - ); + expect(method.getValidatedArgs).toHaveBeenCalledWith(inputValues); expect(method._send).toHaveBeenCalledWith(callArgs, options); }); diff --git a/packages/colony-js-contract-client/src/classes/ContractMethod.js b/packages/colony-js-contract-client/src/classes/ContractMethod.js index 804d872b0..26cc62291 100644 --- a/packages/colony-js-contract-client/src/classes/ContractMethod.js +++ b/packages/colony-js-contract-client/src/classes/ContractMethod.js @@ -1,7 +1,6 @@ /* @flow */ /* eslint-disable no-underscore-dangle */ -import isPlainObject from 'lodash.isplainobject'; import { makeAssert } from '@colony/colony-js-utils'; import ContractClient from './ContractClient'; @@ -33,20 +32,20 @@ export default class ContractMethod< * the parameters and construct and object with the properties from the * input values (if they are defined) or default values. */ - static _getDefaultValues( - inputValues: * = {}, - params: Params, + static _applyDefaultValues( + inputValues: InputValues, + params: Params = [], defaultValues: DefaultValues = {}, - ) { - return params.reduce( - (acc, [name]) => - Object.assign(acc, { - [name]: Object.hasOwnProperty.call(inputValues, name) - ? inputValues[name] - : defaultValues[name], - }), - {}, - ); + ): InputValues { + // XXX it's possible to do this in a more succinct way, but adding + // properties in this way preserves type safety + const values = Object.assign({}, inputValues); + params.forEach(([name]) => { + values[name] = Object.hasOwnProperty.call(values, name) + ? values[name] + : defaultValues[name]; + }); + return values; } constructor({ @@ -70,25 +69,6 @@ export default class ContractMethod< * Given named input values, transform these with the expected parameters * in order to get an array of arguments expected by the contract function. */ - _getMethodArgs(inputValues?: *, params?: Params): Array { - let args = []; - - if (inputValues == null) return args; - - if (params && params.length) { - args = this.convertInputValues(inputValues, params); - } else if ( - isPlainObject(inputValues) && - Object.getOwnPropertyNames(inputValues).length - ) { - // eslint-disable-next-line no-console - console.warn( - 'Warning: _getMethodArgs called with parameters for a method that ' + - 'does not accept parameters', - ); - } - return args; - } /** * Given arguments to call the contract method with, return @@ -103,14 +83,15 @@ export default class ContractMethod< * expected params */ validate( - inputValues?: any, + inputValues: any, params: Params = this.input, defaultValues: DefaultValues = this.defaultValues, ) { - const values = - params && params.length - ? this.constructor._getDefaultValues(inputValues, params, defaultValues) - : inputValues; + const values = this.constructor._applyDefaultValues( + inputValues, + params, + defaultValues, + ); return this._validate(values); } @@ -150,19 +131,23 @@ export default class ContractMethod< } /** - * Given input values, get default values (if params are given), then - * validate them and return parsed method args. + * Given input values, get default values, then validate them and return + * parsed method args. */ getValidatedArgs( inputValues: any, params: Params = this.input, defaultValues: DefaultValues = this.defaultValues, ) { - const values = - params && params.length - ? this.constructor._getDefaultValues(inputValues, params, defaultValues) - : inputValues; + const values = this.constructor._applyDefaultValues( + inputValues, + params, + defaultValues, + ); this._validate(values, params); - return this._getMethodArgs(values, params); + + return params && params.length + ? this.convertInputValues(values, params) + : []; } } From 535a68f752242b245669c30ec99bb7729350c2e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Thu, 5 Jul 2018 14:56:53 +0200 Subject: [PATCH 14/14] Remove `_validate` --- .../src/__tests__/ContractMethod.js | 8 ++++---- .../src/classes/ContractMethod.js | 8 ++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js index bd9ef9bf5..d7405ccc8 100644 --- a/packages/colony-js-contract-client/src/__tests__/ContractMethod.js +++ b/packages/colony-js-contract-client/src/__tests__/ContractMethod.js @@ -67,12 +67,12 @@ describe('ContractMethod', () => { functionName: 'myFunction', }); - sandbox.spyOn(method, '_validate'); + sandbox.spyOn(method, 'validate'); expect(method.getValidatedArgs(inputValues)).toEqual([ 'converted input: 1', ]); - expect(method._validate).toHaveBeenCalledWith(inputValues, method.input); + expect(method.validate).toHaveBeenCalledWith(inputValues, method.input); }); test('Contract return values are mapped to expected output', () => { @@ -140,7 +140,7 @@ describe('ContractMethod', () => { }); sandbox.spyOn(method.constructor, '_applyDefaultValues'); - sandbox.spyOn(method, '_validate'); + sandbox.spyOn(method, 'validate'); const args = method.getValidatedArgs(inputValues); @@ -150,7 +150,7 @@ describe('ContractMethod', () => { input, defaultValues, ); - expect(method._validate).toHaveBeenCalledWith( + expect(method.validate).toHaveBeenCalledWith( expect.objectContaining(withDefaults), input, ); diff --git a/packages/colony-js-contract-client/src/classes/ContractMethod.js b/packages/colony-js-contract-client/src/classes/ContractMethod.js index 26cc62291..424e0c7ad 100644 --- a/packages/colony-js-contract-client/src/classes/ContractMethod.js +++ b/packages/colony-js-contract-client/src/classes/ContractMethod.js @@ -92,11 +92,7 @@ export default class ContractMethod< params, defaultValues, ); - return this._validate(values); - } - - _validate(inputValues?: any, params: Params = this.input) { - return validateParams(inputValues, params, this.assertValid); + return validateParams(values, params, this.assertValid); } /** @@ -144,7 +140,7 @@ export default class ContractMethod< params, defaultValues, ); - this._validate(values, params); + this.validate(values, params); return params && params.length ? this.convertInputValues(values, params)