Decode Calldata + Return Values in Contract Wrappers #2018

hysz · 2019-07-30T18:49:07Z

Description

This adds ABI decoding of transaction calldata and return values to contract wrappers. These are particularly useful for meta-transactions and the delegate-call proxy pattern.

This PR includes the following changes:

Adds getABIDecodedTransactionData to contract wrappers, which decodes calldata.
Adds getABIDecodedReturnData to contract wrappers, which decodes return values.
Adds bounds-checking to the 0x abi encoder. Previously we interpreted anything past calldata as a zero; now reading past the end of calldata throws an exception. This is aligned with decoding in solidity and web3.
Adds a strictDecode to the abi encoder, which decodes to the same format as the function's arguments.

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.

buildsize · 2019-07-30T19:02:04Z

File name	Previous Size	New Size	Change
init.py	141.32 KB	141.32 KB	0 bytes (0%)
abi_gen_dummy.ts	65.64 KB	[deleted]
lib_dummy.ts	4.74 KB	[deleted]
test_lib_dummy.ts	9.8 KB	[deleted]
environment.pickle	1.56 MB	1.56 MB	0 bytes (0%)
index.doctree	189.53 KB	189.53 KB	0 bytes (0%)
.buildinfo	230 bytes	230 bytes	0 bytes (0%)
genindex.html	5.6 KB	5.6 KB	0 bytes (0%)
index.html	2.52 KB	2.52 KB	0 bytes (0%)
objects.inv	375 bytes	375 bytes	0 bytes (0%)
py-modindex.html	3.07 KB	3.07 KB	0 bytes (0%)
search.html	2.81 KB	2.81 KB	0 bytes (0%)
searchindex.js	5.83 KB	5.83 KB	0 bytes (0%)
index.rst.txt	415 bytes	415 bytes	0 bytes (0%)
alabaster.css	10.92 KB	10.92 KB	0 bytes (0%)
basic.css	11.83 KB	11.83 KB	0 bytes (0%)
custom.css	42 bytes	42 bytes	0 bytes (0%)
doctools.js	9.05 KB	9.05 KB	0 bytes (0%)
documentation_options.js	303 bytes	303 bytes	0 bytes (0%)
file.png	286 bytes	286 bytes	0 bytes (0%)
jquery-[version].js	261.76 KB	261.76 KB	0 bytes (0%)
jquery.js	84.63 KB	84.63 KB	0 bytes (0%)
language_data.js	10.59 KB	10.59 KB	0 bytes (0%)
minus.png	90 bytes	90 bytes	0 bytes (0%)
plus.png	90 bytes	90 bytes	0 bytes (0%)
pygments.css	4.69 KB	4.69 KB	0 bytes (0%)
searchtools.js	15.61 KB	15.61 KB	0 bytes (0%)
underscore-[version].js	34.34 KB	34.34 KB	0 bytes (0%)
underscore.js	11.86 KB	11.86 KB	0 bytes (0%)
contract_addresses.html	16.87 KB	16.87 KB	0 bytes (0%)
contract_artifacts.html	7.94 KB	7.94 KB	0 bytes (0%)
_bootstrap.html	142.73 KB	142.73 KB	0 bytes (0%)
json_schemas.html	12.43 KB	12.43 KB	0 bytes (0%)
order_utils.html	44.87 KB	44.87 KB	0 bytes (0%)
erc20_token.html	80.14 KB	80.14 KB	0 bytes (0%)
exchange.html	458.9 KB	458.9 KB	0 bytes (0%)
tx_params.html	8.83 KB	8.83 KB	0 bytes (0%)
local_message_signer.html	15.07 KB	15.07 KB	0 bytes (0%)
asset_data_utils.html	22.65 KB	22.65 KB	0 bytes (0%)
default_api.html	118.49 KB	118.49 KB	0 bytes (0%)

coveralls · 2019-07-30T19:10:04Z

Coverage increased (+0.05%) to 82.114% when pulling 33d8646 on feature/ContractWrappers/abiEncodeDecodeTxData into 57318c0 on development.

xianny

Oh man, this is great! Thanks for implementing this!

The main change I would request is to move the tests from contracts/utils to abi-gen. It looks like they are testing the getAbiDecodedTransactionData and getAbiDecodedReturnData methods whose source code is in abi-gen-templates. We're planning on combining abi-gen-templates and abi-gen soon - for now, we are trying to build up a test suite in abi-gen. I suggest moving the test cases in TestAbi.sol to packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol, and adding the unit tests to packages/abi-gen/test-cli/test_typescript/test/abi_gen_dummy_test.ts.

and a huge thank you for adding test coverage!

jalextowle

I left a few questions about the Dummy smart contract, but otherwise this looks really nice 🍻 !

