Implement StaticCallProxy #1863

abandeali1 · 2019-06-10T04:13:13Z

Description

Implements ZEIP-39
Adds support to types and order-utils
Updates abi-gen-templates to include getABIEncodedTransactionData in the callAsync template (rather than the tx template). This was useful for implementing tests for the StaticCallProxy.
Includes a more descriptive revert reason in the ERC1155Proxy (also used in the StaticCallProxy)
Adds StaticCallProxy support to LibAssetData

Testing instructions

Types of changes

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2019-06-10T06:13:55Z

Coverage decreased (-0.1%) to 83.287% when pulling 323fb0a on feat/contracts/static-call-proxy into fc25752 on development.

jalextowle · 2019-06-11T00:14:16Z

contracts/asset-proxy/contracts/src/StaticCallProxy.sol

+                // + 32 (assetData len)
+                // + 4 (proxyId)
+                // + 32 (callTarget)
+                // + 32 (assetDataOffset)


This should be + 32 (staticCallDataOffset) instead of + 32 (assetDataOffset)

hysz

Looking good ~ I'm excited for this functionality 💪 ! Just left a few small notes.

hysz · 2019-06-17T16:30:52Z

contracts/asset-proxy/contracts/test/TestStaticCallTarget.sol

+        pure
+    {}
+
+    function dynamicInputFunction(bytes calldata a)


It could be good to do some sanity check on the input data, to ensure it was forwarded correctly.

hysz · 2019-06-17T16:31:32Z

contracts/asset-proxy/contracts/test/TestStaticCallTarget.sol

+        return isOdd;
+    }
+
+    function noInputFunction()


It could be good to do a sanity check to ensure that calldata() == <selector> without any extra data.

hysz · 2019-06-17T16:36:15Z

contracts/dev-utils/contracts/src/LibAssetData.sol

+            (bool success,) = _STATIC_CALL_PROXY_ADDRESS.staticcall(transferFromData);
+
+            // Success means that the staticcall can be made an unlimited amount of times
+            balance = success ? _MAX_UINT256 : 0;


Could be worthwhile to special-case Multi-Asset Data, especially in this case where the only valid values are MAX_UINT256 and 0. The alternative would be to highlight in our documentation that any non-zero value implies an unlimited amount of times, in which case we should add a note to the function comment + add a test case for this.

I'm also not sure exactly how to interpret "the staticcall can be made an unlimited amount of times". What if the following function is called:

function a() public view { require(block.number < 7977075); }

That call can be made an unlimited amount of times at the particular block in which this gets called (not accounting for gas). This should always be true since the staticcall can't update state and will therefore never have side effects that would prevent the staticcall from failing if called again.

hysz · 2019-06-17T16:53:58Z

contracts/asset-proxy/contracts/src/StaticCallProxy.sol

+                // |          | 4      |         |   1. offset to assetData (*)        |
+                // |          | 36     |         |   2. from                           |
+                // |          | 68     |         |   3. to                             |
+                // |          | 100    |         |   4. amount                         |


Since we're moving towards localizing validation within the proxies, we may want to assert that amount is some fixed, standardized value -- like 0. Ex, in the MultiAssetProxy scaling this value should have no effect.

The problem is that the Exchange contract doesn't allow 0 transfers. Any non-zero number would be scaled with the MAP.

dorothy-zbornak

Really nice!

dorothy-zbornak · 2019-06-17T21:00:10Z

contracts/asset-proxy/contracts/src/StaticCallProxy.sol

+
+                // Validate length of `assetData`
+                let assetDataLen := calldataload(assetDataOffset)
+                if or(lt(assetDataLen, 100), mod(sub(assetDataLen, 4), 32)) {


Suggested change

if or(lt(assetDataLen, 100), mod(sub(assetDataLen, 4), 32)) {

if lt(assetDataLen, 132) {

staticCallData Length should always be part of the asset data regardless, no?

Do we need to enforce that the entire asset data be word-aligned? It's conceivable to me that an abi-encoder (though probably not ours) would not word-align parameters such as bytes types.

True, but the offset to staticCallData can point to inside the first 100 bytes, which could technically be valid.

From the ABI spec (https://solidity.readthedocs.io/en/v0.5.9/abi-spec.html):

bytes, of length k (which is assumed to be of type uint256): enc(X) = enc(k) pad_right(X), i.e. the number of bytes is encoded as a uint256 followed by the actual value of X as a byte sequence, followed by the minimum number of zero-bytes such that len(enc(X)) is a multiple of 32.

Interesting. I guess this also explains why we word align revert strings.

dorothy-zbornak · 2019-06-17T22:16:08Z

contracts/exchange/test/core.ts

-            }),
-            constants.AWAIT_TRANSACTION_MINED_MS,
-        );
+        await erc20Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, { from: owner });


