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 operator language in #721 #931

Merged
merged 4 commits into from
Mar 28, 2018
Merged

Update operator language in #721 #931

merged 4 commits into from
Mar 28, 2018

Conversation

johnhforrest
Copy link
Contributor

Creating pull request to clarify some language as requested by @fulldecent in discussion at #721. Draft for this EIP was accepted in #841.

  • Fixed typo (operator to operators)
  • Explicitly call out the implementation MUST support multiple operators per owner as implied by the comments
  • Removed unnecessary throw from the dev instructions on setApprovalForAll

@fulldecent Not sure if the dev instruction is the best place to call out the specifics of the operator, feel free to recommend a different location if you feel strongly about the placement.

@fulldecent
Copy link
Contributor

Please review conflict, just merged another PR. Thank you!

@fulldecent
Copy link
Contributor

Sorry, GitHub looks messed up here. Both these changes to master seem to already be in master. Maybe I am looking at this wrong.

@johnhforrest
Copy link
Contributor Author

Look at the PR instead of the individual commits--the most recent commit is just me merging changes from master to my branch to resolve the conflicts you mentioned. Unfortunately it looks like when editing directly on the github.com UI it does a merge instead of a rebase, so it shows the extra commit in history.

@fulldecent
Copy link
Contributor

I see now, thank you. Can you please move the "The contract MUST allow multiple operators per owner." line into the setApproveForAll @dev part.

@Arachnid
Copy link
Contributor

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

- Fixed typo (operator to operators)
- Explicitly call out the implementation MUST support multiple operators per owner as implied by the comments
- Removed unnecessary throw from the dev instructions on setApprovalForAll
@fulldecent
Copy link
Contributor

Thank you. +1. @nicksavers please merge.

@gcolvin gcolvin merged commit 5a68d6b into ethereum:master Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants