Initial ERC1155 implementation with some tests #1803

cag · 2019-06-18T22:41:39Z

Implements ERC-1155.

nventuro · 2019-06-19T00:37:28Z

Hello @cag, thank you very much for this! Reviewing this PR will probably take a while, since there's quite a bit of code, and we need to figure out a reasonable API to provide.

I noticed the implementation by enjin is licensed under Apache v2, whereas we use MIT. We may want to check with them that it is fine to re-license their work to distribute under OpenZeppelin, though I'm not sure to what extent that applies to something like this, where the specification is quite clear-cut regarding what needs to be done.

stale · 2019-07-04T16:25:28Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

frangio · 2019-07-04T21:29:02Z

Hey there. It's taking a while for me to get some free time to look at this, but I'll come back to it.

stale · 2019-07-20T13:23:41Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

cag · 2019-07-23T21:54:52Z

Okay, the tests should be complete for the basic setup now. Cheers!

The18thWarrior · 2019-07-29T23:12:22Z

Any update on this PR? I would like to leverage OpenZepplin in my build, but can't stay on hold indefinitely.

frangio · 2019-08-07T18:41:19Z

We're in the process of reviewing this. 🙂 Expect a review from @andresbach from the OpenZeppelin team in a few days!

The18thWarrior · 2019-08-07T18:46:45Z

Awesome! Thanks!

…

On Wed, Aug 7, 2019, 11:41 AM Francisco Giordano ***@***.***> wrote: We're in the process of reviewing this. 🙂 Expect a review from @andresbach <https://github.com/andresbach> from the OpenZeppelin team in a few days! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1803?email_source=notifications&email_token=AAUDVIMLQA6MFRRH2OKSSKLQDMJOZA5CNFSM4HZENLO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZK4SI#issuecomment-519220809>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUDVIL6DWN2Z75EY2QUTCTQDMJOZANCNFSM4HZENLOQ> .

stale · 2019-08-22T18:58:30Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2019-09-06T19:50:53Z

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

andresbach

Hey @cag! So sorry for the delay. I left a few comments on things that @frangio and I agreed should be changed, and also a couple questions of other things we might want to add.

contracts/token/ERC1155/ERC1155.sol

andresbach · 2019-09-17T20:11:48Z

contracts/token/ERC1155/ERC1155.sol

+ * See https://eips.ethereum.org/EIPS/eip-1155
+ * Originally based on code by Enjin: https://github.com/enjin/erc-1155
+ */
+contract ERC1155 is ERC165, IERC1155


Some of the contracts are missing, such as the one for the metadata and the one for the URI functions. Are those going to be implemented in another stage?

I did not seek to implement those contracts, since we don't know how these features really should work because we don't use them, and didn't plan on using them. These should be added by somebody with more knowledge/use for those features.

contracts/token/ERC1155/ERC1155.sol

stale · 2019-10-03T18:06:11Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

abcoathup · 2019-10-04T00:42:28Z

I’ve reached out to the authors of ERC1155 as would be great to get their assistance.

thecryptofruit · 2019-10-08T08:46:09Z

SafeMath.sub with custom error message is not yet released AFAIK, not in 2.3 at least.

stale · 2019-11-02T23:43:52Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

frangio · 2019-11-03T01:19:42Z

Hi everyone. We haven't had the time to do a second review of the code in this PR, which is something we have to do for such a big feature before releasing it.

However, we don't want to delay merging this PR any longer so we're merging it into a feature branch. If you're interested in getting this feature released as soon as possible, please help us by reviewing the code and posting your results in an issue or in the forum. We will also be doing our own review and may create an issue with a list of things that need work, where help will also be appreciated.

Sorry for taking so long with this and thank you for the great work @cag!

coinfork · 2019-11-12T02:41:01Z

Hello @cag, thank you very much for this! Reviewing this PR will probably take a while, since there's quite a bit of code, and we need to figure out a reasonable API to provide.

I noticed the implementation by enjin is licensed under Apache v2, whereas we use MIT. We may want to check with them that it is fine to re-license their work to distribute under OpenZeppelin, though I'm not sure to what extent that applies to something like this, where the specification is quite clear-cut regarding what needs to be done.

We're completely fine with re-licensing your implementation with the MIT license.

KaiRo-at · 2019-11-19T15:48:06Z

I'm trying to use the contracts for a project right now, and I just realized that - compared to other OpenZeppelin token implementations - there is no internal _transferFrom() function (or similar) in those, which makes it harder for us to override something so we can intercept a transfer and e.g. maintain a "totalSupply"-like counter when something is transferred to/from 0x0, or put some restrictions on transfers via custom logic or similar side-effects - and the external public unctions do not allow overrides that call the inherited code via super.safeTransferFrom(), for example.
This is one case where an implementation in OpenZeppelin probably requires some additional work than just a (reference) implementation, so people can inherit from this code and flexibly extend it.

frangio · 2019-11-19T15:58:43Z

@KaiRo-at Agree! Would you like to open a PR making the functions public?

