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

ERC-5006: Change Status to Review #5147

Merged
merged 2 commits into from
Jun 28, 2022
Merged

ERC-5006: Change Status to Review #5147

merged 2 commits into from
Jun 28, 2022

Conversation

emojidao
Copy link
Contributor

ERC-5006: Change Status to Review

@eth-bot
Copy link
Collaborator

eth-bot commented Jun 13, 2022 •
edited
Loading

All tests passed; auto-merging...

(pass) eip-5006.md

classification
updateEIP
  • passed!

(pass) assets/eip-5006/.gitignore

classification
ambiguous
  • file assets/eip-5006/.gitignore is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/contracts/ERC5006.sol

classification
ambiguous
  • file assets/eip-5006/contracts/ERC5006.sol is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/contracts/ERC5006Demo.sol

classification
ambiguous
  • file assets/eip-5006/contracts/ERC5006Demo.sol is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/contracts/IERC5006.sol

classification
ambiguous
  • file assets/eip-5006/contracts/IERC5006.sol is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/hardhat.config.ts

classification
ambiguous
  • file assets/eip-5006/hardhat.config.ts is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/package.json

classification
ambiguous
  • file assets/eip-5006/package.json is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/test/test.ts

classification
ambiguous
  • file assets/eip-5006/test/test.ts is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

(pass) assets/eip-5006/tsconfig.json

classification
ambiguous
  • file assets/eip-5006/tsconfig.json is associated with EIP 5006; because there are also changes being made to EIPS/eip-5006.md all changes to corresponding assets are also allowed

@poojaranjan poojaranjan mentioned this pull request Jun 14, 2022
Closed
Pandapip1
Pandapip1 suggested changes Jun 14, 2022
View reviewed changes
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my first pass.

EIPS/eip-5006.md
@@ -4,7 +4,7 @@ title: ERC-1155 Usage Rights Extension
description: Add a user role with restricted permissions to ERC-1155 tokens.
author: Lance (@LanceSnow), Anders (@0xanders), Shrug <[email protected]>
discussions-to: https://ethereum-magicians.org/t/eip5006-erc-1155-usage-rights-extension/8941
status: Draft
status: Review
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on unchanged portions. Here are some things that need changing:

  • L3: Do not include the EIP number in the title

  • L4: Change ERC-1155 to EIP-1155.

  • L11: 721 needs to be added (see L20)

  • L16: A link to EIP-1155 needs to be provided (and needs to be referenced as an EIP):

    ERC-1155 -> [EIP-1155](./eip-1155.md)

  • L20: Add a link to EIP-721. A link to EIP-1155 isn't necessary (but feel free to include one)

    ERC-721 -> [EIP-721](./eip-721.md)

  • L32: The UpdateUser event should have some NatSpec.

  • L78: I would recommend using multi-line comments for consistency.

  • L97: I would rephrase the last sentence as:

    The following are some problems that are solved by this standard:

  • L101: There is a double space and there are two contractions (change them to "it is")

  • L109: Compatible with what?

  • L111: Mention that there are no conflicts between EIP-5006 and EIP-1155

  • L339: Please elaborate. It is currently unclear what this means.

  • L148-L335: I would recommend moving the reference implementation and test cases out of inline and linking to their copies in the assets folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EIP-1155 is fine in the title, since this is an extension.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EIP-1155 is fine in the title, since this is an extension.

For some reason, I thought that the title originally contained the actual EIP number (5006). Must have misread it!

@poojaranjan poojaranjan mentioned this pull request Jun 28, 2022
Closed
@lightclient lightclient reopened this Jun 28, 2022
@kodiakhq kodiakhq bot merged commit 5f716e2 into ethereum:master Jun 28, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
@emojidao @0xanders @nachomazzara
ERC-5006: Change Status to Review (ethereum#5147)
9b7155c 
* Change Status to Review

* update by Pandapip1's suggestion

Co-authored-by: Anders <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamWilsn SamWilsn SamWilsn left review comments

@Pandapip1 Pandapip1 Pandapip1 requested changes

@0xanders 0xanders 0xanders approved these changes

@lightclient lightclient lightclient approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@emojidao @eth-bot @lightclient @Pandapip1 @SamWilsn @0xanders