-
Notifications
You must be signed in to change notification settings - Fork 465
@0x/contracts-broker
: Property-based Gods Unchained orders
#2455
Conversation
@0x/contracts-broker
: Property-based Gods Unchained orders@0x/contracts-broker
: Property-based Gods Unchained orders
19e2559
to
bebf90f
Compare
|
@0x/contracts-broker
: Property-based Gods Unchained orders@0x/contracts-broker
: Property-based Gods Unchained orders
9be5661
to
2a4a419
Compare
9992650
to
1fc449b
Compare
@0x/contracts-broker
: Property-based Gods Unchained orders@0x/contracts-broker
: Property-based Gods Unchained orders
1fc449b
to
c5267b6
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.
Nice work! I really like the simplicity of this design.
) | ||
external | ||
{ | ||
if (from != address(this)) { |
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 just ignore the from
parameter and always use address(this)
?
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 put this in to handle the case where e.g, makerFeeAssetData = takerAssetData
both point to the Broker. Without this check, the Broker would send one of the taker's assets to the fee recipient, but this would be disguised as a maker fee
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.
Could this also be an issue for the taker fee? There's nothing explicitly preventing filling an order w/ a taker fee right?
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.
Yeah, I suppose so, but at least if the takerFee
is non-zero the taker should expect to be sending something to the maker, so it's kinda on them/the front-end to surface what that thing is. Also I can't really think of an easy way for the Broker to distinguish between a taker -> maker transfer vs a taker -> feeRecipient transfer
uint256[] calldata amounts, | ||
bytes calldata data | ||
) | ||
external |
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 should add a modifier that checks to make sure msg.sender == ERC1155Proxy
here.
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.
should we check that msg.sender == _EXCHANGE
in the fallback while we're at it?
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.
or I guess if we go with the WETH wrapping/unwrapping approach, msg.sender == _WETH
/// @param fillFunctionSelector The selector for either `fillOrder` or `fillOrKillOrder`. | ||
/// @return fillResults Amounts filled and fees paid by the maker and taker. | ||
function brokerTrade( | ||
bytes[] memory brokeredAssets, |
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.
Since these will always be 721 tokens, do we actually need any information other than the token IDs? It would be much cheaper to only cache a uint256[]
if we could get away with it.
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.
yeah, we can encode the token address in the 1155 asset data at the cost of a little generality
alternatively, I suppose we could save one storage word per asset while retaining full generality by using abi.encode(tokenAddress, tokenId)
instead of the assetData
|
||
// Clear storage | ||
delete _cachedAssetData; | ||
_cacheIndex = 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.
We don't actually need this for a single fill right?
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 do in the case that the order's takerAssetAmount > 1
.
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 really elegant! Still gonna think about it a bit more before approving but I'm really digging it so far.
Integration tests even look nice or w/e 🙄
bytes[] internal _cachedAssetData; | ||
uint256 internal _cacheIndex; | ||
address internal _sender; | ||
address internal _EXCHANGE; // solhint-disable-line var-name-mixedcase |
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.
Could we get comments for these?
contracts/broker/contracts/src/validators/GodsUnchainedValidator.sol
Outdated
Show resolved
Hide resolved
Another thought - if this contract approves the |
hm, that's an interesting thought... I guess this would look like:
|
I don't think you'd actually need to do any wrapping/unwrapping. The Broker receives WETH as part of the trade and then uses that to pay protocol fees automatically if |
This would cause problems if |
True, but nothing is stopping anyone from including ETH in the transaction if the |
Not sure if I follow –– If |
Nvm you're right. I was thinking about how the Wrapping/unwrapping seems like the most logical approach to handle all of the edge cases then. Pretty much exactly what the |
I guess at that point we might as well approve the ERC20Proxy as well, as this would enable WETH taker fees and filling orders with |
/// @param to This should be the maker of the order. | ||
/// @param amounts Should be an array of just one `uint256`, specifying the amount of the brokered assets to transfer. | ||
/// @param data Encodes the validator contract address and any auxiliary data it needs for property validation. | ||
function safeBatchTransferFrom( |
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.
OK, so here is my admittedly kind of lame attack vector for this and why we should probably be checking that msg.sender == ERC1155Proxy
:
- Maker creates one of these orders with
makerFeeAssetData = ERC1155(...)
andfeeRecipient == maker
. - Taker is like, w/e not my fee not my problem so I'll fill this order.
- Taker is also like, my sloppy ass will pass in more ERC721s than the fill/order wants because the broker shouldn't use them anyway.
- Exchange fills taker -> maker, which leaves behind the excess ERC721s "in" the broker contract.
- Exchange then fills maker fee, which is an 1155 so it calls
onERC1155BatchReceived()
onmaker
which, surprise, is a smart contract. - In
onERC1155BatchReceived()
the maker callsbroker.safeBatchTransferFrom(address(broker), maker, [], [1], hex"0x...")
- Maker walks away with an extra ERC721.
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 actually the scenario that the if (from != address(this))
protects against. But yeah, I'm going to add a msg.sender == ERC1155Proxy
check just in case we're missing an edge case
) | ||
external | ||
{ | ||
if (from != address(this)) { |
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.
Could this also be an issue for the taker fee? There's nothing explicitly preventing filling an order w/ a taker fee right?
259c389
to
f02dc29
Compare
Added forwarder-like affiliate fees and WETH support |
f02dc29
to
efe200e
Compare
efe200e
to
d0865b2
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.
Seems legit! 👍
"version": "6.0.0", | ||
"changes": [ | ||
{ | ||
"note": "New year, new me: remove everything, add MixinWethUtils and LibAssetDataTransfer", |
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.
PKG=@0x/contracts-extensions yarn build | ||
PKG=@0x/contracts-dev-utils yarn build |
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.
😲
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, this is is really good shape! The refactors to the Forwarder are also 🔥 .
/// @param initialWethAmount Amount of WETH available after transferring affiliate fees. | ||
/// @param wethSpent Amount of WETH spent when filling orders. | ||
/// @dev Unwraps and refunds WETH to msg.sender. | ||
/// @param refundAmount Amount of WETH balance to refund. | ||
function _transferEthRefund( |
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.
Nit: now that this is in a more generic package, it probably makes sense to give this a more generic name. The first name that comes to mind is withdrawAndTransferEth
.
f44c2e4
to
eb4c697
Compare
eb4c697
to
0691cc7
Compare
Description
Implements the Broker and GodsUnchainedValidator contracts, as described in 0xProject/ZEIPs#75
Also moves
LibAssetDataTransfer
from@0x/contracts-exchange-forwarder
to@0x/contracts-exchange-libs
.A couple of things to note:
batchBrokerTrade
for the same reason.msg.sender
checks to the fallback andsafeBatchTransferFrom
.takerAssetAmount
and the nested 1155 amounts work together is somewhat interesting for brokered assets. Refer to the comment aboveencodeBrokerAssetData
for detailssafeBatchTransferFrom
is fairly opinionated in that it enforces thatfrom == address(this)
and assumes the brokered asset is 721. Generalizing this could open up a few interesting use cases, but this can wait for a later iteration.Testing instructions
Unit tests for the GodsUnchainedValidator contract are found in the new
@0x/contracts-broker
package, while Broker <> GodsUnchainedValidator integration tests are in@0x/contracts-integrations
Types of changes
Checklist:
[WIP]
if necessary.