Remove assetDataUtils everywhere #2373

xianny · 2019-11-27T23:55:35Z

Description

I started removing uses of assetDataUtils a while ago, but left the task incomplete as it was easier to wait for things to be removed from the monorepo rather than refactor them.

This PR replaces the last few uses of assetDataUtils with DevUtilsContract and removes assetDataUtils completely.

After making a few needed fixes, this PR has grown somewhat unwieldy:

moves hex_utils.ts from @0x/contracts-test-utils to @0x/utils
removes assetDataUtils from @0x/order-utils
removes/replaces uses of assetDataUtils everywhere in monorepo
re-implements decodeAssetDataOrThrow in @0x/order-utils, this time with ERC20Bridge support
adds ERC20BridgeAssetData type and interface to @0x/types

Apologies for combining the hex_utils rename in this PR. Trying to get a fix in sooner rather than later as the currently published assetDataUtils does not support ERC20Bridge.

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.

xianny · 2019-11-27T23:56:14Z

packages/instant/src/index.umd.ts

@@ -108,15 +108,15 @@ export interface ZeroExInstantConfig extends ZeroExInstantOverlayProps {
    shouldDisablePushToHistory?: boolean;
 }

-export const render = (config: ZeroExInstantConfig, selector: string = DEFAULT_ZERO_EX_CONTAINER_SELECTOR) => {


I couldn't find where this was being called, so didn't rename it to renderAsync. Worried this breaks something else. Where is this used? @fragosti @steveklebanoff

heh all these functions are exported so developers can use them when importing the UMD bundle. So while they're not used in instant, they are exposed for convenience.

fragosti · 2019-11-27T23:59:33Z

packages/instant/src/index.umd.ts

@@ -108,15 +108,15 @@ export interface ZeroExInstantConfig extends ZeroExInstantOverlayProps {
    shouldDisablePushToHistory?: boolean;
 }

-export const render = (config: ZeroExInstantConfig, selector: string = DEFAULT_ZERO_EX_CONTAINER_SELECTOR) => {


heh all these functions are exported so developers can use them when importing the UMD bundle. So while they're not used in instant, they are exposed for convenience.

fragosti · 2019-11-28T00:00:01Z

packages/instant/src/util/dev_utils_contract.ts

@@ -0,0 +1,5 @@
+import { DevUtilsContract } from '@0x/contract-wrappers';
+import { NULL_ADDRESS } from '@0x/utils';
+


Maybe a little comment about what is going on.

fragosti · 2019-11-28T00:07:42Z

packages/instant/src/index.umd.ts

-export const assetDataForERC20TokenAddress = (tokenAddress: string): string => {
+export const assetDataForERC20TokenAddressAsync = async (tokenAddress: string): Promise<string> => {
    assert.isETHAddressHex('tokenAddress', tokenAddress);
-    return assetDataUtils.encodeERC20AssetData(tokenAddress);
+    return devUtilsContract.encodeERC20AssetData(tokenAddress).callAsync();
 };

-export const assetDataForERC721TokenAddress = (tokenAddress: string, tokenId: string | number): string => {
+export const assetDataForERC721TokenAddressAsync = async (
+    tokenAddress: string,
+    tokenId: string | number,
+): Promise<string> => {
    assert.isETHAddressHex('tokenAddress', tokenAddress);
-    return assetDataUtils.encodeERC721AssetData(tokenAddress, new BigNumber(tokenId));
+    return devUtilsContract.encodeERC721AssetData(tokenAddress, new BigNumber(tokenId)).callAsync();


This is a significant breaking change. Obviously I'm lacking context on this decision but it seems odd that you can no longer get assetDatas in a synchronous way.

That is a good point @fragosti. @xianny we could change the method signature in the wrapper to be synchronous if the method is pure, no?

Not easily, we currently use putContractCode and runCall from ethereumjs-vm and they are async.

@jalextowle showed me a synchronous work-around using IAssetDataContract though:

const assetDataEncoder = new IAssetDataContract(NULL_ADDRESS, fakeProvider); const encoded = assetDataEncoder.ERC20Token(tokenAddress).getABIEncodedTransactionData()

…tant to decode asset data synchronously

buildsize · 2019-11-28T16:53:54Z

File name	Previous Size	New Size	Change
init.py	60.25 KB	60.25 KB	0 bytes (0%)
abi_gen_dummy.ts	77.76 KB	[deleted]
lib_dummy.ts	7.13 KB	[deleted]
test_lib_dummy.ts	10.38 KB	[deleted]
environment.pickle	1.61 MB	1.61 MB	0 bytes (0%)
index.doctree	187.37 KB	187.37 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.84 KB	2.84 KB	0 bytes (0%)
searchindex.js	6.31 KB	6.31 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.89 KB	11.89 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	273.79 KB	273.79 KB	0 bytes (0%)
jquery.js	86.08 KB	86.08 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.8 KB	16.8 KB	0 bytes (0%)
contract_artifacts.html	8.24 KB	8.24 KB	0 bytes (0%)
json_schemas.html	12.56 KB	12.56 KB	0 bytes (0%)
order_utils.html	47.16 KB	47.16 KB	0 bytes (0%)
erc20_token.html	93.56 KB	93.56 KB	0 bytes (0%)
exchange.html	718.38 KB	718.38 KB	0 bytes (0%)
tx_params.html	9.41 KB	9.41 KB	0 bytes (0%)
local_message_signer.html	15.08 KB	15.08 KB	0 bytes (0%)
asset_data_utils.html	22.66 KB	22.66 KB	0 bytes (0%)
default_api.html	113.17 KB	113.17 KB	0 bytes (0%)
asset_proxy_owner.html	337.39 KB	337.39 KB	0 bytes (0%)
coordinator.html	128.72 KB	128.72 KB	0 bytes (0%)
coordinator_registry.html	39.59 KB	39.59 KB	0 bytes (0%)
dutch_auction.html	66.13 KB	66.13 KB	0 bytes (0%)
erc20_proxy.html	109.07 KB	109.07 KB	0 bytes (0%)
erc721_proxy.html	109.19 KB	109.19 KB	0 bytes (0%)
erc721_token.html	150.2 KB	150.2 KB	0 bytes (0%)
forwarder.html	124.01 KB	124.01 KB	0 bytes (0%)
i_asset_proxy.html	40.18 KB	40.18 KB	0 bytes (0%)
i_validator.html	27.06 KB	27.06 KB	0 bytes (0%)
i_wallet.html	24.9 KB	24.9 KB	0 bytes (0%)
multi_asset_proxy.html	144.04 KB	144.04 KB	0 bytes (0%)
order_validator.html	107.69 KB	107.69 KB	0 bytes (0%)
weth9.html	132.09 KB	132.09 KB	0 bytes (0%)
zrx_token.html	107.61 KB	107.61 KB	0 bytes (0%)
dev_utils.html	526.89 KB	526.89 KB	0 bytes (0%)
types.html	8.54 KB	8.54 KB	0 bytes (0%)
erc1155_mintable.html	276.51 KB	276.51 KB	0 bytes (0%)
erc1155_proxy.html	130.38 KB	130.38 KB	0 bytes (0%)
static_call_proxy.html	34.04 KB	34.04 KB	0 bytes (0%)

coveralls · 2019-11-28T17:50:37Z

Coverage remained the same at 77.804% when pulling 37f5cab on refactor/remove-asset-data-utils into b86d190 on development.

…support

fragosti · 2019-12-03T23:28:47Z

packages/instant/src/index.umd.ts

@@ -158,12 +158,12 @@ export const ERC20_PROXY_ID = AssetProxyId.ERC20;

 export const assetDataForERC20TokenAddress = (tokenAddress: string): string => {
    assert.isETHAddressHex('tokenAddress', tokenAddress);
-    return assetDataUtils.encodeERC20AssetData(tokenAddress);
+    return assetDataEncoder.ERC20Token(tokenAddress).getABIEncodedTransactionData();


😍 so much better!

fragosti · 2019-12-03T23:31:05Z

packages/instant/src/util/asset_data_encoder.ts

+export function decodeAssetProxyId(assetData: string): string {
+    return hexSlice(assetData, 0, 4);
+}


Shouldn't something like this be in order-utils

Maybe but right now nothing else uses it and instant doesn't use order-utils as a dependency yet...

fragosti · 2019-12-03T23:43:06Z

Still feel like I'm lacking context though. Why are we removing assetDataUtils? What is the standard way of creating asset datas now?

xianny · 2019-12-04T18:28:38Z

@fragosti the motivation is to stop maintaining separate typescript libs for encoding/decoding asset data, and only have the code implemented and tested once in Solidity. Cutting unnecessary overhead is gonna save us time in the future; e.g. in this instance we already neglected to add ERC20Bridge support, but it's already implemented in Solidity.

@fabio @dekz btw, I think we should encourage devs to use IAssetDataContract instead of DevUtilsContract, especially if they are only going to be using DUC for asset data encoding/decoding. It's much faster because it doesn't have to instantiate the EVM and load the ABI.

fabioberger · 2019-12-04T18:57:27Z

contracts/coordinator/test/mixins.ts

    randomAddress,
    TransactionFactory,
    transactionHashUtils,
 } from '@0x/contracts-test-utils';
 import { LibBytesRevertErrors } from '@0x/contracts-utils';
 import { SignatureType, SignedOrder } from '@0x/types';
-import { BigNumber, CoordinatorRevertErrors } from '@0x/utils';
+import { BigNumber, CoordinatorRevertErrors, hexConcat, hexSlice } from '@0x/utils';


Can we export a hexUtils from @0x/utils instead of several hex prefixed methods?

* remove assetDataUtils everywhere * export IAssetDataContract from @0x/contract-wrappers to allow @0x/instant to decode asset data synchronously * export generic function `decodeAssetDataOrThrow` and add ERC20Bridge support * export `hexUtils` from order-utils instead of contracts-test-utils

xianny requested review from abandeali1, BMillman19, dave4506, fabioberger, fragosti and hysz as code owners November 27, 2019 23:55

xianny commented Nov 27, 2019

View reviewed changes

xianny force-pushed the refactor/remove-asset-data-utils branch from 79fa0c0 to 2a37c9d Compare November 28, 2019 00:03

fragosti suggested changes Nov 28, 2019

View reviewed changes

xianny added 2 commits November 28, 2019 08:20

remove assetDataUtils everywhere

093be46

export IAssetDataContract from @0x/contract-wrappers to allow @0x/ins…

48bf1e3

…tant to decode asset data synchronously

xianny force-pushed the refactor/remove-asset-data-utils branch from 2a37c9d to 48bf1e3 Compare November 28, 2019 16:44

xianny requested a review from albrow as a code owner November 28, 2019 16:44

fix CI

f7e5aac

xianny force-pushed the refactor/remove-asset-data-utils branch from 00b6b38 to f7e5aac Compare November 28, 2019 19:43

dekz mentioned this pull request Dec 1, 2019

Update taker related docs to v3 0xProject/website#65

Merged

xianny force-pushed the refactor/remove-asset-data-utils branch 2 times, most recently from 4e464d3 to b27434c Compare December 3, 2019 21:30

Merge branch 'development' into refactor/remove-asset-data-utils

8b09a63

xianny force-pushed the refactor/remove-asset-data-utils branch 3 times, most recently from 10a99a7 to 3b8bee4 Compare December 3, 2019 22:49

xianny added 2 commits December 3, 2019 15:13

export generic function decodeAssetDataOrThrow and add ERC20Bridge …

2aeeb03

…support

update changelogs

81c6d98

xianny force-pushed the refactor/remove-asset-data-utils branch from 3b8bee4 to 81c6d98 Compare December 3, 2019 23:13

xianny requested review from fragosti and removed request for albrow and BMillman19 December 3, 2019 23:23

xianny requested review from alexkroeger and dekz and removed request for hysz, dave4506 and abandeali1 December 3, 2019 23:30

fragosti reviewed Dec 3, 2019

View reviewed changes

xianny closed this Dec 4, 2019

xianny reopened this Dec 4, 2019

fabioberger approved these changes Dec 4, 2019

View reviewed changes

xianny added 2 commits December 4, 2019 11:31

export a single hexUtils instead of separate functions

d995631

Merge branch 'development' into refactor/remove-asset-data-utils

37f5cab

xianny merged commit fcbcbac into development Dec 4, 2019

xianny deleted the refactor/remove-asset-data-utils branch December 4, 2019 21:08

xianny mentioned this pull request Dec 5, 2019

Add syntactic sugar for assetDataUtils #2388

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove assetDataUtils everywhere #2373

Remove assetDataUtils everywhere #2373

xianny commented Nov 27, 2019 •

edited

Loading

xianny Nov 27, 2019

fragosti Nov 27, 2019

fragosti Nov 27, 2019

fragosti Nov 28, 2019

fragosti Nov 28, 2019

fabioberger Dec 2, 2019

xianny Dec 3, 2019

buildsize bot commented Nov 28, 2019 •

edited

Loading

coveralls commented Nov 28, 2019 •

edited

Loading

fragosti Dec 3, 2019

fragosti Dec 3, 2019

xianny Dec 4, 2019

fragosti commented Dec 3, 2019

xianny commented Dec 4, 2019

fabioberger Dec 4, 2019

xianny Dec 4, 2019

		@@ -0,0 +1,5 @@
		import { DevUtilsContract } from '@0x/contract-wrappers';
		import { NULL_ADDRESS } from '@0x/utils';

Remove assetDataUtils everywhere #2373

Remove assetDataUtils everywhere #2373

Conversation

xianny commented Nov 27, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

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

buildsize bot commented Nov 28, 2019 • edited Loading

coveralls commented Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fragosti commented Dec 3, 2019

xianny commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xianny commented Nov 27, 2019 •

edited

Loading

buildsize bot commented Nov 28, 2019 •

edited

Loading

coveralls commented Nov 28, 2019 •

edited

Loading