Address feedback and lint

This commit is contained in:
Amir Bandeali
2018-03-09 15:47:21 -08:00
parent 2bd1ddd129
commit 6db0b2e398
19 changed files with 92 additions and 176 deletions

View File

@@ -20,7 +20,7 @@ pragma solidity ^0.4.21;
pragma experimental ABIEncoderV2;
contract ISigner {
function isValidSignature(
bytes32 hash,
bytes signature)

View File

@@ -20,7 +20,7 @@ pragma solidity ^0.4.21;
pragma experimental ABIEncoderV2;
contract LibErrors {
// Error Codes
enum Errors {
ORDER_EXPIRED, // Order has already expired
@@ -28,7 +28,7 @@ contract LibErrors {
ROUNDING_ERROR_TOO_LARGE, // Rounding error too large
INSUFFICIENT_BALANCE_OR_ALLOWANCE // Insufficient balance or allowance for token transfer
}
event LogError(uint8 indexed errorId, bytes32 indexed orderHash);
}

View File

@@ -20,7 +20,7 @@ pragma solidity ^0.4.21;
pragma experimental ABIEncoderV2;
contract LibOrder {
bytes32 constant ORDER_SCHEMA_HASH = keccak256(
"address exchangeAddress",
"address makerAddress",
@@ -35,7 +35,7 @@ contract LibOrder {
"uint256 expirationTimeSeconds",
"uint256 salt"
);
struct Order {
address makerAddress;
address takerAddress;
@@ -49,7 +49,7 @@ contract LibOrder {
uint256 expirationTimeSeconds;
uint256 salt;
}
/// @dev Calculates Keccak-256 hash of the order.
/// @param order The order structure.
/// @return Keccak-256 EIP712 hash of the order.
@@ -58,6 +58,7 @@ contract LibOrder {
returns (bytes32 orderHash)
{
// TODO: EIP712 is not finalized yet
// Source: https://github.com/ethereum/EIPs/pull/712
orderHash = keccak256(
ORDER_SCHEMA_HASH,
keccak256(

View File

@@ -22,7 +22,7 @@ pragma experimental ABIEncoderV2;
import "../../utils/SafeMath/SafeMath.sol";
contract LibPartialAmount is SafeMath {
/// @dev Calculates partial value given a numerator and denominator.
/// @param numerator Numerator.
/// @param denominator Denominator.

View File

@@ -42,7 +42,7 @@ contract MixinExchangeCore is
// Mappings of orderHash => amounts of takerTokenAmount filled or cancelled.
mapping (bytes32 => uint256) public filled;
mapping (bytes32 => uint256) public cancelled;
event LogFill(
address indexed makerAddress,
address takerAddress,
@@ -84,7 +84,7 @@ contract MixinExchangeCore is
{
// Compute the order hash
bytes32 orderHash = getOrderHash(order);
// Validate order and maker only if first time seen
// TODO: Read filled and cancelled only once
if (filled[orderHash] == 0 && cancelled[orderHash] == 0) {
@@ -92,7 +92,7 @@ contract MixinExchangeCore is
require(order.takerTokenAmount > 0);
require(isValidSignature(orderHash, order.makerAddress, signature));
}
// Validate taker
if (order.takerAddress != address(0)) {
require(order.takerAddress == msg.sender);
@@ -104,7 +104,7 @@ contract MixinExchangeCore is
LogError(uint8(Errors.ORDER_EXPIRED), orderHash);
return 0;
}
// Validate order availability
uint256 remainingTakerTokenAmount = safeSub(order.takerTokenAmount, getUnavailableTakerTokenAmount(orderHash));
takerTokenFilledAmount = min256(takerTokenFillAmount, remainingTakerTokenAmount);
@@ -112,7 +112,7 @@ contract MixinExchangeCore is
LogError(uint8(Errors.ORDER_FULLY_FILLED_OR_CANCELLED), orderHash);
return 0;
}
// Validate fill order rounding
if (isRoundingError(takerTokenFilledAmount, order.takerTokenAmount, order.makerTokenAmount)) {
LogError(uint8(Errors.ROUNDING_ERROR_TOO_LARGE), orderHash);
@@ -121,11 +121,11 @@ contract MixinExchangeCore is
// Update state
filled[orderHash] = safeAdd(filled[orderHash], takerTokenFilledAmount);
// Settle order
var (makerTokenFilledAmount, makerFeeAmountPaid, takerFeeAmountPaid) =
settleOrder(order, msg.sender, takerTokenFilledAmount);
// Log order
LogFill(
order.makerAddress,
@@ -160,21 +160,21 @@ contract MixinExchangeCore is
require(order.takerTokenAmount > 0);
require(takerTokenCancelAmount > 0);
require(order.makerAddress == msg.sender);
if (block.timestamp >= order.expirationTimeSeconds) {
LogError(uint8(Errors.ORDER_EXPIRED), orderHash);
return 0;
}
uint256 remainingTakerTokenAmount = safeSub(order.takerTokenAmount, getUnavailableTakerTokenAmount(orderHash));
takerTokenCancelledAmount = min256(takerTokenCancelAmount, remainingTakerTokenAmount);
if (takerTokenCancelledAmount == 0) {
LogError(uint8(Errors.ORDER_FULLY_FILLED_OR_CANCELLED), orderHash);
return 0;
}
cancelled[orderHash] = safeAdd(cancelled[orderHash], takerTokenCancelledAmount);
LogCancel(
order.makerAddress,
order.feeRecipientAddress,
@@ -197,7 +197,9 @@ contract MixinExchangeCore is
returns (bool isError)
{
uint256 remainder = mulmod(target, numerator, denominator);
if (remainder == 0) return false; // No rounding error.
if (remainder == 0) {
return false; // No rounding error.
}
uint256 errPercentageTimes1000000 = safeDiv(
safeMul(remainder, 1000000),

View File

@@ -32,31 +32,30 @@ contract MixinSettlementProxy is
ITokenTransferProxy TRANSFER_PROXY;
IToken ZRX_TOKEN;
function transferProxy()
external view
returns (ITokenTransferProxy)
{
return TRANSFER_PROXY;
}
function zrxToken()
external view
returns (IToken)
{
return ZRX_TOKEN;
}
function MixinSettlementProxy(
ITokenTransferProxy proxyContract,
IToken zrxToken
)
IToken zrxToken)
public
{
ZRX_TOKEN = zrxToken;
TRANSFER_PROXY = proxyContract;
}
function settleOrder(
Order order,
address takerAddress,

View File

@@ -35,7 +35,7 @@ contract MixinSignatureValidator is
Trezor,
Contract
}
function isValidSignature(
bytes32 hash,
address signer,
@@ -44,16 +44,16 @@ contract MixinSignatureValidator is
returns (bool isValid)
{
// TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.)
require(signature.length >= 1);
SignatureType signatureType = SignatureType(uint8(signature[0]));
// Variables are not scoped in Solidity
uint8 v;
bytes32 r;
bytes32 s;
address recovered;
// Always illegal signature
// This is always an implicit option since a signer can create a
// signature array with invalid type or length. We may as well make
@@ -61,7 +61,7 @@ contract MixinSignatureValidator is
// also the initialization value for the enum type.
if (signatureType == SignatureType.Illegal) {
revert();
// Always invalid signature
// Like Illegal, this is always implicitly available and therefore
// offered explicitly. It can be implicitly created by providing
@@ -70,7 +70,7 @@ contract MixinSignatureValidator is
require(signature.length == 1);
isValid = false;
return isValid;
// Implicitly signed by caller
// The signer has initiated the call. In the case of non-contract
// accounts it means the transaction itself was signed.
@@ -83,7 +83,7 @@ contract MixinSignatureValidator is
require(signature.length == 1);
isValid = signer == msg.sender;
return isValid;
// Signed using web3.eth_sign
} else if (signatureType == SignatureType.Ecrecover) {
require(signature.length == 66);
@@ -98,7 +98,7 @@ contract MixinSignatureValidator is
);
isValid = signer == recovered;
return isValid;
// Signature using EIP712
} else if (signatureType == SignatureType.EIP712) {
require(signature.length == 66);
@@ -108,7 +108,7 @@ contract MixinSignatureValidator is
recovered = ecrecover(hash, v, r, s);
isValid = signer == recovered;
return isValid;
// Signature from Trezor hardware wallet
// It differs from web3.eth_sign in the encoding of message length
// (Bitcoin varint encoding vs ascii-decimal, the latter is not
@@ -130,13 +130,13 @@ contract MixinSignatureValidator is
);
isValid = signer == recovered;
return isValid;
// Signature verified by signer contract
} else if (signatureType == SignatureType.Contract) {
isValid = ISigner(signer).isValidSignature(hash, signature);
return isValid;
}
// Anything else is illegal (We do not return false because
// the signature may actually be valid, just not in a format
// that we currently support. In this case returning false
@@ -144,16 +144,16 @@ contract MixinSignatureValidator is
// signature was invalid.)
revert();
}
function get32(bytes b, uint256 index)
private pure
returns (bytes32 result)
{
require(b.length >= index + 32);
// Arrays are prefixed by a 256 bit length parameter
index += 32;
// Read the bytes32 from array memory
assembly {
result := mload(add(b, index))

View File

@@ -74,11 +74,11 @@ contract MixinWrapperFunctions is
// (1): len(signature)
// (2): 452 + len(signature)
// (3): 32 - (len(signature) mod 32)
// (4): 452 + len(signature) + 32 - (len(signature) mod 32)
// (3): (32 - len(signature)) mod 32
// (4): 452 + len(signature) + (32 - len(signature)) mod 32
// [1]: https://solidity.readthedocs.io/en/develop/abi-spec.html
bytes4 fillOrderSelector = this.fillOrder.selector;
assembly {
@@ -112,7 +112,7 @@ contract MixinWrapperFunctions is
mstore(add(start, 420), sigLen)
// Calculate signature length with padding
let paddingLen := sub(32, mod(sigLen, 32))
let paddingLen := mod(sub(0, sigLen), 32)
let sigLenWithPadding := add(sigLen, paddingLen)
// Write signature
@@ -120,11 +120,11 @@ contract MixinWrapperFunctions is
for { let curr := 0 }
lt(curr, sigLenWithPadding)
{ curr := add(curr, 32) }
{ mstore(add(start, add(452, curr)), mload(add(sigStart, curr))) }
{ mstore(add(start, add(452, curr)), mload(add(sigStart, curr))) } // Note: we assume that padding consists of only 0's
// Execute delegatecall
let success := delegatecall(
gas, // forward all gas
gas, // forward all gas, TODO: look into gas consumption of assert/throw
address, // call address of this contract
start, // pointer to start of input
add(452, sigLenWithPadding), // input length is 420 + signature length + padding length
@@ -214,11 +214,12 @@ contract MixinWrapperFunctions is
{
for (uint256 i = 0; i < orders.length; i++) {
require(orders[i].takerTokenAddress == orders[0].takerTokenAddress);
uint256 remainingTakerTokenFillAmount = safeSub(takerTokenFillAmount, totalTakerTokenFilledAmount);
totalTakerTokenFilledAmount = safeAdd(
totalTakerTokenFilledAmount,
fillOrder(
orders[i],
safeSub(takerTokenFillAmount, totalTakerTokenFilledAmount),
remainingTakerTokenFillAmount,
signatures[i]
)
);
@@ -243,11 +244,12 @@ contract MixinWrapperFunctions is
{
for (uint256 i = 0; i < orders.length; i++) {
require(orders[i].takerTokenAddress == orders[0].takerTokenAddress);
uint256 remainingTakerTokenFillAmount = safeSub(takerTokenFillAmount, totalTakerTokenFilledAmount);
totalTakerTokenFilledAmount = safeAdd(
totalTakerTokenFilledAmount,
fillOrderNoThrow(
orders[i],
safeSub(takerTokenFillAmount, totalTakerTokenFilledAmount),
remainingTakerTokenFillAmount,
signatures[i]
)
);

View File

@@ -22,7 +22,7 @@ pragma experimental ABIEncoderV2;
import "../LibOrder.sol";
contract MExchangeCore is LibOrder {
function fillOrder(
Order order,
uint256 takerTokenFillAmount,

View File

@@ -22,7 +22,7 @@ pragma experimental ABIEncoderV2;
import "../LibOrder.sol";
contract MSettlement is LibOrder {
function settleOrder(
Order order,
address taker,

View File

@@ -3,8 +3,6 @@ import ABI = require('ethereumjs-abi');
import ethUtil = require('ethereumjs-util');
import * as _ from 'lodash';
import { SignedOrder } from './types';
export const crypto = {
/**
* We convert types from JS to Solidity as follows:

View File

@@ -31,13 +31,7 @@ export class ExchangeWrapper {
params.signature,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async cancelOrderAsync(
@@ -51,13 +45,7 @@ export class ExchangeWrapper {
params.takerTokenCancelAmount,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async fillOrKillOrderAsync(
@@ -72,13 +60,7 @@ export class ExchangeWrapper {
params.signature,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async fillOrderNoThrowAsync(
@@ -93,13 +75,7 @@ export class ExchangeWrapper {
params.signature,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async batchFillOrdersAsync(
@@ -114,13 +90,7 @@ export class ExchangeWrapper {
params.signatures,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async batchFillOrKillOrdersAsync(
@@ -135,13 +105,7 @@ export class ExchangeWrapper {
params.signatures,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async batchFillOrdersNoThrowAsync(
@@ -156,13 +120,7 @@ export class ExchangeWrapper {
params.signatures,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async marketFillOrdersAsync(
@@ -177,13 +135,7 @@ export class ExchangeWrapper {
params.signatures,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async marketFillOrdersNoThrowAsync(
@@ -198,13 +150,7 @@ export class ExchangeWrapper {
params.signatures,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async batchCancelOrdersAsync(
@@ -218,13 +164,7 @@ export class ExchangeWrapper {
params.takerTokenCancelAmounts,
{ from },
);
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => {
const logWithDecodedArgs = this._logDecoder.decodeLogOrThrow(log);
wrapLogBigNumbers(logWithDecodedArgs);
return logWithDecodedArgs;
});
const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
return tx;
}
public async getOrderHashAsync(signedOrder: SignedOrder): Promise<string> {
@@ -262,14 +202,10 @@ export class ExchangeWrapper {
const filledAmount = new BigNumber(await this._exchange.filled.callAsync(orderHashHex));
return filledAmount;
}
}
function wrapLogBigNumbers(log: any): any {
const argNames = _.keys(log.args);
for (const argName of argNames) {
const isWeb3BigNumber = _.startsWith(log.args[argName].constructor.toString(), 'function BigNumber(');
if (isWeb3BigNumber) {
log.args[argName] = new BigNumber(log.args[argName]);
}
private async _getTxWithDecodedExchangeLogsAsync(txHash: string) {
const tx = await this._zeroEx.awaitTransactionMinedAsync(txHash);
tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address);
tx.logs = _.map(tx.logs, log => this._logDecoder.decodeLogOrThrow(log));
return tx;
}
}

View File

@@ -1,5 +1,5 @@
import { LogWithDecodedArgs, RawLog } from '@0xproject/types';
import { AbiDecoder } from '@0xproject/utils';
import { AbiDecoder, BigNumber } from '@0xproject/utils';
import * as _ from 'lodash';
import * as Web3 from 'web3';
@@ -27,6 +27,17 @@ export class LogDecoder {
if (_.isUndefined((logWithDecodedArgsOrLog as LogWithDecodedArgs<ArgsType>).args)) {
throw new Error(`Unable to decode log: ${JSON.stringify(log)}`);
}
wrapLogBigNumbers(logWithDecodedArgsOrLog);
return logWithDecodedArgsOrLog;
}
}
function wrapLogBigNumbers(log: any): any {
const argNames = _.keys(log.args);
for (const argName of argNames) {
const isWeb3BigNumber = _.startsWith(log.args[argName].constructor.toString(), 'function BigNumber(');
if (isWeb3BigNumber) {
log.args[argName] = new BigNumber(log.args[argName]);
}
}
}

View File

@@ -8,10 +8,10 @@ import { DefaultOrderParams, SignatureType, SignedOrder, UnsignedOrder } from '.
export class OrderFactory {
private _defaultOrderParams: Partial<UnsignedOrder>;
private _secretKey: Buffer;
constructor(secretKey: Buffer, defaultOrderParams: Partial<UnsignedOrder>) {
private _privateKey: Buffer;
constructor(privateKey: Buffer, defaultOrderParams: Partial<UnsignedOrder>) {
this._defaultOrderParams = defaultOrderParams;
this._secretKey = secretKey;
this._privateKey = privateKey;
}
public newSignedOrder(
customOrderParams: Partial<UnsignedOrder> = {},
@@ -26,7 +26,7 @@ export class OrderFactory {
...customOrderParams,
} as any) as UnsignedOrder;
const orderHashBuff = orderUtils.getOrderHashBuff(order);
const signature = signingUtils.signMessage(orderHashBuff, this._secretKey, signatureType);
const signature = signingUtils.signMessage(orderHashBuff, this._privateKey, signatureType);
const signedOrder = {
...order,
signature: `0x${signature.toString('hex')}`,

View File

@@ -3,10 +3,10 @@ import * as ethUtil from 'ethereumjs-util';
import { SignatureType } from './types';
export const signingUtils = {
signMessage(message: Buffer, secretKey: Buffer, signatureType: SignatureType): Buffer {
signMessage(message: Buffer, privateKey: Buffer, signatureType: SignatureType): Buffer {
if (signatureType === SignatureType.Ecrecover) {
const prefixedMessage = ethUtil.hashPersonalMessage(message);
const ecSignature = ethUtil.ecsign(prefixedMessage, secretKey);
const ecSignature = ethUtil.ecsign(prefixedMessage, privateKey);
const signature = Buffer.concat([
ethUtil.toBuffer(signatureType),
ethUtil.toBuffer(ecSignature.v),
@@ -15,7 +15,7 @@ export const signingUtils = {
]);
return signature;
} else if (signatureType === SignatureType.EIP712) {
const ecSignature = ethUtil.ecsign(message, secretKey);
const ecSignature = ethUtil.ecsign(message, privateKey);
const signature = Buffer.concat([
ethUtil.toBuffer(signatureType),
ethUtil.toBuffer(ecSignature.v),

View File

@@ -108,19 +108,7 @@ export interface Artifact {
};
}
export interface SignedOrder {
exchangeAddress: string;
makerAddress: string;
takerAddress: string;
makerTokenAddress: string;
takerTokenAddress: string;
feeRecipientAddress: string;
makerTokenAmount: BigNumber;
takerTokenAmount: BigNumber;
makerFeeAmount: BigNumber;
takerFeeAmount: BigNumber;
expirationTimeSeconds: BigNumber;
salt: BigNumber;
export interface SignedOrder extends UnsignedOrder {
signature: string;
}
@@ -138,19 +126,8 @@ export interface OrderStruct {
salt: BigNumber;
}
export interface UnsignedOrder {
export interface UnsignedOrder extends OrderStruct {
exchangeAddress: string;
makerAddress: string;
takerAddress: string;
makerTokenAddress: string;
takerTokenAddress: string;
feeRecipientAddress: string;
makerTokenAmount: BigNumber;
takerTokenAmount: BigNumber;
makerFeeAmount: BigNumber;
takerFeeAmount: BigNumber;
expirationTimeSeconds: BigNumber;
salt: BigNumber;
}
export enum SignatureType {

View File

@@ -1,7 +0,0 @@
export const utils = {
consoleLog(message: string): void {
/* tslint:disable */
console.log(message);
/* tslint:enable */
},
};