limit order metatransactions #44
Conversation
…around ExecuteStates
smarx
requested review from
abandeali1,
dorothy-zbornak,
hysz and
moodlezoup
as code owners
| bytes memory args = new bytes(data.length - 4); | ||
| uint256 from; | ||
| uint256 to; | ||
| assembly { |
There was a problem hiding this comment.
Since there are no dynamic fields, I wonder if we could easily get away with just copying fields from the calldata directly into the structs. Ex,
// makerToken
mstore(order, mload(add(data, 0x24)))
// takerToken
mstore(add(order, 0x20), mload(add(data, 0x44)))
// makerAmount
mstore(add(order, 0x40), mload(add(data, 0x64)))
...
mstore(signature, ...)
...
takerTokenFillAmount := mload(add(data, ...)))
There was a problem hiding this comment.
We can do that, but I'm not convinced it's going to be (much) better. We're already doing a low-level memory copy.
There was a problem hiding this comment.
Yeah I was thinking this could save us the abi.decode, which looking in Remix is ~4000 gas . Alternatively we could probably also just use memCpy to extract the order/signature.
There was a problem hiding this comment.
Actually given the time constraints I'm down to punt on optimizations for now. We can do more golfing in 4.1.
|
|
||
| return _callSelf( | ||
| state.hash, | ||
| abi.encodeWithSelector( |
There was a problem hiding this comment.
Actually... I wonder if if we could just modify in-place the mtx.state.callData. This is untested, but something like:
assembly {
// length on input calldata
let mtxCallDataLen := mload(data)
// we'll extend the input calldata with two extra fields (0x40)
mstore(data, add(mtxCallDataLen, 0x40))
// where in memory to append these fields (right after state.mtx.callData)
let mtxCallDataEnd := add(data, add(0x20, mul(mtxCallDataLen, 0x20)))
// add fields to end of state.mtx.callData
mstore(mtxCallDataEnd, taker)
mstore(add(mtxCallDataEnd, 0x20), sender)
}
There was a problem hiding this comment.
^^ we'd also have to replace the selector w/ INativeOrdersFeature._fillLimitOrder.selector
There was a problem hiding this comment.
IIUC, this is a bit dangerous. We'd be writing to memory that may not be safe to touch. How important is gas efficiency here?
There was a problem hiding this comment.
I guess we do control the struct, so we can make sure we have room to overwrite things. But I think we still need to decide how important gas efficiency is before we do that sort of thing.
There was a problem hiding this comment.
Yeah... I was thinking we could probably show it's safe so long as we don't add code after executing the MTX. But, looking more thoroughly, I see we could also be running in a batchExecute in which case it's probably pretty likely we're overwriting someone else's memory.
| bytes memory data = state.mtx.callData; | ||
| bytes memory args = new bytes(data.length - 4); | ||
| uint256 from; | ||
| uint256 to; | ||
| assembly { | ||
| from := add(data, 36) // skip length and selector | ||
| to := add(args, 32) // after length | ||
| } | ||
| LibBytesV06.memCopy(to, from, data.length - 4); |
There was a problem hiding this comment.
Can we refactor this out into a stripSelectorFromCallData() function or something?
| @@ -319,6 +319,7 @@ contract NativeOrdersFeature is | |||
| address sender | |||
| ) | |||
| public | |||
| virtual | |||
There was a problem hiding this comment.
I really hate this keyword.
| bytes32 greedyTokensBloomFilter | ||
| ) | ||
| public | ||
| NativeOrdersFeature(zeroExAddress, weth, staking, protocolFeeMultiplier, greedyTokensBloomFilter) |
There was a problem hiding this comment.
Could probably just populate all these with nulls here to reduce setup in the test script.
Description
This adds support for metatransactions that invoke
fillLimitOrderandfillRfqOrder.Testing instructions
yarn testTypes of changes
Checklist:
[WIP]if necessary.