…t methods

…rget

abandeali1 requested review from fabioberger and hysz as code owners June 10, 2019 04:13

probot-autolabeler bot added 0x.js abi-gen-templates @0x/abi-gen-wrappers contracts labels Jun 10, 2019

abandeali1 force-pushed the feat/contracts/static-call-proxy branch 2 times, most recently from 6538e34 to 416045a Compare June 10, 2019 05:26

abandeali1 requested review from dorothy-zbornak and jalextowle June 10, 2019 05:39

abandeali1 force-pushed the feat/contracts/static-call-proxy branch from 4164577 to 6342faa Compare June 10, 2019 16:29

jalextowle suggested changes Jun 11, 2019

View reviewed changes

hysz reviewed Jun 17, 2019

View reviewed changes

abandeali1 force-pushed the feat/contracts/static-call-proxy branch from 6342faa to 5da88f7 Compare June 17, 2019 19:24

dorothy-zbornak suggested changes Jun 17, 2019

View reviewed changes

abandeali1 requested a review from albrow as a code owner June 18, 2019 04:50

abandeali1 force-pushed the feat/contracts/static-call-proxy branch from 47c84dd to 8897dc1 Compare June 18, 2019 05:32

abandeali1 requested a review from feuGeneA as a code owner June 18, 2019 05:32

dorothy-zbornak approved these changes Jun 18, 2019

View reviewed changes

abandeali1 added 10 commits June 18, 2019 19:50

Implement StaticCallProxy

4ed111a

Use more specific revert reason in ERC1155Proxy

f0ecda1

Add StaticCallProxy types and move 1155 method ABI into order-utils

720d335

Add StaticCallAssetData encoding and decoding

570c1e1

Add TestStaticCallTarget for testing StaticCallProxy

7d9e43b

Add boilerplate for StaticCallProxy

5345f7c

Allow getABIEncodedTransactionData to be called for read only contrac…

f51c4f9

…t methods

Add unit tests for StaticCallProxy

0c8bb2e

Regenerate abi-gen-wrappers

f2cbf4a

Add StaticCallProxy support to LibAssetData

5810e7d

abandeali1 added 6 commits June 18, 2019 19:52

Remove pipeline package from CI build

92ce258

Add fillOrder tests that use the StaticCallProxy

bf3ae73

Update CHANGELOGs

f15560b

Fix build errors

5dcf791

Fix StaticCallProxy comment and add sanity checks to TestStaticCallTa…

45aacf1

…rget

Bump solidity version pragma for undeployed AssetProxy contracts

323fb0a

abandeali1 force-pushed the feat/contracts/static-call-proxy branch from 8897dc1 to 323fb0a Compare June 19, 2019 02:53

jalextowle approved these changes Jun 20, 2019

View reviewed changes

hysz approved these changes Jun 20, 2019

View reviewed changes

abandeali1 merged commit 82f7308 into development Jun 21, 2019

xianny added @0x/abi-gen-templates and removed abi-gen-templates labels Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement StaticCallProxy #1863

Implement StaticCallProxy #1863

abandeali1 commented Jun 10, 2019 •

edited

Loading

coveralls commented Jun 10, 2019 •

edited

Loading

jalextowle Jun 11, 2019

hysz left a comment

hysz Jun 17, 2019

hysz Jun 17, 2019

hysz Jun 17, 2019

hysz Jun 17, 2019

abandeali1 Jun 17, 2019

hysz Jun 17, 2019

abandeali1 Jun 17, 2019

dorothy-zbornak left a comment

dorothy-zbornak Jun 17, 2019

abandeali1 Jun 18, 2019

dorothy-zbornak Jun 18, 2019

dorothy-zbornak Jun 17, 2019

	if or(lt(assetDataLen, 100), mod(sub(assetDataLen, 4), 32)) {
	if lt(assetDataLen, 132) {

Implement StaticCallProxy #1863

Implement StaticCallProxy #1863

Conversation

abandeali1 commented Jun 10, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

hysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandeali1 commented Jun 10, 2019 •

edited

Loading

coveralls commented Jun 10, 2019 •

edited

Loading