Skip to content
This repository was archived by the owner on Sep 20, 2019. It is now read-only.

feature/multi-trade - #95

Open
mattdf wants to merge 9 commits into
version/3.0.0from
feature/multi-trade
Open

feature/multi-trade#95
mattdf wants to merge 9 commits into
version/3.0.0from
feature/multi-trade

Conversation

@mattdf

Copy link
Copy Markdown
Contributor
  • extend trade func to return fill amounts
  • implement fill per order and cumulative fill checks
  • write tests

@mattdfmattdf changed the title multi-trade functionalityfeature/multi-tradeApr 27, 2018
Comment threadcontracts/Exchange.sol Outdated
}


function multitrade(uint numOrders, address[] addresses, uint[] values, bytes32[] sigmain, uint16[] sigaux, uint maxFillAmount) external {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look into this because of the trade off of readibility.

Comment threadcontracts/Exchange.sol Outdated

address[3] memory addrs;
uint[4] memory vals;
bytes memory s = new bytes(66);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this

bytes32 s1 = sm[i*2];
bytes32 s2 = sm[i*2 + 1];
uint16 s3 = sa[i];
bytes memory s = new bytes(66);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that not everything is 0x0 before, if it is, do a return.

Comment threadcontracts/Exchange.sol Outdated
require(addresses.length == 3*numOrders);
require(values.length == 4*numOrders);
require(sigmain.length == 2*numOrders);
require(sigaux.length == numOrders);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numOrders is not needed, as we can guarantee that sigaux is the length of numorders.

@decanus
decanus changed the base branch from development to version/3.0.0April 27, 2018 13:54
Comment threadcontracts/Exchange.sol Outdated
vals[j] = values[(i*4)+j];
}
s = sigArrayToBytes(sigmain, sigaux, i);
trade(OrderLibrary.createOrder(addrs, vals), msg.sender, s, maxFillAmount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider doing this differently, maybe first checking if we can trade. Otherwise this function will throw, which might be pretty stupid as that will cancel the entire multitrade

uint maxFillAmount
)
internal
internal returns (uint)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns on new line

}
addrs[0] = makers[i];
addrs[1] = makerToken;
addrs[2] = takerToken;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no point in addrs

vals[j] = values[(i*4)+j];
}
s = sigArrayToBytes(sigmain, sigaux, i);
exchange.trade(OrderLibrary.createOrder(addrs, vals), msg.sender, s, maxFillAmount[i]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for s, do it right here.

}


function multitrade(address[] addresses, uint[] values, bytes32[] sigmain, uint16[] sigaux, uint[] maxFillAmount) external {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we do not fully use it, we should consider ABI Encoder V2 here, it will allow us to use an array of bytes, rather than this weird split we are doing here.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mattdf@decanus