KaiRo-at · 2019-11-19T16:56:23Z

I think it would be even better to abstract the actual transfer into an internal function like e.g. ERC20 and ERC721 OpenZeppelin implementations do. They also have the beauty of having a single internal function for all variants of transfers, which is harder here as single and batch emit different events, so I'm not 100% sure yet how to do this in the best way.

frangio · 2019-11-19T17:02:38Z

@KaiRo-at Let's continue this discussion in #2003.

KaiRo-at · 2019-12-13T13:33:22Z

Another thing I just noticed is that this PR contains a URI() event in the interface, but nothing using it or anything else related to URIs, so either the event should be removed or the implementation using it some way. Though actually, this may be a bug in the ERC itself, as that even should belong to the "interface ERC1155Metadata_URI" definition in there, I guess... See ethereum/EIPs#1155 (comment)

frangio · 2019-12-13T13:43:49Z

Thank you @KaiRo-at. Please move all related discussion to #2014. 🙂

* Initial ERC1155 implementation with some tests * Remove mocked isERC1155TokenReceiver * Revert reason edit nit * Remove parameters associated with isERC1155TokenReceiver call * Add tests for approvals and single transfers * Add tests for transferring to contracts * Add tests for batch transfers * Make expectEvent.inTransaction tests async * Renamed "owner" to "account" and "holder" * Document unspecified balanceOfBatch reversion on zero behavior * Ensure accounts can't set their own operator status * Specify descriptive messages for underflow errors * Bring SafeMath.add calls in line with OZ style * Explicitly prevent _burn on the zero account * Implement batch minting/burning * Refactored operator approval check into isApprovedForAll calls * Renamed ERC1155TokenReceiver to ERC1155Receiver * Added ERC1155Holder * Fix lint issues

