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.

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

Update ERC-7578: Move to Review #501

Merged
merged 35 commits into from
Jul 9, 2024
Merged

Conversation

V1d0r
Copy link
Contributor

@V1d0r V1d0r commented Jun 20, 2024

Move to Review

gabrielstoica and others added 27 commits May 9, 2024 13:04
ERC-7578 interface & reference implementation updates
Formatting for bot.
Improve documentation and standard naming
ERC-721 links
Removed external link
Add `virtual` to allow `_update` overridden implementations based on the ERC-7578
Remove `properties` field from `PropertiesRemoved` event
Fix potential linearization issue when inheriting 'ERC721' in the child contract
Update `Security Considerations` section
@github-actions github-actions bot added the t-erc label Jun 20, 2024
@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Jun 20, 2024

✅ All reviewers have approved.

Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Please only use backticks (```) for inline code. You're using them in your motivation section for definitions.


Those definitions in your motivation section, why are they there? Are they used later in the document, and need specific definitions? If so, please move them to the Specification section. If they're supposed to be motivating examples, maybe try changing the formatting to make that more clear?


Where is "set mint" (line 52) defined?


On line 54, try to describe the externally visible behaviour of the contract. Like instead of "if the token burns, the properties MUST be deleted", how about "if the token is burned, getProperties MUST revert"


On line 127, you aren't really stating a requirement on implementers. You should describe the behaviour of the contract if setProperties is not called, instead of saying how the contract is supposed to be used.


On line 139, you mention an _update function that must be overridden. _update doesn't exist in the ERC-721 standard. I believe it's part of OpenZeppelin's implementation. Instead of referencing a specific implementation here (at the risk of sounding like a broken record) you should describe the externally visible behaviour of the contract in terms of the functions in ERC-721.

@gabrielstoica
Copy link

gabrielstoica commented Jul 1, 2024

Hey @SamWilsn!

Thanks for your feedback - it was very valuable and we've tried to cover all points as follows:

  • Moved all definitions to the specification section;
  • Updated the rationale to be less restrictive and define the externally visible behaviour;
  • Removed the setProperties definition to improve the standard's interoperability;

Edit: We've also addressed your point from the Ethereum Magicians forum here.

Let me know what you think!

@eip-review-bot eip-review-bot enabled auto-merge (squash) July 9, 2024 14:19
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eip-review-bot eip-review-bot merged commit f25b023 into ethereum:master Jul 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants