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

721, the first graduate from the Last Call process #1170

Merged
merged 1 commit into from Jun 22, 2018

Conversation

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Jun 21, 2018

This pull requests promotes ERC-721 from Last Call status to Final status.

One immaterial change was made during the Last Call and all questions were addressed.

The time now is Thursday in UTC time zone. We have added two days to the Last Call in good taste.

Policy

Relevant policy is given in EIP-1 for when to promote an EIP from Last Call status to Final status:

➡️ Final (Not core EIPs) -- A successful Last Call without material changes or unaddressed complaints will become Final.

All changes

ERC-721 began Last Call with commit c9a9320#diff-a87271f8186c59c075e229cd7421f205

Every change since then are viewable with the compare view: https://gith ub.com/ethereum/EIPs/compare/c9a9320...master#diff-a87271f8186c59c075e229cd7421f205

^ github bug / you need to copy paste and remove the space to access url

The entire set of changes is:

  • Fix punctuation (add and remove periods, remove underscore)
  • Make NatSpec documentation more clear, use consistent words
  • Add operator parameter to onERC721Received callback function
  • Add justification for operator parameter into rationale section

These documentation improvements and this additional parameter are not material changes.

All discussion

The designated place for discussion is #721

The comment just before Last Call status is #721 (comment)

The entire list of discussion topics were:

  • @johnhforrest proposed to remove underscores from event parameter names -- RESOLUTION change not adopted, this is immaterial for an ABI
  • @mg6maciej proposed to add the parameter operator to the token receiver callback, dissent from @paulbarclay was that this increases complexity because a workaround exists, counterpoint is that the workaround requires an extra transfer and does not follow the philosophy of 721 -- RESOLUTION this change was adopted
  • @jacqueswww noted that Vyper is incompatible 721 because we use two functions with the same name (overloading) -- RESOLUTION no change is necessary, Vyper will update to support function default parameters and be compatible with 721
  • @paulbarclay proposed to remove one transfer function -- RESOLUTION change not adopted, we have previously discussed this at length and our transfer mechanism has strong community support and documented rationale (the discussion during last call added no new arguments in this point)
  • @mudgen proposed adding a the bytes data into the event log -- RESOLUTION change not adopted, this would cost much gas and not be useful, applications can do their own application-specific logging
  • @mudgen questioned why functions throw rather than return false -- RESOLUTION change not adopted, ERC20's decision is well-recognized as a poor design choice
  • @mudgen proposed allowing an arbitrary number of URIs per token -- RESOLUTION change not adopted, this smells like an application-specific requirement and does not benefit interoperability
  • @frangio requested that a list of major changes should be published -- RESOLUTION this was added as a comment to the #721 discussion

All questions/comments have been fully addressed.

REFERENCE: - #1150

@eip-automerger
Copy link
Collaborator

@eip-automerger eip-automerger commented Jun 21, 2018

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • Trying to change EIP 721 state from Last Call to Final
@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Jun 21, 2018

Any objections to moving this to final from those mentioned as having submitted comments?

@Souptacular
Copy link
Member

@Souptacular Souptacular commented Jun 21, 2018

I have no objection.

@paulbarclay
Copy link

@paulbarclay paulbarclay commented Jun 21, 2018

@mudgen
Copy link
Contributor

@mudgen mudgen commented Jun 21, 2018

I have no objections.

1 similar comment
@MoMannn
Copy link

@MoMannn MoMannn commented Jun 21, 2018

I have no objections.

@davision
Copy link

@davision davision commented Jun 21, 2018

No objections.

@xpepermint
Copy link

@xpepermint xpepermint commented Jun 21, 2018

No objections ;).

@mg6maciej
Copy link
Contributor

@mg6maciej mg6maciej commented Jun 21, 2018

👍 Congratulations to @fulldecent for execution of the whole process. 🥇

@fulldecent
Copy link
Contributor Author

@fulldecent fulldecent commented Jun 21, 2018

@jacqueswww @frangio @johnhforrest a little help please?

@johnhforrest
Copy link
Contributor

@johnhforrest johnhforrest commented Jun 21, 2018

Ship it!

@frangio
Copy link
Contributor

@frangio frangio commented Jun 21, 2018

No objections.

@jacqueswww
Copy link

@jacqueswww jacqueswww commented Jun 21, 2018

No objections. 🎉🛫

@dekz
Copy link

@dekz dekz commented Jun 21, 2018

No objections

@dete
Copy link

@dete dete commented Jun 22, 2018

I have no objections.

HUGE thanks to @fulldecent for running this down a very long field. Glad to see this getting finalized!

@fulldecent
Copy link
Contributor Author

@fulldecent fulldecent commented Jun 22, 2018

Thank you!!!

If my calculation is correct we now have unanimous consent to merge.

@nicksavers @Arachnid @Souptacular who would like the honors?

@nicksavers nicksavers merged commit b015a86 into ethereum:master Jun 22, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xinbenlv
Copy link
Contributor

@xinbenlv xinbenlv commented Jul 9, 2018

Congrats on moving on final!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet