Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ERC1155 feature pending tasks #2014

Merged
merged 6 commits into from
May 8, 2020
Merged

ERC1155 feature pending tasks #2014

merged 6 commits into from
May 8, 2020

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Dec 2, 2019 •
edited by nventuro
Loading

This PR tracks the tasks pending to merge feature-erc1155, introduced in #1803.

More tasks may come out of the review.

@cag @frangio
Initial ERC1155 implementation with some tests (#1803)
20642cc 
* 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
@frangio frangio mentioned this pull request Dec 13, 2019
Merged
@frangio
Copy link
Contributor Author

frangio commented Dec 13, 2019

From @KaiRo-at in #1803:

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)

@KaiRo-at
Copy link
Contributor

FWIW, I have a very simple catch-all implementation of the metadata URI interface in KaiRo-at@859f6e7 (a second should probably follow that allows a different URI per id, the catch-all solution is for using the {id} placeholder in the URI), should this go into this PR or its own?

@frangio
Copy link
Contributor Author

frangio commented Dec 16, 2019

@KaiRo-at Please submit in its own PR to the feature-erc1155 branch. I think we should stick to the catch all URI only, at least for now.

@KaiRo-at KaiRo-at mentioned this pull request Dec 19, 2019
Merged
@KaiRo-at
Copy link
Contributor

Thanks, created #2029 but tests are still missing.

@frangio frangio added the meta label Dec 19, 2019
@nventuro nventuro mentioned this pull request Jan 29, 2020
Closed
@andresbach andresbach mentioned this pull request Mar 3, 2020
Merged
@andresbach
Copy link
Member

Added some minor changes to the test in the PR #2107 (feature-erc1155-tests branch)

In the ERC1155.behavior.js tests it may be useful to refactor more parts of the functionalities, as it was done with transferWasSuccessful and batchTransferWasSuccessful. It would be helpful to check the ERC777.behavior.js tests for that.

Also, there is a warning telling that expectEvent.inLogs() has been deprecated in favor of expectEvent(). It should be changed to the newer method.

@nventuro nventuro mentioned this pull request Apr 20, 2020
Closed
@KaiRo-at
Port ERC 1155 branch to Solidity 0.6 (and current master) (#2130)
5fd36e7 
* 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
@KaiRo-at KaiRo-at mentioned this pull request May 3, 2020
Closed
@nventuro nventuro marked this pull request as ready for review May 8, 2020 16:35
@nventuro
Copy link
Contributor

nventuro commented May 8, 2020

Working on the different ERC1155 PRs is becoming a bit of a mess, which I think we can simplify by merging current work into master and then having PRs target that, as with all other features.

This may be entirely avoidable and our fault for not properly handling release branches, but since v3.1 is expected to include ERC1155 anyway I don't think merging now will be a problem.

I created #2230 to replace this PR as the place to track progress.

@nventuro nventuro merged commit 956d663 into master May 8, 2020
@frangio frangio deleted the feature-erc1155 branch November 11, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frangio @KaiRo-at @andresbach @nventuro @cag