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

ERC-1363: Move to Last Call #2617

Merged

Conversation

vittominacori
Copy link
Contributor

@vittominacori vittominacori commented Apr 26, 2020

Moves ERC-1363 to Final.

@eip-automerger
Copy link

eip-automerger commented Apr 26, 2020

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 1363 state from Draft to Last Call

@vittominacori vittominacori changed the title 1363: Move to Last Call ERC-1363: Move to Final May 7, 2020
@axic
Copy link
Member

axic commented Sep 3, 2020

It is not possible to move from draft to final in a single step, please refer to EIP-1.

@vittominacori vittominacori changed the title ERC-1363: Move to Final ERC-1363: Move to Last Call Sep 5, 2020
@vittominacori
Copy link
Contributor Author

Updated to Last Call.

It was Last Call first but automerger fails. So I moved to final.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Last Call must have a review-period-end of at least 2 weeks in the future.

EIPS/eip-1363.md Show resolved Hide resolved
Co-authored-by: Micah Zoltu <[email protected]>
@MicahZoltu
Copy link
Contributor

## Security Considerations section is required for all EIPs. It comes right before the Copyright section. If you believe there are no security considerations, the section can just include one-sentence saying as much.

@MicahZoltu
Copy link
Contributor

I would also recommend changing:

Every Payable Token compliant contract MUST implement the ERC-1363 interface other than ERC-20 and ERC-165 interfaces.

to

Every Payable Token compliant contract **MUST** implement the ERC-1363 interface as well as the ERC-20 and ERC-165 interfaces.

@MicahZoltu
Copy link
Contributor

I recommend using the RFC 2119 style for **MUST** rather than MUST.

@MicahZoltu
Copy link
Contributor

Recommend changing:

Description of a Payable Token compatible with the ERC-20 definition. Also description of a Token Receiver and/or Spender that accepts Payable Token payments.

to

Defines a token interface for ERC-20 tokens that supports executing recipient code on transfer.

@MicahZoltu
Copy link
Contributor

Recommend changing the abstract to be a bit more descriptive, maybe mentioning the highlights of the new functions. I generally recommend the abstract contain a human-readable and terse version of the specification. Something like, "Adds a transferAndCall method that allows the caller to specify that the that the recipient contract will be called with calldata supplied by the caller. ...". Maybe add a sentence or two about some other features, but I recommend leaving out the specific details like all of the overloads, and the exact mechanism for callbacks, etc.

Basically, state the core protocol in human readable words, but don't cover all of the edge cases and little details.

@MicahZoltu MicahZoltu self-requested a review September 9, 2020 05:21
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

The Security Considerations section is a hard requirement. The rest are feedback that I encourage applying, but aren't strictly required to move to Last Call.

@MicahZoltu
Copy link
Contributor

I know I said that the other changes were optional, but on doing a final review I think this change is necessary afterall because the current wording doesn't align with the rest of the document:

Every Payable Token compliant contract MUST implement the ERC-1363 interface other than ERC-20 and ERC-165 interfaces.

to

Every Payable Token compliant contract MUST implement the ERC-1363 interface as well as the ERC-20 and ERC-165 interfaces.

"other than" implies that someone implementing this standard MUST NOT implement ERC-20 and ERC-165, which I don't think was the intent.

Along with that change, please also set the review period end to 2 weeks from the date of that change as I hope this is the last change before Last Call. 😄

@MicahZoltu MicahZoltu merged commit 64fefe6 into ethereum:master Sep 13, 2020
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
Defines a token interface for ERC-20 tokens that supports executing recipient code on `transfer` or spender code on `approve`.
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Defines a token interface for ERC-20 tokens that supports executing recipient code on `transfer` or spender code on `approve`.
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