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

Commit

Permalink
Merge pull request #2127 from 0xProject/feat/3.0/catch-pay-protocol-f…
Browse files Browse the repository at this point in the history 
…ee-err

Wrap `payProtocolFee` with rich revert + reduce codesize
  • Loading branch information
@abandeali1
abandeali1 authored Sep 4, 2019
2 parents 88736aa + cf35a80 commit e5dcf90
Show file tree
Hide file tree
Showing 13 changed files with 339 additions and 235 deletions.
113 changes: 50 additions & 63 deletions contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ library LibExchangeRichErrors {
INVALID_LENGTH_RIGHT_SIGNATURES
}

enum ExchangeContextErrorCodes {
INVALID_MAKER,
INVALID_TAKER,
INVALID_SENDER
}

enum FillErrorCodes {
INVALID_TAKER_AMOUNT,
TAKER_OVERPAY,
Expand Down Expand Up @@ -83,22 +89,14 @@ library LibExchangeRichErrors {
bytes4 internal constant ORDER_STATUS_ERROR_SELECTOR =
0xfdb6ca8d;

// bytes4(keccak256("InvalidSenderError(bytes32,address)"))
bytes4 internal constant INVALID_SENDER_ERROR_SELECTOR =
0x95b59997;

// bytes4(keccak256("InvalidMakerError(bytes32,address)"))
bytes4 internal constant INVALID_MAKER_ERROR_SELECTOR =
0x26bf55d9;
// bytes4(keccak256("ExchangeInvalidContextError(uint8,bytes32,address)"))
bytes4 internal constant EXCHANGE_INVALID_CONTEXT_ERROR_SELECTOR =
0xe53c76c8;

// bytes4(keccak256("FillError(uint8,bytes32)"))
bytes4 internal constant FILL_ERROR_SELECTOR =
0xe94a7ed0;

// bytes4(keccak256("InvalidTakerError(bytes32,address)"))
bytes4 internal constant INVALID_TAKER_ERROR_SELECTOR =
0xfdb328be;

// bytes4(keccak256("OrderEpochError(address,address,uint256)"))
bytes4 internal constant ORDER_EPOCH_ERROR_SELECTOR =
0x4ad31275;
Expand Down Expand Up @@ -147,6 +145,10 @@ library LibExchangeRichErrors {
bytes4 internal constant BATCH_MATCH_ORDERS_ERROR_SELECTOR =
0xd4092f4f;

// bytes4(keccak256("PayProtocolFeeError(bytes32,uint256,address,address,bytes)"))
bytes4 internal constant PAY_PROTOCOL_FEE_ERROR_SELECTOR =
0x87cb1e75;

// solhint-disable func-name-mixedcase
function SignatureErrorSelector()
internal
Expand Down Expand Up @@ -188,20 +190,12 @@ library LibExchangeRichErrors {
return ORDER_STATUS_ERROR_SELECTOR;
}

function InvalidSenderErrorSelector()
internal
pure
returns (bytes4)
{
return INVALID_SENDER_ERROR_SELECTOR;
}

function InvalidMakerErrorSelector()
function ExchangeInvalidContextErrorSelector()
internal
pure
returns (bytes4)
{
return INVALID_MAKER_ERROR_SELECTOR;
return EXCHANGE_INVALID_CONTEXT_ERROR_SELECTOR;
}

function FillErrorSelector()
Expand All @@ -212,14 +206,6 @@ library LibExchangeRichErrors {
return FILL_ERROR_SELECTOR;
}

function InvalidTakerErrorSelector()
internal
pure
returns (bytes4)
{
return INVALID_TAKER_ERROR_SELECTOR;
}

function OrderEpochErrorSelector()
internal
pure
Expand Down Expand Up @@ -316,6 +302,14 @@ library LibExchangeRichErrors {
return TRANSACTION_INVALID_CONTEXT_ERROR_SELECTOR;
}

function PayProtocolFeeErrorSelector()
internal
pure
returns (bytes4)
{
return PAY_PROTOCOL_FEE_ERROR_SELECTOR;
}

function BatchMatchOrdersError(
BatchMatchOrdersErrorCodes errorCode
)
Expand Down Expand Up @@ -416,33 +410,20 @@ library LibExchangeRichErrors {
);
}

function InvalidSenderError(
function ExchangeInvalidContextError(
ExchangeContextErrorCodes errorCode,
bytes32 orderHash,
address senderAddress
address contextAddress
)
internal
pure
returns (bytes memory)
{
return abi.encodeWithSelector(
INVALID_SENDER_ERROR_SELECTOR,
orderHash,
senderAddress
);
}

function InvalidMakerError(
bytes32 orderHash,
address makerAddress
)
internal
pure
returns (bytes memory)
{
return abi.encodeWithSelector(
INVALID_MAKER_ERROR_SELECTOR,
EXCHANGE_INVALID_CONTEXT_ERROR_SELECTOR,
errorCode,
orderHash,
makerAddress
contextAddress
);
}

Expand All @@ -461,21 +442,6 @@ library LibExchangeRichErrors {
);
}

function InvalidTakerError(
bytes32 orderHash,
address takerAddress
)
internal
pure
returns (bytes memory)
{
return abi.encodeWithSelector(
INVALID_TAKER_ERROR_SELECTOR,
orderHash,
takerAddress
);
}

function OrderEpochError(
address makerAddress,
address orderSenderAddress,
Expand Down Expand Up @@ -652,4 +618,25 @@ library LibExchangeRichErrors {
actualAssetFillAmount
);
}

function PayProtocolFeeError(
bytes32 orderHash,
uint256 protocolFee,
address makerAddress,
address takerAddress,
bytes memory errorData
)
internal
pure
returns (bytes memory)
{
return abi.encodeWithSelector(
PAY_PROTOCOL_FEE_ERROR_SELECTOR,
orderHash,
protocolFee,
makerAddress,
takerAddress,
errorData
);
}
}
60 changes: 28 additions & 32 deletions contracts/exchange/contracts/src/MixinExchangeCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol";
import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
import "@0x/contracts-exchange-libs/contracts/src/LibEIP712ExchangeDomain.sol";
import "@0x/contracts-exchange-libs/contracts/src/LibExchangeRichErrors.sol";
import "@0x/contracts-staking/contracts/src/interfaces/IStaking.sol";
import "./interfaces/IExchangeCore.sol";
import "./MixinAssetProxyDispatcher.sol";
import "./MixinProtocolFees.sol";
Expand Down Expand Up @@ -350,17 +349,21 @@ contract MixinExchangeCore is
// Validate sender is allowed to fill this order
if (order.senderAddress != address(0)) {
if (order.senderAddress != msg.sender) {
LibRichErrors.rrevert(LibExchangeRichErrors.InvalidSenderError(
orderInfo.orderHash, msg.sender
LibRichErrors.rrevert(LibExchangeRichErrors.ExchangeInvalidContextError(
LibExchangeRichErrors.ExchangeContextErrorCodes.INVALID_SENDER,
orderInfo.orderHash,
msg.sender
));
}
}

// Validate taker is allowed to fill this order
if (order.takerAddress != address(0)) {
if (order.takerAddress != takerAddress) {
LibRichErrors.rrevert(LibExchangeRichErrors.InvalidTakerError(
orderInfo.orderHash, takerAddress
LibRichErrors.rrevert(LibExchangeRichErrors.ExchangeInvalidContextError(
LibExchangeRichErrors.ExchangeContextErrorCodes.INVALID_TAKER,
orderInfo.orderHash,
takerAddress
));
}
}
Expand Down Expand Up @@ -404,14 +407,22 @@ contract MixinExchangeCore is
// Validate sender is allowed to cancel this order
if (order.senderAddress != address(0)) {
if (order.senderAddress != msg.sender) {
LibRichErrors.rrevert(LibExchangeRichErrors.InvalidSenderError(orderInfo.orderHash, msg.sender));
LibRichErrors.rrevert(LibExchangeRichErrors.ExchangeInvalidContextError(
LibExchangeRichErrors.ExchangeContextErrorCodes.INVALID_SENDER,
orderInfo.orderHash,
msg.sender
));
}
}

// Validate transaction signed by maker
address makerAddress = _getCurrentContextAddress();
if (order.makerAddress != makerAddress) {
LibRichErrors.rrevert(LibExchangeRichErrors.InvalidMakerError(orderInfo.orderHash, makerAddress));
LibRichErrors.rrevert(LibExchangeRichErrors.ExchangeInvalidContextError(
LibExchangeRichErrors.ExchangeContextErrorCodes.INVALID_MAKER,
orderInfo.orderHash,
makerAddress
));
}
}

Expand Down Expand Up @@ -464,31 +475,16 @@ contract MixinExchangeCore is
fillResults.makerFeePaid
);

// Transfer protocol fee -> staking if the fee should be paid
address feeCollector = protocolFeeCollector;
if (feeCollector != address(0)) {
// Create a stack variable to hold the value that will be sent so that the gas optimization of
// only having one call statement can be implemented.
uint256 valuePaid = 0;

// Calculate the protocol fee that should be paid and populate the `protocolFeePaid` field in `fillResults`.
// It's worth noting that we leave this calculation until now so that work isn't wasted if a fee collector
// is not registered in the exchange.
uint256 protocolFee = fillResults.protocolFeePaid;

// If sufficient ether was sent to the contract, the protocol fee should be paid in ETH.
// Otherwise the fee should be paid in WETH. Since the exchange doesn't actually handle
// this case, it will just forward the procotolFee in ether in case 1 and will send zero
// value in case 2.
if (address(this).balance >= protocolFee) {
valuePaid = protocolFee;
}
IStaking(feeCollector).payProtocolFee.value(valuePaid)(
order.makerAddress,
takerAddress,
protocolFee
);
} else {
// Pay protocol fee
bool didPayProtocolFee = _paySingleProtocolFee(
orderHash,
fillResults.protocolFeePaid,
order.makerAddress,
takerAddress
);

// Protocol fees are not paid if the protocolFeeCollector contract is not set
if (!didPayProtocolFee) {
fillResults.protocolFeePaid = 0;
}
}
Expand Down
62 changes: 19 additions & 43 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,13 @@ contract MixinMatchOrders is
/// @dev Validates context for matchOrders. Succeeds or throws.
/// @param leftOrder First order to match.
/// @param rightOrder Second order to match.
/// @param leftOrderInfo OrderStatus, orderHash, and amount already filled of leftOrder.
/// @param rightOrderInfo OrderStatus, orderHash, and amount already filled of rightOrder.
/// @param leftOrderHash First matched order hash.
/// @param rightOrderHash Second matched order hash.
function _assertValidMatch(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder,
LibOrder.OrderInfo memory leftOrderInfo,
LibOrder.OrderInfo memory rightOrderInfo
bytes32 leftOrderHash,
bytes32 rightOrderHash
)
internal
view
Expand All @@ -178,8 +178,8 @@ contract MixinMatchOrders is
if (leftOrder.makerAssetAmount.safeMul(rightOrder.makerAssetAmount) <
leftOrder.takerAssetAmount.safeMul(rightOrder.takerAssetAmount)) {
LibRichErrors.rrevert(LibExchangeRichErrors.NegativeSpreadError(
leftOrderInfo.orderHash,
rightOrderInfo.orderHash
leftOrderHash,
rightOrderHash
));
}
}
Expand Down Expand Up @@ -379,8 +379,8 @@ contract MixinMatchOrders is
_assertValidMatch(
leftOrder,
rightOrder,
leftOrderInfo,
rightOrderInfo
leftOrderInfo.orderHash,
rightOrderInfo.orderHash
);

// Compute proportional fill amounts
Expand Down Expand Up @@ -495,42 +495,18 @@ contract MixinMatchOrders is
matchedFillResults.profitInRightMakerAsset
);

// Pay the protocol fees if there is a registered `protocolFeeCollector` address.
address feeCollector = protocolFeeCollector;
if (feeCollector != address(0)) {
// Only one of the protocol fees is used because they are identical.
uint256 protocolFee = matchedFillResults.left.protocolFeePaid;

// Create a stack variable for the value that will be sent to the feeCollector when `payProtocolFee` is called.
// This allows a gas optimization where the `leftOrder.makerAddress` only needs be loaded onto the stack once AND
// a stack variable does not need to be allocated for the call.
uint256 valuePaid = 0;

// Since the `BALANCE` opcode costs 400 gas, we choose to calculate this value by hand rather than calling it twice.
uint256 balance = address(this).balance;

// Pay the left order's protocol fee.
if (balance >= protocolFee) {
valuePaid = protocolFee;
}
IStaking(feeCollector).payProtocolFee.value(valuePaid)(
leftOrder.makerAddress,
takerAddress,
protocolFee
);
// Pay protocol fees for each maker
bool didPayProtocolFees = _payTwoProtocolFees(
leftOrderHash,
rightOrderHash,
matchedFillResults.left.protocolFeePaid,
leftOrder.makerAddress,
rightOrder.makerAddress,
takerAddress
);

// Pay the right order's protocol fee.
if (balance - valuePaid >= protocolFee) {
valuePaid = protocolFee;
} else {
valuePaid = 0;
}
IStaking(feeCollector).payProtocolFee.value(valuePaid)(
rightOrder.makerAddress,
takerAddress,
protocolFee
);
} else {
// Protocol fees are not paid if the protocolFeeCollector contract is not set
if (!didPayProtocolFees) {
matchedFillResults.left.protocolFeePaid = 0;
matchedFillResults.right.protocolFeePaid = 0;
}
Expand Down
Loading

0 comments on commit e5dcf90

Please sign in to comment.