Add OrderTransactionOpts to enable optional validation to exchange_wr… #172
Conversation
| assert.doesConformToSchema('signedOrder', signedOrder, schemas.signedOrderSchema); | ||
| assert.isBigNumber('fillTakerTokenAmount', fillTakerTokenAmount); | ||
| assert.isBoolean('shouldThrowOnInsufficientBalanceOrAllowance', shouldThrowOnInsufficientBalanceOrAllowance); | ||
| await assert.isSenderAddressAsync('takerAddress', takerAddress, this._web3Wrapper); | ||
|
|
||
| const exchangeInstance = await this._getExchangeContractAsync(); | ||
| await this.validateFillOrderThrowIfInvalidAsync(signedOrder, fillTakerTokenAmount, takerAddress); | ||
| const shouldValidate = _.isUndefined(orderTransactionOpts) ? true : orderTransactionOpts.shouldValidate; |
There was a problem hiding this comment.
Let's assign true to a constant defined at the top of the file called SHOULD_VALIDATE_BY_DEFAULT. I'm guessing we'd want to default to be the same for all methods.
| const shouldValidate = _.isUndefined(orderTransactionOpts) ? true : orderTransactionOpts.shouldValidate; | ||
| if (shouldValidate) { | ||
| await Promise.all(orderFillRequests.map(orderFillRequest => this.validateFillOrderThrowIfInvalidAsync( | ||
| orderFillRequest.signedOrder, orderFillRequest.takerTokenFillAmount, takerAddress))); |
There was a problem hiding this comment.
Nit: Let's move the ending parens to the next line.
| const shouldValidate = _.isUndefined(orderTransactionOpts) ? true : orderTransactionOpts.shouldValidate; | ||
| if (shouldValidate) { | ||
| await Promise.all(orderFillOrKillRequests.map(request => this.validateFillOrKillOrderThrowIfInvalidAsync( | ||
| request.signedOrder, request.fillTakerAmount, takerAddress))); |
There was a problem hiding this comment.
Nit: ditto above. We try to align starting and ending parens, brackets, dom elements, as best we can. Does this make sense? I'm open to alternative conventions.
test/exchange_wrapper_test.ts
Outdated
| return expect(zeroEx.exchange.batchFillOrKillAsync(orderFillOrKillRequests, takerAddress, | ||
| { | ||
| shouldValidate: true, | ||
| }, |
There was a problem hiding this comment.
Nit: No need to put the start and end curly brackets on a new line when using an anonymous object as a method param.
test/exchange_wrapper_test.ts
Outdated
| @@ -323,6 +468,26 @@ describe('ExchangeWrapper', () => { | |||
| expect(cancelledAmount).to.be.bignumber.equal(cancelAmount); | |||
| }); | |||
| }); | |||
| describe('order transaction options', () => { | |||
| it('should validate when orderTransactionOptions are not present', async () => { | |||
| return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, new BigNumber(0))) | |||
There was a problem hiding this comment.
Nit: Let's not in-line instantiating new objects in method params. Instead declare outside with a descriptive variable name of what it represents.
Our convention is also not to pass numbers/strings/booleans directly at params but to always assign them to variables with readable names. Otherwise the reader needs to look at the method definition to figure out what the value represents (saves future devs time).
…apper