-
Notifications
You must be signed in to change notification settings - Fork 466
Conversation
d6cbff0
to
c9f51f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job on this one! I've got a couple of questions and nits, but after that it looks good to go.
selector == FILL_ORDER_SELECTOR || | ||
selector == FILL_ORDER_NO_THROW_SELECTOR || | ||
selector == FILL_OR_KILL_ORDER_SELECTOR | ||
selector == IExchange(address(0)).fillOrder.selector || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is 🔥. I'm so glad we're getting rid of LibExchangeSelectors
.
tokenId | ||
); | ||
|
||
(bool success, bytes memory returnData) = tokenAddress.staticcall(ownerOfCalldata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would make sense to make a staticcall
library that you could use to abstract away some of this logic. For example, this staticcall could be written as:
balance = tokenAddress.staticcallReturnsAddress(ownerOfCalldata) == ownerAddress ? 1 : 0;
This is probably out of scope for this PR, but I think that it would be a good thing to add to the backlog.
transferableMakerFeeAssetAmount, | ||
makerFee, | ||
takerAssetAmount | ||
); | ||
transferableTakerAssetAmount = _min256(transferableMakerToTakerAmount, transferableMakerFeeToTakerAmount); | ||
transferableTakerAssetAmount = LibSafeMath.min256(transferableMakerToTakerAmount, transferableMakerFeeToTakerAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use transferableMakerToTakerAmount.min256(transferableMakerFeeToTakerAmount)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt like that was less readable, but I could go either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a huge preference either way.
} | ||
} | ||
|
||
// `fillableTakerAssetAmount` is the lower of the order's remaining `takerAssetAmount` and the `transferableTakerAssetAmount` | ||
fillableTakerAssetAmount = _min256( | ||
_safeSub(takerAssetAmount, orderInfo.orderTakerAssetFilledAmount), | ||
fillableTakerAssetAmount = LibSafeMath.min256( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -171,7 +172,7 @@ contract OrderValidationUtils is | |||
returns (uint256 transferableAssetAmount) | |||
{ | |||
(uint256 balance, uint256 allowance) = getBalanceAndAssetProxyAllowance(ownerAddress, assetData); | |||
transferableAssetAmount = _min256(balance, allowance); | |||
transferableAssetAmount = LibSafeMath.min256(balance, allowance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
uint256 remainingTakerAssetAmount = _safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); | ||
uint256 takerAssetFilledAmount = _min256(takerAssetFillAmount, remainingTakerAssetAmount); | ||
uint256 remainingTakerAssetAmount = order.takerAssetAmount.safeSub(orderInfo.orderTakerAssetFilledAmount); | ||
uint256 takerAssetFilledAmount = LibSafeMath.min256(takerAssetFillAmount, remainingTakerAssetAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as earlier.
)); | ||
bytes32 schemaHash = _EIP712_DOMAIN_SEPARATOR_SCHEMA_HASH; | ||
|
||
// Assembly for more efficient computing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
/// @param fillResults1 The first FillResults. | ||
/// @param fillResults2 The second FillResults. | ||
/// @return The sum of both fill results. | ||
function addFillResults( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of this not being a destructive function. I think that we should make this the norm (if it isn't already).
// If the left maker can buy exactly as much as the right maker can sell, then both orders are fully filled. | ||
if (leftTakerAssetAmountRemaining > rightMakerAssetAmountRemaining) { | ||
// Case 1: Right order is fully filled | ||
matchedFillResults = _calculateCompleteRightFill( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Really glad this is non-destructive now.
// leftTakerAssetAmountRemaining == rightMakerAssetAmountRemaining | ||
// Case 3: Both orders are fully filled. Technically, this could be captured by the above cases, but | ||
// this calculation will be more precise since it does not include rounding. | ||
matchedFillResults = _calculateCompleteFillBoth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solid! I'm happy we were able to fit this refactor into 3.0 (~‾▿‾)~
pragma solidity ^0.5.9; | ||
|
||
|
||
contract LibExchangeSelectors { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIP level 💯
import "../src/LibFillResults.sol"; | ||
|
||
|
||
contract TestLibFillResults { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of these public-to-internal wrappers now. It could be worth looking into auto-generating them. This would also alleviate some of the code bloat. I've created a task on our backlog to look into this.
import "./LibSafeMathRichErrors.sol"; | ||
|
||
|
||
library LibSafeMath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
// uint256(verifyingContractAddress) | ||
// )) | ||
|
||
assembly { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me ~ do you know roughly how much gas this saves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's super significant, maybe 1000 or so. Also note that this function only gets called once in the Exchange
contract. I mostly did this for consistency pre-vuln since all of the other hashing is done in assembly, but it would probably be a good time to formalize what we want to do with all of the hashing as a whole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, these are all much cleaner changes! 💯
Some small requests then probably g2g after rebasing against 3.0.
Not approving yet because I wanna take another look after the rebase.
contracts/exchange-libs/package.json
Outdated
@@ -35,7 +35,7 @@ | |||
"generate-exchange-selectors": "node lib/scripts/generate-exchange-selectors.js ../../../exchange/generated-artifacts/Exchange.json ./contracts/src/LibExchangeSelectors.sol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of this and the associated script now. 🎉
contract LibFillResults is | ||
SafeMath | ||
{ | ||
library LibFillResults { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging this refactor.
public | ||
view | ||
function getOrderHash(Order memory order, bytes32 eip712ExchangeDomainHash) | ||
internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder if we should expose a getOrderHash()
function in MixinExchangeCore
to make up for this being turned into an internal library. getOrderInfo()
might be overkill for some users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed this with Fabio and we agreed that we can just put this in the DevUtils
contract. Do you think that is sufficient?
public | ||
view | ||
function getTransactionHash(ZeroExTransaction memory transaction, bytes32 eip712ExchangeDomainHash) | ||
internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Consider exposing getTransactionHash()
in MixinTransactions
.
return orderHash; | ||
} | ||
|
||
/// @dev Calculates EIP712 hash of the order. | ||
/// @param order The order structure. | ||
/// @return EIP712 hash of the order. | ||
function _hashOrder(Order memory order) | ||
function hashOrder(Order memory order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just rename this to getHash()
now since order.hashOrder()
is stuttering.
env.provider, | ||
env.txDefaults, | ||
); | ||
chainId = await providerUtils.getChainIdAsync(env.provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, getting a legitimate chainId
doesn't seem to matter?
|
||
// Compute proportional fill amounts | ||
fillResults = _calculateFillResults(order, takerAssetFilledAmount); | ||
fillResults = LibFillResults.calculateFillResults(order, takerAssetFilledAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; | ||
import "@0x/contracts-utils/contracts/src/LibBytes.sol"; | ||
import "@0x/contracts-utils/contracts/src/LibEIP1271.sol"; | ||
|
||
|
||
interface ISimplifiedExchange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
@@ -1,57 +0,0 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't even know this contract existed.
} | ||
}); | ||
compilerJSON.contracts = _.sortBy(compilerJSON.contracts); | ||
if (compilerJSON.contracts !== undefined && compilerJSON.contracts !== ALL_CONTRACTS_IDENTIFIER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOICE! 👍
… contracts by default
be5107f
to
1dae1d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small nit then g2g! 👍 💃
makerFeePaid: constants.ZERO_AMOUNT, | ||
takerFeePaid: constants.ZERO_AMOUNT, | ||
}; | ||
const EMPTY_MATCHED_FILL_RESULTS: MatchedFillResults = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the matched fill results stuff closer to the tests that use them?
Description
LibMath
,LibFillResults
,LibOrder
,LibZeroExTransaction
, andLibSafeMath
to be actual libraries. Note that the oldSafeMath
still exists so that contracts that weren't yet refactored don't break. This can be removed in a separate PR.LibEIP712ExchangeDomain
to only calculate the domain hash in its constructor and nothing else.calculateFillResults
andcalculateMatchedFillResults
toLibFillResults
. All functions inLibFillResults
were also refactored to be pure. This refactor should probably be one of the focus areas of this review.IExchange(address(0)).fillOrder.selector
pattern.LibEIP712.hashEIP712Domain
to use assembly. This was actually written pre-vuln, but might be a good discussion to have for all of the hashing functions.contracts-gen
to accept acompiler.json
with no specified contracts. In this case, boilerplate for all contracts in the package will be generated. Thecontracts:gen
command is also added into thebuild
command for each package, so it no longer needs to be explicitly called._
prefix from internal function names in actual libraries.LibExchangeRichErrors
to theexchange-libs
package.Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.