jalextowle · 2019-07-31T21:09:05Z

packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol

+    /// This allows us to test `getABIDecodedTransactionData` and `getABIDecodedReturnData` that is
+    /// include in contract wrappers.
+    // solhint-disable no-complex-fallback
+    function ()


Doesn't this function just loop forever? If the calldata hits the fallback function, then the fallback will call back into this contract using the same calldata, which would intuitively also make it hit the fallback function, creating an infinite loop. Please let me know if I'm wrong about this, but I've done a bit of testing locally and it seems to loop forever.

Wouldn't this never get called if used as intended because the functions it's forwarding to are defined in this contract?

Ah, yeah you're right this can be removed now that all tests are in the same contract. 🙏

@dorothy-zbornak Yeah, I think that is the case. I was just saying that it would loop infinitely if it was called for some reason.

jalextowle · 2019-07-31T21:15:01Z

packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol

+        payable
+    {
+        address addr = address(this);
+        assembly {


Is there a reason why you're using assembly here? It seems like you could use high-level solidity for this.

jalextowle · 2019-07-31T21:15:20Z

packages/utils/test/abi_encoder/methods_test.ts

-    const encodingRules: AbiEncoder.EncodingRules = { shouldOptimize: false }; // optimizer is tested separately.
+    const defaultEncodingRules: AbiEncoder.EncodingRules = { shouldOptimize: false }; // optimizer is tested separately.
+    const defaultDecodingRules: AbiEncoder.DecodingRules = { shouldConvertStructsToObjects: false };
+    const runTest = <T>(


This function is 🔥

dorothy-zbornak

New to this package, but looks pretty good. Just some Qs.

dorothy-zbornak · 2019-07-31T21:53:05Z

packages/abi-gen-templates/partials/callAsync.handlebars

@@ -44,3 +44,21 @@ getABIEncodedTransactionData(
    const abiEncodedTransactionData = self._strictEncodeArguments('{{this.functionSignature}}', [{{> normalized_params inputs=inputs}}]);
    return abiEncodedTransactionData;
 },
+getABIDecodedTransactionData(
+    returnData: string


Why is this called returnData instead of callData?

Typo, thank you 🙏

dorothy-zbornak · 2019-07-31T22:10:51Z

packages/utils/src/abi_encoder/evm_data_types/method.ts

@@ -34,6 +34,19 @@ export class MethodDataType extends AbstractSetDataType {
        return value;
    }

+    public strictDecode<T>(calldata: string, rules?: DecodingRules): T {
+        const value = super.decode(calldata, rules, this._methodSelector);


I can't find any tests where we check that decode() with a mismatched method selector throws. Do we have one?

Great catch -- test added!

dorothy-zbornak · 2019-07-31T22:24:50Z

packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol

+    /// This allows us to test `getABIDecodedTransactionData` and `getABIDecodedReturnData` that is
+    /// include in contract wrappers.
+    // solhint-disable no-complex-fallback
+    function ()


Wouldn't this never get called if used as intended because the functions it's forwarding to are defined in this contract?

dorothy-zbornak · 2019-07-31T22:25:10Z

packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol

+    }
+
+    /// @dev The fallback function calls into this contract and executes one of the above functions.
+    /// This allows us to test `getABIDecodedTransactionData` and `getABIDecodedReturnData` that is


I'm not really sure how this can be used to test getABIDecodedTransactionData(). Where are the tests that use this?

See below 🙏

… contract wrappers + test cases

…-gen

jalextowle

Looks great!

xianny

Just one note about using comments to organise AbiGenDummy.sol. Feel free to change or not. Looks good to me!

xianny · 2019-08-01T20:16:08Z

packages/abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol

@@ -138,4 +138,90 @@ contract AbiGenDummy
    uint someState;
    function nonPureMethod() public returns(uint) { return someState += 1; }
    function nonPureMethodThatReturnsNothing() public { someState += 1; }
+


We've been keeping all the test cases in one contract to speed up test run times. I'm thinking about how we could keep it somewhat organised, since we're working towards having full coverage of Solidity language features. We don't have this convention yet, but what about prefixing a section of test cases with the methods it's meant to test?

e.g.

// begin tests for `decodeTransactionData`, `decodeReturnData` ... // end tests for `decodeTransactionData`, `decodeReturnData`

Why not just use inheritance?

@xianny I've added comments for now for consistency, although I agree w Dorothy - it could be good to split this into multiple contracts alongside the refactor of abi-gen.

Ya, let's keep that in mind. I'm a little concerned that inheritance might complicate testing, especially when we add the EVM simulator. As in we would have to test whatever case we want PLUS inheritance.

xianny · 2019-08-01T20:16:48Z

packages/abi-gen/test-cli/test_typescript/test/abi_gen_dummy_test.ts

@@ -28,6 +28,20 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);

 describe('AbiGenDummy Contract', () => {
    let abiGenDummy: AbiGenDummyContract;
+    const runTestAsync = async (contractMethod: any, input: any, output: any) => {


love this setup! 👍

dorothy-zbornak

╰(▔∀▔)╯

hysz requested review from abandeali1, feuGeneA and xianny as code owners July 30, 2019 18:49

hysz changed the title ~~[WIP] Decode Calldata + Return Values in Contract Wrappers~~ Decode Calldata + Return Values in Contract Wrappers Jul 31, 2019

hysz force-pushed the feature/ContractWrappers/abiEncodeDecodeTxData branch from dc30e10 to a90783c Compare July 31, 2019 15:32

xianny suggested changes Jul 31, 2019

View reviewed changes

hysz assigned dorothy-zbornak and jalextowle Jul 31, 2019

dorothy-zbornak requested a review from jalextowle July 31, 2019 18:44

hysz force-pushed the feature/ContractWrappers/abiEncodeDecodeTxData branch 2 times, most recently from a5f7152 to 118818b Compare July 31, 2019 19:49

jalextowle reviewed Jul 31, 2019

View reviewed changes

dorothy-zbornak reviewed Jul 31, 2019

View reviewed changes

hysz added 11 commits August 1, 2019 19:00

Added getABIDecodedTransactionData and getABIDecodedReturnData to…

db74db6

… contract wrappers + test cases

Do not allow allow reading beyond calldata

547322a

Added tests for strict decoding of method arguments

f97ee80

updated abi-gen-wrappers

5b4c29c

updated expected output for abi-gen cli tests

7c199d8

updated changelogs

884864c

Appeased the linter

0d4dd5f

Moved contract wrapper abi tests into abi-gen package

03f04f4

removed unnecessary fallback function from abi-gen tests

92602d3

Added abi encoder test when bad selector is passed to method decoding

da28a54

Fixed typo in callAsync.handlebars and updated expected output in abi…

6787629

…-gen

hysz force-pushed the feature/ContractWrappers/abiEncodeDecodeTxData branch from 26d2dcd to 6787629 Compare August 1, 2019 17:07

Ran prettier

7eb2d29

jalextowle approved these changes Aug 1, 2019

View reviewed changes

xianny approved these changes Aug 1, 2019

View reviewed changes

dorothy-zbornak approved these changes Aug 1, 2019

View reviewed changes

added start/end test comments to AbiGenDummy for consistency

33d8646

hysz merged commit 76ca211 into development Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode Calldata + Return Values in Contract Wrappers #2018

Decode Calldata + Return Values in Contract Wrappers #2018

hysz commented Jul 30, 2019 •

edited

Loading

buildsize bot commented Jul 30, 2019 •

edited

Loading

coveralls commented Jul 30, 2019 •

edited

Loading

xianny left a comment •

edited

Loading

jalextowle left a comment

jalextowle Jul 31, 2019

dorothy-zbornak Jul 31, 2019

hysz Aug 1, 2019

jalextowle Aug 1, 2019

jalextowle Jul 31, 2019

jalextowle Jul 31, 2019

dorothy-zbornak left a comment •

edited

Loading

dorothy-zbornak Jul 31, 2019

hysz Aug 1, 2019 •

edited

Loading

dorothy-zbornak Jul 31, 2019

hysz Aug 1, 2019

dorothy-zbornak Jul 31, 2019

dorothy-zbornak Jul 31, 2019

hysz Aug 1, 2019

jalextowle left a comment

xianny left a comment

xianny Aug 1, 2019

dorothy-zbornak Aug 1, 2019

hysz Aug 2, 2019

xianny Aug 2, 2019

xianny Aug 1, 2019

dorothy-zbornak left a comment

Decode Calldata + Return Values in Contract Wrappers #2018

Decode Calldata + Return Values in Contract Wrappers #2018

Conversation

hysz commented Jul 30, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

buildsize bot commented Jul 30, 2019 • edited Loading

coveralls commented Jul 30, 2019 • edited Loading

xianny left a comment • edited Loading

Choose a reason for hiding this comment

jalextowle 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

dorothy-zbornak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hysz Aug 1, 2019 • edited Loading

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

jalextowle left a comment

Choose a reason for hiding this comment

xianny 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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

hysz commented Jul 30, 2019 •

edited

Loading

buildsize bot commented Jul 30, 2019 •

edited

Loading

coveralls commented Jul 30, 2019 •

edited

Loading

xianny left a comment •

edited

Loading

dorothy-zbornak left a comment •

edited

Loading

hysz Aug 1, 2019 •

edited

Loading