* Initial ERC1155 implementation with some tests (#1803) * Initial ERC1155 implementation with some tests * Remove mocked isERC1155TokenReceiver * Revert reason edit nit * Remove parameters associated with isERC1155TokenReceiver call * Add tests for approvals and single transfers * Add tests for transferring to contracts * Add tests for batch transfers * Make expectEvent.inTransaction tests async * Renamed "owner" to "account" and "holder" * Document unspecified balanceOfBatch reversion on zero behavior * Ensure accounts can't set their own operator status * Specify descriptive messages for underflow errors * Bring SafeMath.add calls in line with OZ style * Explicitly prevent _burn on the zero account * Implement batch minting/burning * Refactored operator approval check into isApprovedForAll calls * Renamed ERC1155TokenReceiver to ERC1155Receiver * Added ERC1155Holder * Fix lint issues * Migrate tests to @openzeppelin/test-environment * Port ERC 1155 branch to Solidity 0.6 (and current master) (#2130) * port ERC1155 to Solidity 0.6 * make ERC1155 constructor more similar to ERC721 one * also migrate mock contracts to Solidity 0.6 * mark all non-view functions as virtual Co-authored-by: Alan Lu <[email protected]> Co-authored-by: Nicolás Venturo <[email protected]> Co-authored-by: Robert Kaiser <[email protected]>

…2029) * Initial ERC1155 implementation with some tests (#1803) * Initial ERC1155 implementation with some tests * Remove mocked isERC1155TokenReceiver * Revert reason edit nit * Remove parameters associated with isERC1155TokenReceiver call * Add tests for approvals and single transfers * Add tests for transferring to contracts * Add tests for batch transfers * Make expectEvent.inTransaction tests async * Renamed "owner" to "account" and "holder" * Document unspecified balanceOfBatch reversion on zero behavior * Ensure accounts can't set their own operator status * Specify descriptive messages for underflow errors * Bring SafeMath.add calls in line with OZ style * Explicitly prevent _burn on the zero account * Implement batch minting/burning * Refactored operator approval check into isApprovedForAll calls * Renamed ERC1155TokenReceiver to ERC1155Receiver * Added ERC1155Holder * Fix lint issues * Migrate tests to @openzeppelin/test-environment * port ERC1155 to Solidity 0.6 * make ERC1155 constructor more similar to ERC721 one * also migrate mock contracts to Solidity 0.6 * mark all non-view functions as virtual * add simple catch-all implementation for the metadata URI interface * include an internal function to set the URI so users can implement functionality to switch URIs * add tests for ERC1155 metadata URI * fix nits, mostly pointed out by linter * convert ERC1155 metadata URI work to Solidity 0.6 * mark all non-view functions as virtual * Port ERC 1155 branch to Solidity 0.6 (and current master) (#2130) * port ERC1155 to Solidity 0.6 * make ERC1155 constructor more similar to ERC721 one * also migrate mock contracts to Solidity 0.6 * mark all non-view functions as virtual * Update contracts/token/ERC1155/IERC1155MetadataURI.sol Starting on Solidity v0.6.2, interfaces can now inherit. \o/ Co-authored-by: Nicolás Venturo <[email protected]> * Fix compile errors * Remove URI event * Merge MetadataCatchAll into ERC1155 * Improve documentation. * Simplify tests * Move tests into ERC1155 tests * Update documentation * Bump minimum compiler version for inteface inheritance * Fix holder tests * Improve setUri docs * Fix docs generation Co-authored-by: Alan Lu <[email protected]> Co-authored-by: Nicolás Venturo <[email protected]> Co-authored-by: Francisco Giordano <[email protected]>

nventuro requested a review from frangio June 19, 2019 00:37

stale bot added the stale label Jul 4, 2019

stale bot removed the stale label Jul 4, 2019

stale bot added stale and removed stale labels Jul 20, 2019

cag marked this pull request as ready for review July 23, 2019 21:53

stale bot added the stale label Aug 22, 2019

stale bot closed this Sep 6, 2019

andresbach reopened this Sep 17, 2019

stale bot removed the stale label Sep 17, 2019

andresbach reviewed Sep 17, 2019

View reviewed changes

frangio removed their request for review September 18, 2019 17:25

stale bot added the stale label Oct 3, 2019

stale bot removed the stale label Oct 4, 2019

cag added 4 commits October 4, 2019 09:08

Initial ERC1155 implementation with some tests

16bcf7f

Remove mocked isERC1155TokenReceiver

a41d23b

Revert reason edit nit

2d757b3

Remove parameters associated with isERC1155TokenReceiver call

2621a8b

cag added 6 commits October 4, 2019 11:38

Explicitly prevent _burn on the zero account

ae3725b

Implement batch minting/burning

77b9627

Refactored operator approval check into isApprovedForAll calls

dc86dba

Renamed ERC1155TokenReceiver to ERC1155Receiver

7f7d9eb

Added ERC1155Holder

b7125f2

Fix lint issues

077f8e9

nventuro added contracts Smart contract code. feature New contracts, functions, or helpers. labels Oct 18, 2019

nventuro self-assigned this Oct 18, 2019

stale bot added the stale label Nov 2, 2019

stale bot removed the stale label Nov 3, 2019

frangio changed the base branch from master to feature-erc1155 November 3, 2019 01:20

frangio merged commit 20642cc into OpenZeppelin:feature-erc1155 Nov 3, 2019

frangio mentioned this pull request Nov 19, 2019

ERC1155 #1728

Closed

frangio mentioned this pull request Dec 2, 2019

ERC1155 feature pending tasks #2014

Merged

6 tasks

KaiRo-at mentioned this pull request Mar 16, 2020

Port ERC 1155 branch to Solidity 0.6 (and current master) #2130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial ERC1155 implementation with some tests #1803

Initial ERC1155 implementation with some tests #1803

cag commented Jun 18, 2019

nventuro commented Jun 19, 2019

stale bot commented Jul 4, 2019

frangio commented Jul 4, 2019

stale bot commented Jul 20, 2019

cag commented Jul 23, 2019

The18thWarrior commented Jul 29, 2019

frangio commented Aug 7, 2019

The18thWarrior commented Aug 7, 2019 via email

stale bot commented Aug 22, 2019

stale bot commented Sep 6, 2019

andresbach left a comment

andresbach Sep 17, 2019

cag Oct 4, 2019 •

edited

Loading

stale bot commented Oct 3, 2019

abcoathup commented Oct 4, 2019

thecryptofruit commented Oct 8, 2019

stale bot commented Nov 2, 2019

frangio commented Nov 3, 2019

coinfork commented Nov 12, 2019

KaiRo-at commented Nov 19, 2019

frangio commented Nov 19, 2019

KaiRo-at commented Nov 19, 2019

frangio commented Nov 19, 2019

KaiRo-at commented Dec 13, 2019 •

edited

Loading

frangio commented Dec 13, 2019

Initial ERC1155 implementation with some tests #1803

Initial ERC1155 implementation with some tests #1803

Conversation

cag commented Jun 18, 2019

nventuro commented Jun 19, 2019

stale bot commented Jul 4, 2019

frangio commented Jul 4, 2019

stale bot commented Jul 20, 2019

cag commented Jul 23, 2019

The18thWarrior commented Jul 29, 2019

frangio commented Aug 7, 2019

The18thWarrior commented Aug 7, 2019 via email

stale bot commented Aug 22, 2019

stale bot commented Sep 6, 2019

andresbach left a comment

Choose a reason for hiding this comment

andresbach Sep 17, 2019

Choose a reason for hiding this comment

cag Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

stale bot commented Oct 3, 2019

abcoathup commented Oct 4, 2019

thecryptofruit commented Oct 8, 2019

stale bot commented Nov 2, 2019

frangio commented Nov 3, 2019

coinfork commented Nov 12, 2019

KaiRo-at commented Nov 19, 2019

frangio commented Nov 19, 2019

KaiRo-at commented Nov 19, 2019

frangio commented Nov 19, 2019

KaiRo-at commented Dec 13, 2019 • edited Loading

frangio commented Dec 13, 2019

cag Oct 4, 2019 •

edited

Loading

KaiRo-at commented Dec 13, 2019 •

edited

Loading