Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Address feedback comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@dekz
dekz committed Mar 16, 2020
1 parent b68026d commit 3ba8dc2
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 49 deletions.
67 changes: 21 additions & 46 deletions contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ contract MixinExchangeWrapper {
// ")"
// )));
bytes4 constant public EXCHANGE_V2_ORDER_ID = 0x770501f8;
bytes4 constant internal ERC20_BRIDGE_PROXY_ID = 0xdc1600f3;

// solhint-disable var-name-mixedcase
IExchange internal EXCHANGE;
Expand Down Expand Up @@ -123,6 +124,14 @@ contract MixinExchangeWrapper {
internal
returns (SellFillResults memory sellFillResults)
{
// If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance.
bytes4 makerAssetProxyId = order.makerAssetData.readBytes4(0);
address tokenAddress;
uint256 balanceBefore;
if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) {
tokenAddress = order.makerAssetData.readAddress(16);
balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this));
}
// No taker fee or percentage fee
if (
order.takerFee == 0 ||
Expand Down Expand Up @@ -170,6 +179,16 @@ contract MixinExchangeWrapper {
LibRichErrors.rrevert(LibForwarderRichErrors.UnsupportedFeeError(order.takerFeeAssetData));
}

// Account for the ERC20Bridge transfering more of the maker asset than expected.
if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) {
uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this));
sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256(
balanceAfter.safeSub(balanceBefore),
sellFillResults.makerAssetAcquiredAmount
);
}

order.makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount);
return sellFillResults;
}

Expand All @@ -192,7 +211,6 @@ contract MixinExchangeWrapper {
)
{
uint256 protocolFee = tx.gasprice.safeMul(EXCHANGE.protocolFeeMultiplier());
bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector;

for (uint256 i = 0; i != orders.length; i++) {
// Preemptively skip to avoid division by zero in _marketSellSingleOrder
Expand All @@ -205,32 +223,12 @@ contract MixinExchangeWrapper {
.safeSub(totalWethSpentAmount)
.safeSub(_isV2Order(orders[i]) ? 0 : protocolFee);

// If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance.
bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0);
address tokenAddress;
uint256 balanceBefore;
if (makerAssetProxyId == erc20BridgeProxyId) {
tokenAddress = orders[i].makerAssetData.readAddress(16);
balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this));
}

SellFillResults memory sellFillResults = _marketSellSingleOrder(
orders[i],
signatures[i],
remainingTakerAssetFillAmount
);

// Account for the ERC20Bridge transfering more of the maker asset than expected.
if (makerAssetProxyId == erc20BridgeProxyId) {
uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this));
sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256(
balanceAfter.safeSub(balanceBefore),
sellFillResults.makerAssetAcquiredAmount
);
}

orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount);

totalWethSpentAmount = totalWethSpentAmount
.safeAdd(sellFillResults.wethSpentAmount)
.safeAdd(sellFillResults.protocolFeePaid);
Expand Down Expand Up @@ -262,7 +260,6 @@ contract MixinExchangeWrapper {
uint256 totalMakerAssetAcquiredAmount
)
{
bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector;
uint256 totalProtocolFeePaid;

for (uint256 i = 0; i != orders.length; i++) {
Expand All @@ -275,32 +272,12 @@ contract MixinExchangeWrapper {
uint256 remainingTakerAssetFillAmount = wethSellAmount
.safeSub(totalWethSpentAmount);

// If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance.
bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0);
address tokenAddress;
uint256 balanceBefore;
if (makerAssetProxyId == erc20BridgeProxyId) {
tokenAddress = orders[i].makerAssetData.readAddress(16);
balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this));
}

SellFillResults memory sellFillResults = _marketSellSingleOrder(
orders[i],
signatures[i],
remainingTakerAssetFillAmount
);

// Account for the ERC20Bridge transfering more of the maker asset than expected.
if (makerAssetProxyId == erc20BridgeProxyId) {
uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this));
sellFillResults.makerAssetAcquiredAmount = LibSafeMath.max256(
balanceAfter.safeSub(balanceBefore),
sellFillResults.makerAssetAcquiredAmount
);
}

orders[i].makerAssetData.transferOut(sellFillResults.makerAssetAcquiredAmount);

totalWethSpentAmount = totalWethSpentAmount
.safeAdd(sellFillResults.wethSpentAmount);
totalMakerAssetAcquiredAmount = totalMakerAssetAcquiredAmount
Expand Down Expand Up @@ -410,8 +387,6 @@ contract MixinExchangeWrapper {
uint256 totalMakerAssetAcquiredAmount
)
{
bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector;

uint256 ordersLength = orders.length;
for (uint256 i = 0; i != ordersLength; i++) {
// Preemptively skip to avoid division by zero in _marketBuySingleOrder
Expand All @@ -426,7 +401,7 @@ contract MixinExchangeWrapper {
bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0);
address tokenAddress;
uint256 balanceBefore;
if (makerAssetProxyId == erc20BridgeProxyId) {
if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) {
tokenAddress = orders[i].makerAssetData.readAddress(16);
balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this));
}
Expand All @@ -441,7 +416,7 @@ contract MixinExchangeWrapper {
);

// Account for the ERC20Bridge transfering more of the maker asset than expected.
if (makerAssetProxyId == erc20BridgeProxyId) {
if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) {
uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this));
makerAssetAcquiredAmount = LibSafeMath.max256(
balanceAfter.safeSub(balanceBefore),
Expand Down
5 changes: 2 additions & 3 deletions contracts/integrations/test/forwarder/forwarder_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ blockchainTests('Forwarder integration tests', env => {
);
});
});
blockchainTests.resets.only('marketSellOrdersWithEth without extra fees', () => {
blockchainTests.resets('marketSellOrdersWithEth without extra fees', () => {
it('should fill a single order without a taker fee', async () => {
const orderWithoutFee = await maker.signOrderAsync();
await testFactory.marketSellTestAsync([orderWithoutFee], 0.78);
Expand Down Expand Up @@ -449,13 +449,12 @@ blockchainTests('Forwarder integration tests', env => {
});
});
blockchainTests.resets('marketSellAmountWithEth', () => {
const protocolFee = new BigNumber(150000).times(DeploymentManager.gasPrice);
it('should fail if the supplied amount is not sold', async () => {
const order = await maker.signOrderAsync();
const ethSellAmount = order.takerAssetAmount;
const revertError = new ExchangeForwarderRevertErrors.CompleteSellFailedError(
ethSellAmount,
order.takerAssetAmount.times(0.5).plus(protocolFee),
order.takerAssetAmount.times(0.5).plus(DeploymentManager.protocolFee),
);
await testFactory.marketSellAmountTestAsync([order], ethSellAmount, 0.5, {
revertError,
Expand Down

0 comments on commit 3ba8dc2

Please sign in to comment.