Use Compressed Calldata in Contract Wrappers #1475

hysz · 2018-12-24T01:57:50Z

Description

The contract templates now use the optimized ABI encoder to compress all calldata.

We wanted to do this without changing the interface of the templates at all, so a few minor changes were made to the encoder:

Enforce lowercase addresses
(this also fixes a bug in current return value decoding, where nested addresses would not always be decoded)
The types uint8 and int8 are decoded as number instead of BigNumber
Convenience functions to get decoded data as an array
Convenience function to create an encoder from a signature.
Ex, signatures like 'string' and '(string,uint256)' and '(foo string,bar uint256)' can now all be used to instantiate an optimized encoder.

In a subsequent PR we'll add the ability for templates to decode calldata.

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 · 2018-12-24T04:10:57Z

Coverage decreased (-0.06%) to 85.381% when pulling b081785 on feature/monorepo/useNewAbiEncoder into 797d7c7 on development.

LogvinovLeon · 2019-01-07T11:28:07Z

Why did we remove all generated wrappers?

LogvinovLeon · 2019-01-07T12:14:40Z

contracts/protocol/test/utils/exchange_wrapper.ts

@@ -275,6 +278,19 @@ export class ExchangeWrapper {
        );
        return data;
    }
+    public abiDecodeFillOrder(data: string): AbiDecodedFillOrderData {
+        // Lookup fillOrder ABI in exchange abi
+        const fillOrderAbi = _.find(this._exchange.abi, (value: AbiDefinition) => {


Maybe:

Suggested change

const fillOrderAbi = _.find(this._exchange.abi, (value: AbiDefinition) => {

const fillOrderAbi = _.find(this._exchange.abi, { name: 'fillOrder' }) as MethodAbi;

We don't usually use that form but I feel like here it's more readable. It's not type safe, but the current solution is not either.

LogvinovLeon · 2019-01-07T12:16:22Z

packages/utils/src/abi_encoder/abstract_data_types/data_type.ts

@@ -51,8 +51,26 @@ export abstract class DataType {
        return value;
    }

+    public decodeAsArray(returndata: string, rules?: DecodingRules): any {


Suggested change

public decodeAsArray(returndata: string, rules?: DecodingRules): any {

public decodeAsArray(returndata: string, rules?: DecodingRules): any[] {

fabioberger · 2019-01-07T14:21:35Z

contracts/protocol/test/utils/types.ts

+
+export interface AbiDecodedFillOrderData {
+    order: SignedOrder;
+    akerAssetFillAmount: BigNumber;


Suggested change

akerAssetFillAmount: BigNumber;

makerAssetFillAmount: BigNumber;

fabioberger · 2019-01-07T14:26:12Z

packages/base-contract/src/index.ts

@@ -1,4 +1,4 @@
-import { abiUtils, BigNumber } from '@0x/utils';


Were we able to remove abiUtils and ethers.js entirely from our codebase?

abiUtils is still used to rename overloaded methods and assert correct encoding.

ethers is used by deployAsync.
There is a task to remove this from deployAsync after which we can remove ethers.js.

fabioberger · 2019-01-07T14:32:03Z

packages/utils/src/abi_encoder/abstract_data_types/types/set.ts

                );
            }
-            const memberBlock = this._members[this._memberIndexByName[key]].generateCalldataBlock(value, block);
+            const memberValue: any = (obj as { [key: string]: any })[memberName];


Suggested change

const memberValue: any = (obj as { [key: string]: any })[memberName];

const memberValue: any = (obj as ObjectMap<any>)[memberName];

fabioberger · 2019-01-07T14:33:57Z

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

@@ -44,7 +44,19 @@ export class MethodDataType extends AbstractSetDataType {
        return returnValues;
    }

-    public getSignature(): string {
+    public decodeReturnValuesAsArray(returndata: string, rules?: DecodingRules): any {


Suggested change

public decodeReturnValuesAsArray(returndata: string, rules?: DecodingRules): any {

public decodeReturnValuesAsArray(returnData: string, rules?: DecodingRules): any {

Always camel--case variable names

Shouldn't the return type here be any[]?

fabioberger · 2019-01-07T14:34:27Z

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

+        return this._destination.getSignature(false);
+    }
+
+    public getSignature(detailed?: boolean): string {


Suggested change

public getSignature(detailed?: boolean): string {

public getSignature(isDetailed?: boolean): string {

Stick to boolean naming conventions.

Let's investigate why was this not caught by linter rule

I suspect it's because it's a param name and not a variable. We should improve our linter rule.

fabioberger · 2019-01-07T14:35:17Z

packages/utils/src/abi_encoder/utils/signatureParser.ts

@@ -0,0 +1,101 @@
+import { DataItem } from 'ethereum-types';


Why is this file name not snake-case? It should be.

You mean snake case?

abandeali1 · 2019-01-08T00:26:36Z

contracts/protocol/test/utils/fill_order_combinatorial_utils.ts

            params.order,
            params.takerAssetFillAmount,
            params.signature,
        );
-        expect(libsAbiEncodedData).to.be.equal(expectedAbiEncodedData, 'ABIEncodedFillOrderData');
+        const paramsDecodeddByClient = this.exchangeWrapper.abiDecodeFillOrder(abiDataEncodedByContract);


paramsDecodedByClient

abandeali1 · 2019-01-08T00:33:56Z

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

@@ -44,7 +44,19 @@ export class MethodDataType extends AbstractSetDataType {
        return returnValues;
    }

-    public getSignature(): string {
+    public decodeReturnValuesAsArray(returndata: string, rules?: DecodingRules): any {


Shouldn't the return type here be any[]?

abandeali1 · 2019-01-08T00:39:24Z

packages/abi-gen-wrappers/src/generated-wrappers/asset_proxy_owner.ts

@@ -1,1877 +0,0 @@
-// tslint:disable:no-consecutive-blank-lines ordered-imports align trailing-comma whitespace class-name


Why are all of the generated wrappers deleted rather than replaced?

abandeali1 · 2019-01-10T23:41:27Z

contracts/protocol/test/utils/types.ts

+
+export interface AbiDecodedFillOrderData {
+    order: SignedOrder;
+    makerAssetFillAmount: BigNumber;


takerAssetFillAmount

abandeali1 · 2019-01-10T23:43:31Z

packages/abi-gen-wrappers/src/generated-wrappers/asset_proxy_owner.ts

-        ]);
-            const ethersFunction = self._lookupEthersInterface(functionSignature).functions.owners;
-            const encodedData = ethersFunction.encode([index_0
+            const encodedData = self._strictEncodeArguments('owners(uint256)', [index_0


Doesn't particularly matter since these are auto-generated, but I'm curious why the formatting is so strange (looks like it was this way before too).

That is a good question. I always figured it had something to do with Handlebars. @LogvinovLeon any ideas?

abandeali1 · 2019-01-10T23:53:42Z

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

+        return returnValuesAsArray;
+    }
+
+    public decodeReturnValuesAsArrayOrNull(returnData: string, rules?: DecodingRules): any {


This should also return any[].

abandeali1 · 2019-01-10T23:58:01Z

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

@@ -44,7 +44,19 @@ export class MethodDataType extends AbstractSetDataType {
        return returnValues;
    }

-    public getSignature(): string {
+    public decodeReturnValuesAsArray(returnData: string, rules?: DecodingRules): any[] {


For all of these functions with an any return type (or variant), should the signatures include a generic type? E.g:

public decodeReturnValuesAsArray<T>(returnData: string, rules?: DecodingRules): T[]

Thoughts @fabioberger @LogvinovLeon @hysz ?

I really like this idea.

abandeali1 · 2019-01-14T18:25:00Z

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

@@ -44,7 +44,20 @@ export class MethodDataType extends AbstractSetDataType {
        return returnValues;
    }

-    public getSignature(): string {
+    public strictDecodeReturnValue<T>(returndata: string, rules?: DecodingRules): T {


Is there a point in having a non-strict version of this still? I feel like we could probably add generic types to the regular decodeReturnValues.

Also this doesn't have to be in this PR if it's a lot of changes, but we should add generic types to the regular decode function as well.

I've created a task to use generics for all decoding. Shouldn't take long but I think it makes sense to have in a separate PR as well.

…thodAbi/DataItems are not readily available

…cts`

hysz requested a review from LogvinovLeon as a code owner December 24, 2018 01:57

probot-autolabeler bot added @0x/abi-gen abi-gen-templates @0x/abi-gen-wrappers @0x/base-contract contracts @0x/utils labels Dec 24, 2018

hysz force-pushed the feature/monorepo/useNewAbiEncoder branch from 8eac4c5 to 99f0fcf Compare December 24, 2018 03:40

hysz requested a review from abandeali1 December 24, 2018 05:56

hysz changed the title ~~[WIP] Use Compressed Calldata in Contract Wrappers~~ Use Compressed Calldata in Contract Wrappers Dec 24, 2018

LogvinovLeon approved these changes Jan 7, 2019

View reviewed changes

fabioberger suggested changes Jan 7, 2019

View reviewed changes

abandeali1 suggested changes Jan 8, 2019

View reviewed changes

hysz force-pushed the feature/monorepo/useNewAbiEncoder branch 4 times, most recently from 8f2c221 to b183be4 Compare January 10, 2019 22:28

abandeali1 approved these changes Jan 10, 2019

View reviewed changes

fabioberger approved these changes Jan 11, 2019

View reviewed changes

hysz force-pushed the feature/monorepo/useNewAbiEncoder branch from b2f5b12 to b6a298f Compare January 12, 2019 01:04

abandeali1 mentioned this pull request Jan 12, 2019

Remove optimization from matchOrdersAsync in Exchange wrapper #1514

Merged

4 tasks

abandeali1 reviewed Jan 14, 2019

View reviewed changes

hysz added 6 commits January 14, 2019 10:49

Progress

285fb3d

Ensure that addresses are lower case

b89f986

Working for almost every test

2f1454e

Works for almost all tests

e4551c8

All contract tests working with new abi encoder

f8684d6

Abi Encoder tests fixed + added a signature parser for cases where Me…

e9a8290

…thodAbi/DataItems are not readily available

hysz added 22 commits January 14, 2019 10:49

Generalized create for signature / dataItems. Cleaner.

7991de9

Finished porting new abi encoder to contracts

b06f823

Use string argument encoding with new encoder

d1fd442

Ran prettier

0b6c9c8

Decodes revert reason as array to appease Geth

6d832de

Ran prettier and linter

86caa4a

let -> const in callAsync

33f3405

Created an interface for abi decoded fillOrder data

f73c68e

Removed console log from handlebars

05adb38

Ran linter

f39c031

circle build failed. New commit to resubmit job.

6892f92

Renamed decode rule structsAsObjects to `shouldConvertStructsToObje…

0b87aec

…cts`

Renamed signatureParser.ts to signature_parser.ts

4f53335

Added back abi-gen-wrappers

1c9a49c

Updated dutch auction wrapper

1cbc03a

Use simpler _.find to locate fillOrderBai

892be49

Style cleanup for Compressed Calldata in Contract Wrappers PR

82a4455

Fix build after rebase

3a28eb1

Linter

80aa288

Ran prettier

cf2dee6

makerAssetFillAmount -> takerAssetFillAmount

1907ecc

strict decoding of return values using generics

b081785

hysz force-pushed the feature/monorepo/useNewAbiEncoder branch from b6a298f to b081785 Compare January 14, 2019 18:50

hysz merged commit 1c25d8e into development Jan 14, 2019

hysz deleted the feature/monorepo/useNewAbiEncoder branch March 16, 2019 04:35

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

Use Compressed Calldata in Contract Wrappers #1475

Use Compressed Calldata in Contract Wrappers #1475

hysz commented Dec 24, 2018 •

edited

Loading

coveralls commented Dec 24, 2018 •

edited

Loading

LogvinovLeon commented Jan 7, 2019

LogvinovLeon Jan 7, 2019

LogvinovLeon Jan 7, 2019

fabioberger Jan 7, 2019

fabioberger Jan 7, 2019

hysz Jan 9, 2019

fabioberger Jan 7, 2019

fabioberger Jan 7, 2019

abandeali1 Jan 8, 2019

fabioberger Jan 7, 2019

LogvinovLeon Jan 7, 2019

fabioberger Jan 8, 2019

fabioberger Jan 7, 2019 •

edited

Loading

LogvinovLeon Jan 8, 2019

abandeali1 Jan 8, 2019

abandeali1 Jan 8, 2019

abandeali1 Jan 8, 2019

abandeali1 Jan 10, 2019

hysz Jan 11, 2019

abandeali1 Jan 10, 2019

hysz Jan 11, 2019

abandeali1 Jan 10, 2019

abandeali1 Jan 10, 2019

hysz Jan 11, 2019

abandeali1 Jan 14, 2019

hysz Jan 14, 2019

	const fillOrderAbi = _.find(this._exchange.abi, (value: AbiDefinition) => {
	const fillOrderAbi = _.find(this._exchange.abi, { name: 'fillOrder' }) as MethodAbi;

	public decodeAsArray(returndata: string, rules?: DecodingRules): any {
	public decodeAsArray(returndata: string, rules?: DecodingRules): any[] {

	akerAssetFillAmount: BigNumber;
	makerAssetFillAmount: BigNumber;

		@@ -1,4 +1,4 @@
		import { abiUtils, BigNumber } from '@0x/utils';

	const memberValue: any = (obj as { [key: string]: any })[memberName];
	const memberValue: any = (obj as ObjectMap<any>)[memberName];

	public decodeReturnValuesAsArray(returndata: string, rules?: DecodingRules): any {
	public decodeReturnValuesAsArray(returnData: string, rules?: DecodingRules): any {

	public getSignature(detailed?: boolean): string {
	public getSignature(isDetailed?: boolean): string {

		@@ -1,1877 +0,0 @@
		// tslint:disable:no-consecutive-blank-lines ordered-imports align trailing-comma whitespace class-name

Use Compressed Calldata in Contract Wrappers #1475

Use Compressed Calldata in Contract Wrappers #1475

Conversation

hysz commented Dec 24, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Dec 24, 2018 • edited Loading

LogvinovLeon commented Jan 7, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabioberger Jan 7, 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

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

hysz commented Dec 24, 2018 •

edited

Loading

coveralls commented Dec 24, 2018 •

edited

Loading

fabioberger Jan 7, 2019 •

edited

Loading