Merge pull request #233 from JoinColony/maintenance/153-check-domain-…

…id-when-creating-task

Check domain ID when creating task

CHANGELOG.md

@@ -14,6 +14,8 @@
 
 **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`)

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",

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,

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,12 +825,14 @@ export default class ColonyClient extends ContractClient {
  ['token', 'tokenAddress'],
  ],
  });
- this.addSender('createTask', {
+ this.createTask = new CreateTask({
+ client: this,
+ name: 'createTask',
  functionName: 'makeTask',
- input: [
-  ['specificationHash', 'ipfsHash'],
- ['domainId', 'number', DEFAULT_DOMAIN_ID],
- ],
+ input: [['specificationHash', 'ipfsHash'], ['domainId', 'number']],
+ defaultValues: {
+ domainId: DEFAULT_DOMAIN_ID,
+ },
  eventHandlers: {
  TaskAdded: {
  contract: this.contract,

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);
+ }
+}

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', () => {
@@ -140,26 +127,32 @@ 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, '_applyDefaultValues');
+ sandbox.spyOn(method, 'validate');
+
+ const args = method.getValidatedArgs(inputValues);
+
+ expect(args).toEqual([specificationHash, domainId]);
+ expect(method.constructor._applyDefaultValues).toHaveBeenCalledWith(
+ inputValues,
+ input,
+ defaultValues,
+ );
+ expect(method.validate).toHaveBeenCalledWith(
+ expect.objectContaining(withDefaults),
+ input,
  );
- expect(types.convertInputValue).toHaveBeenCalledWith(domainId, 'number');
  });
 });

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,

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);
  });
 

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', () => {

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`);
  }

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<ParamTypes> = (args: ParamTypes) => void;
 
-const normalizeEventParams = (spec: EventParams): Params =>
- spec.map(([parameterName, parameterType]) => [
- parameterName,
- parameterType,
- undefined,
- ]);
-
 export default class ContractEvent<ParamTypes: Object> {
  // The event's name as defined in the contract.
  eventName: string;
@@ -40,11 +33,11 @@ export default class ContractEvent<ParamTypes: Object> {
  }: {
  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}`);