ERC1155 Asset Proxy #1661

hysz · 2019-02-28T22:03:44Z

Description

Asset Proxy for transferring ERC1155 assets. ERC1155 allows a set of fungible / non-fungible assets to exist under the same contract. The proxy uses ERC1155's safeBatchTransferFrom to transfer a set of ERC1155 assets between accounts. See #1657 for a minimal ERC1155 implementation.

Note - integration tests with the Exchange and MultiAsset Proxy in a separate PR.

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-02-28T22:46:37Z

Coverage decreased (-0.02%) to 85.138% when pulling 22bc1fb on feature/contracts/erc1155Proxy-4 into d3f45d2 on development.

merklejerk · 2019-03-12T18:54:42Z

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

+                // 4 (function selector)
+                // + assetDataOffset
+                // + 32 (length of assetData)
+                calldatacopy(32, add(36, assetDataOffset), assetDataLength)


You're probably right, but why not calldatacopy(68, add(36, assetDataOffset), sub(assetDataLength, 36))?

Good question - this line copies the entire assetData into memory at offset 32. Memory allocation looks like this:

32 : assetProxyId 36 : address of ERC1155 contract 68 : offset to tokenIds ...

This is later transformed into:

00 : batchTransferFrom selector 04 : from address 36 : to address 68 : offset to tokenIds ...

If we replaced this line with calldatacopy(68, add(36, assetDataOffset), sub(assetDataLength, 36)) then we would have the following allocation:

68 : assetProxyId 72 : address of ERC1155 contract 104 : offset to tokenIds ...

And it would truncate 36 bytes at the tail end of the asset data.

Note that assetData is a byte array so it is abi encoded as:

0 : length 32 : contents

Thanks! Super appreciate the walkthrough.

abandeali1 · 2019-03-12T22:19:12Z

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

+        // | Header   | 0           | 4       | assetProxyId                        |
+        // | Params   |             | 4 * 32  | function parameters:                |
+        // |          | 4           |         |   1. address of ERC1155 contract    |
+        // |          | 36          |         |   2. offset to tokenIds (*)         |


I feel like these param names should be ids, values, and data to be consistent with the naming of the actual ERC1155 transfer params.

Meh, actually we aren't really consistent in the other proxies either.

No I agree, we should be consistent with the naming of the ERC1155 transfer params.

abandeali1 · 2019-03-12T22:23:21Z

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

+                // 4 (function selector)
+                // + assetDataOffset
+                // + 32 (length of assetData)
+                calldatacopy(32, add(36, assetDataOffset), assetDataLength)


I personally like annotating calldatacopy like we do in the MAP here: https://github.com/0xProject/0x-monorepo/blob/development/contracts/asset-proxy/contracts/src/MultiAssetProxy.sol#L161

I like this +1.

abandeali1 · 2019-03-12T22:32:09Z

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

