Balance Threshold Filter #1383
Balance Threshold Filter #1383
Conversation
hysz
force-pushed
the
feature/contracts/balanceThresholdFilter
branch
from
9c2fed6 to
60fa565
Compare
| /// @dev Records addresses to be validated when Exchange transaction is a batch fill variant. | ||
| /// This is one of: batchFillOrders, batchFillOrKillOrders, batchFillNoThrow | ||
| /// Reference signature<T>: <batchFillVariant>(Order[],uint256[],bytes[]) | ||
| function recordAddressesForBatchFillVariant() { |
There was a problem hiding this comment.
Since this includes batchCancel, I'd just name this function recordAddressesForBatchVariant.
| /// @dev Records addresses to be validated when Exchange transaction is a fill order variant. | ||
| /// This is one of: fillOrder, fillOrKillOrder, fillOrderNoThrow | ||
| /// Reference signature<T>: <fillOrderVariant>(Order,uint256,bytes) | ||
| function recordAddressesForFillOrderVariant() { |
There was a problem hiding this comment.
Rename to recordAddressesForSingleOrderVariant.
| /// @dev Records addresses to be validated when Exchange transaction is a market fill variant. | ||
| /// This is one of: marketBuyOrders, marketBuyOrdersNoThrow, marketSellOrders, marketSellOrdersNoThrow | ||
| /// Reference signature<T>: <marketFillInvariant>(Order[],uint256,bytes[]) | ||
| function recordAddressesForMarketFillVariant() { |
There was a problem hiding this comment.
I think we can just remove this function and lump it in with the rest of the batch variants.
| */ | ||
|
|
||
| pragma solidity 0.4.24; | ||
| pragma experimental ABIEncoderV2; |
There was a problem hiding this comment.
We should be able to remove pragma experimental ABIEncoderV2; throughout all of these contracts.
| import "./MixinBalanceThresholdFilterCore.sol"; | ||
|
|
||
|
|
||
| contract BalanceThresholdFilter is MixinBalanceThresholdFilterCore { |
There was a problem hiding this comment.
nit: all of our other contracts put inherited contracts on a newline:
contract BalanceThresholdFilter is
MixinBalanceThresholdFilterCore
{}
| /// @param signerAddress Address of transaction signer. | ||
| /// @param signedExchangeTransaction AbiV2 encoded calldata. | ||
| /// @param signature Proof of signer transaction by signer. | ||
| function executeTransaction( |
There was a problem hiding this comment.
external and public functions should live in an interface (which the mixin can inherit)
| /// @dev Records addresses to be validated when Exchange transaction is a market fill variant. | ||
| /// This is one of: marketBuyOrders, marketBuyOrdersNoThrow, marketSellOrders, marketSellOrdersNoThrow | ||
| /// Reference signature<T>: <marketFillInvariant>(Order[],uint256,bytes[]) | ||
| function recordAddressesForMarketFillVariant() { |
There was a problem hiding this comment.
I'd merge this with recordAddressesForBatchFillVariant (I'd consider the market fill functions to be a subset of the batch fill functions).
| // See ../../utils/ExchangeSelectors/ExchangeSelectors.sol for selectors | ||
| // solhint-disable no-empty-blocks | ||
| switch exchangeFunctionSelector | ||
| case 0x297bb70b00000000000000000000000000000000000000000000000000000000 { recordAddressesForBatchFillVariant() } // batchFillOrders |
There was a problem hiding this comment.
maybe add comments above each of these selectors with the function names
| /// @param addressToValidate - Address to record for validation. | ||
| function recordAddressToValidate(addressToValidate) { | ||
| // Compute `addressesToValidate` memory offset | ||
| let addressesToValidate_ := mload(0x40) |
There was a problem hiding this comment.
It might be cleaner to do this without creating new temporary variables (I find these variable names confusing). All we really need to do is 1. increment length, 2. store new address at free memory ptr, 3. increment free memory ptr.
| ///// Validate Recorded Addresses ///// | ||
|
|
||
| // Load from memory the addresses to validate | ||
| let addressesToValidate := mload(0x40) |
There was a problem hiding this comment.
It feels safer and more descriptive to make this let addressesToValidate := mload(add(validatedAddresses, 0x20)) (only really matters if we're manually incrementing the free memory ptr as suggested above).
| mstore(add(freeMemPtr, 0x04), mload(addressToValidate)) | ||
|
|
||
| // call `THRESHOLD_ASSET.balanceOf` | ||
| let success := call( |
There was a problem hiding this comment.
I think it makes sense to make this a staticcall. Thoughts?
| 0x20 // reserve space for return balance (0x20 bytes) | ||
| ) | ||
| if eq(success, 0) { | ||
| // @TODO Revert with `Error("BALANCE_QUERY_FAILED")` |
hysz
force-pushed
the
feature/contracts/balanceThresholdFilter
branch
2 times, most recently
from
fe2efbe to
a10418b
Compare
| IThresholdAsset thresholdAsset = THRESHOLD_ASSET; | ||
| for (uint256 i = 0; i < addressesToValidate.length; ++i) { | ||
| uint256 addressBalance = thresholdAsset.balanceOf(addressesToValidate[i]); | ||
| if (addressBalance < balanceThreshold) { |
There was a problem hiding this comment.
Let's make this a require statement.
| import "@0x/contracts-utils/contracts/utils/LibBytes/LibBytes.sol"; | ||
|
|
||
|
|
||
| library LibAddressArray { |
There was a problem hiding this comment.
Maybe rename this LibAddress and we can add in other helper functions later if necessary?
hysz
force-pushed
the
feature/contracts/balanceThresholdFilter
branch
from
20c2bd7 to
ca0ab38
Compare
Description
This supersedes #1358. This is a more generalized implementation with support for more Exchange functions. Also includes wrappers.
Testing instructions
Types of changes
Checklist:
[WIP]if necessary.