Implement MultiAssetProxy #1224
Conversation
|
|
||
| // To lookup a value in a mapping, we load from the storage location keccak256(k, p), | ||
| // where k is the key left padded to 32 bytes and p is the storage slot | ||
| mstore(0, and(caller, 0xffffffffffffffffffffffffffffffffffffffff)) |
There was a problem hiding this comment.
This and is unnecessary, caller will already have the high bits set to zero.
| } | ||
|
|
||
| // `transferFrom`. | ||
| // The function is marked `external`, so no abi decodeding is done for |
|
|
||
| // Load number of elements in `amounts` | ||
| // We must add 4 (function selector) + 128 (4 params * 32) + 32 (assetData len) + 4 (assetProxyId) + 32 (amounts len) to the amounts offset | ||
| let amountsContentsStart := add(amountsOffset, 200) |
There was a problem hiding this comment.
This takes into account the offset in the inner data (amountsOffset) but ignores the outer offset (assetDataOffset) and assumes it is always fixed (zero I assume).
We should take all offsets into account: add(assetDataOffset, add(amountsOffset, 4 + 4 + 32)).
I tried using absolute addresses for all offsets, it doesn't make the code much more readable.
| let amountsLen := calldataload(sub(amountsContentsStart, 32)) | ||
|
|
||
| // Load number of elements in `nestedAssetData` | ||
| let nestedAssetDataContentsStart := add(nestedAssetDataOffset, 200) |
There was a problem hiding this comment.
Same here : add(assetDataOffset, add(nestedAssetDataOffset, 4 + 4 + 32)).
| mstore(64, 0x0000000f4c454e4754485f4d49534d4154434800000000000000000000000000) | ||
| mstore(96, 0) | ||
| revert(0, 100) | ||
| } |
There was a problem hiding this comment.
We assume without checking that the (nested) assetDatas are correctly formatted (follow ABI2 spec, no out of bounds errors, etc.). We don't check for that. We can similarly argue here that it is on the user to supply correct data. But I thing this check makes sense.
| let nestedAssetDataElementOffset := calldataload(add(nestedAssetDataContentsStart, i)) | ||
|
|
||
| // Load length of `nestedAssetData[i]` | ||
| let nestedAssetDataElementContentsStart := add(nestedAssetDataElementOffset, 264) |
There was a problem hiding this comment.
:= add(assetDataOffset, add(nestedAssetDataElementOffset, 4 + 4 + 32)). (I think)
|
|
||
| // To lookup a value in a mapping, we load from the storage location keccak256(k, p), | ||
| // where k is the key left padded to 32 bytes and p is the storage slot | ||
| mstore(132, assetProxyId) |
There was a problem hiding this comment.
132 is where nestedAssetDataElementLen should be stored for our outgoing calldata. assetProxyId would starting from 164.
Edit: doesn't matter, we overwrite anyway in the big calldatacopy. It does seem worth re-using these 4 bytes.
| // Copy `nestedAssetData[i]` from calldata to memory | ||
| calldatacopy( | ||
| 132, // memory slot after `amounts[i]` | ||
| nestedAssetDataElementOffset, // location of `nestedAssetData[i]` in calldata |
There was a problem hiding this comment.
This does not take offset from outer assetData into account, nor the selectors. Should be add(assetDataOffset, add(nestedAssetDataElementOffset, 4 + 4)) or sub(nestedAssetDataElementContentsStart, 32).
| 0, // pointer to start of input | ||
| add(164, nestedAssetDataElementLen), // length of input | ||
| 100, // write output over memory that won't be reused | ||
| 512 // reserve 512 bytes for output |
There was a problem hiding this comment.
This assumes or proxies will never return more then 512 bytes. Reasonable assumption, but why not use returndatacopy and leave both of these zero?
| ) | ||
|
|
||
| if iszero(success) { | ||
| revert(100, returndatasize()) |
There was a problem hiding this comment.
returndatacopy(0, 0, returndatasize())
revert(0, returndatasize())
abandeali1
force-pushed
the
feature/contracts/multiAssetProxy
branch
from
13c5300
to
2174209
Compare
abandeali1
force-pushed
the
feature/contracts/multiAssetProxy
branch
2 times, most recently
from
9b6f8ec
to
551efd1
Compare
abandeali1
force-pushed
the
feature/contracts/multiAssetProxy
branch
from
551efd1
to
53bb98a
Compare
| 0, // pointer to start of input | ||
| add(164, nestedAssetDataElementLen), // length of input | ||
| 0, // write output over memory that won't be reused | ||
| 0 // reserve 512 bytes for output |
There was a problem hiding this comment.
// reserve 512 bytes for output - do we want to explicitly reserve 512 bytes here?
There was a problem hiding this comment.
Whoops, outdated comment
| // where k is the key left padded to 32 bytes and p is the storage slot | ||
| mstore(132, assetProxyId) | ||
| mstore(164, assetProxies_slot) | ||
| let assetProxy := sload(keccak256(132, 64)) |
There was a problem hiding this comment.
One possible optimization: If assetProxyId is the same as the previous iteration then we could skip this check. Clients could then optimize their calldata by grouping-by-proxy.
| }); | ||
| it('should have an id of 0x94cfcdd7', async () => { | ||
| const proxyId = await multiAssetProxy.getProxyId.callAsync(); | ||
| const expectedProxyId = '0x94cfcdd7'; |
There was a problem hiding this comment.
Could be good to add a note about how this was derived.
|
🔥 🔥 🔥 |
abandeali1
force-pushed
the
feature/contracts/multiAssetProxy
branch
from
53bb98a
to
0e36f4c
Compare
abandeali1
force-pushed
the
feature/contracts/multiAssetProxy
branch
from
0e36f4c
to
708f4e9
Compare
Description
Implements 0xProject/ZEIPs#23
TODO: add combinatorial tests
TODO: use custom ABI encoder and update
assetDataUtilsTODO: determine if we can safely remove sanity checks
Testing instructions
Types of changes
Checklist:
[WIP]if necessary.[sol-cov] Fixed bug.