+                    let scaledTokenValue := mul(tokenValue, scaleAmount)
+
+                    // Check if scaled value is zero
+                    if iszero(scaledTokenValue) {


We don't actually check if the amount is 0 in any of the other proxies. What is the reason for doing so here, and can this ever be potentially limiting?

Mm yeah, here's my reasoning - curious to get your thoughts - in the Exchange we check that the maker/taker amounts are non-zero. This amount is then directly used by the ERC20 and ERC721 proxies, so their check would be redundant. In the case of 1155, any one of the encoded values could also be zero, so I added this check for consistency.

abandeali1 · 2019-03-12T22:39:21Z

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

+
+                    // Check for multiplication overflow
+                    let expectedTokenValue := div(scaledTokenValue, scaleAmount)
+                    if iszero(eq(expectedTokenValue, tokenValue)) {


Just a note that if we do remove the last check, we'll still need to check if scaleAmount is 0 before doing this division.

A check has been added above, right after we load amount.

abandeali1 · 2019-03-12T22:47:27Z

packages/types/CHANGELOG.json

+        "version": "2.2.3",
+        "changes": [
+            {
+                "note": "Added `ERC1155AssetData`, `ERC1155AssetDataNoProxyId`, and `ERC1155AssetDataAbi`",


This change can be merged with the previous entry (not yet published).

abandeali1 · 2019-03-12T22:50:10Z

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

+
+*/
+
+pragma solidity 0.4.24;


We should test gas differences with a newer version of Solidity too.

Yeah it's only ~100 gas cheaper if we use solidity 0.5.5.

Additionally, I believe all the proxies have to use the same version since they're in the same package, ya? If this is the case and the savings become more substantial it could be worthwhile to split proxies into standalone packages.

abandeali1 · 2019-03-12T22:54:36Z

packages/order-utils/src/asset_data_utils.ts

@@ -1,5 +1,8 @@
 import {
    AssetProxyId,
+    ERC1155AssetData,


You'll need to reexport these in the index.ts of this package and 0x.js (see doc-gen failing tests).

…to be consistent with the ERC1155 reference implementation.

… and there is an overflow.

…is PR

hysz requested review from fabioberger and LogvinovLeon as code owners February 28, 2019 22:03

probot-autolabeler bot added contracts @0x/order-utils @0x/types labels Feb 28, 2019

hysz mentioned this pull request Feb 28, 2019

[WIP] ERC1155 Asset Proxy #1632

Closed

4 tasks

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from 24e2100 to 302ed95 Compare March 5, 2019 16:09

hysz requested a review from abandeali1 March 5, 2019 18:58

hysz force-pushed the feature/contracts/erc1155MinimalImplementation branch from 82cafd6 to 6fe5fed Compare March 5, 2019 19:37

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch 2 times, most recently from 590dbf1 to c0eef21 Compare March 6, 2019 00:40

hysz force-pushed the feature/contracts/erc1155MinimalImplementation branch 2 times, most recently from 785f1c5 to 6fa806a Compare March 7, 2019 21:08

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from c0eef21 to 49ccb98 Compare March 7, 2019 22:40

hysz changed the title ~~[WIP] ERC1155 Asset Proxy~~ ERC1155 Asset Proxy Mar 11, 2019

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from 78ec115 to 12eeed4 Compare March 12, 2019 01:24

hysz force-pushed the feature/contracts/erc1155MinimalImplementation branch from 9b5199e to 9822792 Compare March 12, 2019 01:25

merklejerk reviewed Mar 12, 2019

View reviewed changes

hysz changed the base branch from feature/contracts/erc1155MinimalImplementation to development March 12, 2019 20:08

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from 12eeed4 to 441b379 Compare March 12, 2019 21:38

merklejerk approved these changes Mar 12, 2019

View reviewed changes

abandeali1 suggested changes Mar 12, 2019

View reviewed changes

abandeali1 reviewed Mar 12, 2019

View reviewed changes

probot-autolabeler bot added the 0x.js label Mar 13, 2019

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from 56df883 to 334e935 Compare March 13, 2019 01:52

probot-autolabeler bot added the @0x/contract-wrappers label Mar 13, 2019

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch 2 times, most recently from 9b2197f to 32b0b0b Compare March 14, 2019 21:21

hysz mentioned this pull request Mar 14, 2019

Exchange & Proxy Integration Tests - ERC1155 #1673

Merged

4 tasks

hysz added 27 commits March 15, 2019 15:05

Added remaining erc1155 revert reasons to types package

d5d9df3

Split ERC1155 Asset Proxy from ERC1155 POC implementation - squashed

15d9e2d

Remaining tests for erc1155 proxy

4473851

test with several tokens erc1155 proxy

2882c4b

cleaning up erc1155 proxy tests

008eb8d

updated erc1155 proxy tests and moved to their own test file

4dfb610

removed erc1155 proxy tests from general proxy test file

054c0e9

Renamed erc1155token to erc1155Contract in proxy tests

888c173

Ran prettier / linter

55c4fc9

ERC1155 Asset Data tests + types

5704afc

moved transferFrom into ERC1155 Proxy Wrapper

a86ba7a

Moved transferFrom with/without logs into ERC1155 Proxy Wrapper

38e4871

Ran prettier & linter

05ef250

Added method descriptions to erc1155 proxy wrapper

ae51cfe

Updated changelogs and documentation for erc1155 proxy

1e56481

Ran prettier

8916d0d

Rebased against development

9ec3807

Renamed tokenIds -> ids, tokenValues -> values, callbackData -> data …

2a6ed0c

…to be consistent with the ERC1155 reference implementation.

added exports for 1155 proxy

de971e6

yarn.lock for erc1155 proxy

df5786d

updated comment for calldatacopy

f783c9b

Do not revert if value or amount are zero. Only if amount is non-zero…

9207d1c

… and there is an overflow.

Added calldatacopy comment

b6571d0

Adjusted changelog version since nothing has been published during th…

eb9bf7c

…is PR

added test for amount=0

6a7530d

renamed perUnitValue to valueMultiplier

63ba764

additional zero outcome tests

22bc1fb

hysz force-pushed the feature/contracts/erc1155Proxy-4 branch from a33da79 to 22bc1fb Compare March 15, 2019 22:06

hysz merged commit bab368b into development Mar 15, 2019

hysz deleted the feature/contracts/erc1155Proxy-4 branch March 16, 2019 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC1155 Asset Proxy #1661

ERC1155 Asset Proxy #1661

hysz commented Feb 28, 2019 •

edited

Loading

coveralls commented Feb 28, 2019 •

edited

Loading

merklejerk Mar 12, 2019

hysz Mar 12, 2019

merklejerk Mar 12, 2019

abandeali1 Mar 12, 2019

abandeali1 Mar 12, 2019

hysz Mar 12, 2019

abandeali1 Mar 12, 2019

hysz Mar 13, 2019

abandeali1 Mar 12, 2019

hysz Mar 12, 2019

abandeali1 Mar 12, 2019

hysz Mar 14, 2019

abandeali1 Mar 12, 2019

abandeali1 Mar 12, 2019

hysz Mar 13, 2019

abandeali1 Mar 12, 2019

ERC1155 Asset Proxy #1661

ERC1155 Asset Proxy #1661

Conversation

hysz commented Feb 28, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Feb 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

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 Feb 28, 2019 •

edited

Loading

coveralls commented Feb 28, 2019 •

edited

Loading