Update eip-173.md, last call

[mudgen]

Set status to last call.

[eip-automerger]

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

[mudgen]

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

@MicahZoltu Help with this please.

[MicahZoltu]

The following standard allows for the implementation of a standard API for getting the owner address of a contract and transferring contract ownership to a different address.

This statement is in the middle of the standard, so it really isn't "the following standard" but instead "this standard".

[MicahZoltu]

Most of the content currently in the abstract should be moved to the motivation section or the rationale section. The motivation section is "why does this exist", the rationale section is "why did we make these particular choices" and the abstract is a "what is this". Explaining why you included only a couple functions is a good candidate for the rationale section. Maybe keep the "gas efficient" bit in the abstract, since that may be a good "what is this" statement: "A mechanism for establishing ownership and ownership transfer in a gas efficient way".

[MicahZoltu]

Consider putting MUST and SHOULD in bold and capitalized in the specification section, and also consider putting the RFC blurb at the beginning of the specification section:

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

[MicahZoltu]

The OwnershipTransferred event does not have to be emitted when a contract is created.

This has caused problems in the past with ERC20 due to the way tools aggregate data. I recommend changing this to assert that the owner transferred event MUST be fired on contract creation if an owner is assigned (anything other than 0 address).

[MicahZoltu]

Consider having a separate event for OwnershipRenounced rather than using the same event with a sentinel value. This will make it a bit easier to find instances of ownership destruction due to the way events are indexed in databases.

[MicahZoltu]

/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-173.md

Recommend removing this line as specifications ideally shouldn't have external links in them.

[MicahZoltu]

Recommend removing all of the comments from the specification actually, they are not part of the specification. If you want to include some interfaces that people can copy/paste into their Solidity project I recommend including them in a code block in the Implementations section of the EIP, rather than the specification section. Ideally, the specification section ONLY includes the actual specification (stuff that must be standardized on) and doesn't contain superfluous material.

Note: this includes removing the pragma solidity at the top, since that isn't part of the specification. You may also want to consider removing the interfaces and just having a function list instead (though from an organizational standpoint, they may be valuable)

[MicahZoltu]

Security Considerations section is missing and is required for all EIPs. If there are no known security related issues then you can just say that, but the section must be present.

[MicahZoltu]

Nitpick: This EIP has a dozen or two blank lines at the end of the document that should probably be cleaned up.

[mudgen]

The following standard allows for the implementation of a standard API for getting the owner address of a contract and transferring contract ownership to a different address.

This statement is in the middle of the standard, so it really isn't "the following standard" but instead "this standard".

Understood. I updated this.

Consider putting MUST and SHOULD in bold and capitalized in the specification section, and also consider putting the RFC blurb at the beginning of the specification section:

I understand and considered this. I feel that the standard is clear without this.

The OwnershipTransferred event does not have to be emitted when a contract is created.

This has caused problems in the past with ERC20 due to the way tools aggregate data. I recommend changing this to assert that the owner transferred event MUST be fired on contract creation if an owner is assigned (anything other than 0 address).

I understand. I changed the text to say that the OwnershipTransferred event should be emitted. I don't say must in order to maintain backwards compatibility.

Consider having a separate event for OwnershipRenounced rather than using the same event with a sentinel value. This will make it a bit easier to find instances of ownership destruction due to the way events are index

Thank you, I considered this. I'm not adding this because it is not backwards compatible with existing implementations. New implementations are free to implement this.

/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-173.md

Recommend removing this line as specifications ideally shouldn't have external links in them.

Thank you, I removed it.

Recommend removing all of the comments from the specification actually, they are not part of the specification. If you want to include some interfaces that people can copy/paste into their Solidity project I recommend including them in a code block in the Implementations section of the EIP, rather than the specification section. Ideally, the specification section ONLY includes the actual specification (stuff that must be standardized on) and doesn't contain superfluous material.

Note: this includes removing the pragma solidity at the top, since that isn't part of the specification. You may also want to consider removing the interfaces and just having a function list instead (though from an organizational standpoint, they may be valuable)

I understand. I want to keep the comments and interfaces. Good point about the pragma solidity; I removed that.

Nitpick: This EIP has a dozen or two blank lines at the end of the document that should probably be cleaned up.

Thank you, I removed the blank lines.

Most of the content currently in the abstract should be moved to the motivation section or the rationale section. The motivation section is "why does this exist", the rationale section is "why did we make these particular choices" and the abstract is a "what is this". Explaining why you included only a couple functions is a good candidate for the rationale section. Maybe keep the "gas efficient" bit in the abstract, since that may be a good "what is this" statement: "A mechanism for establishing ownership and ownership transfer in a gas efficient way".

Thank you for these clarifications. I understand that there is some overlap. I feel that the abstract how it is now helps the reader understand the standard clearly.

Security Considerations section is missing and is required for all EIPs. If there are no known security related issues then you can just say that, but the section must be present.

Understood. I added a Security Considerations section.

[MicahZoltu]

I have been trying to get another editor to review this but at the moment they are all busy. My current reluctance to merge is because currently, I personally believe that the sections are not being used correctly and I'm a bit of a stickler for that sort of thing. I suspect once another editor gets a chance to review they probably will care less (or openly disagree with me) and merge without any changes.

• Simple Summary: One-liner "what is this".
• Abstract: What this EIP does (human readable).
• Motivation: Why this EIP exists.
• Specification: Technical (almost computer readable).
• Rationale: Why specific decisions were made.

At the moment, you have rationale and motivation stuff in the abstract, and the abstract doesn't (IMO) go into quite enough detail about how the specification works (basically human readable high level overview of the specification section). Something like this is what I would prefer to see in an abstract:

This specification defines standard functions for a single-owner contract. An implementation allows reading the current owner (owner() returns (address)) and transferring ownership (transferOwnership(address newOwner)) along with standardized events for when ownership is changed (OwnershipTransferred(address indexed previousOwner, address indexed newOwner)).

---

Again, I want to stress that these are my personal thoughts and not the thoughts of all of the editors (we don't agree on everything), but I wanted to let you know what was holding me back from merging at the moment so you didn't think I had simply forgotten about this.

[mudgen]

@MicahZoltu Thank you. I appreciate your feedback and help. I think the abstract you wrote is good and I added it. I made the changes. Can we merge it?

[MicahZoltu]

@mudgen Sadly, at the moment nothing is getting merged in this repository as CI is broken across the board. I'll snooze the email reminder for next week so I don't forget to get this merged once CI is fixed (and continue to snooze until CI is fixed), though feel free to ping me if you see this go green and I'll take a look!

On a personal note, I feel bad that I couldn't get any other editor to merge this previously (before CI broke). I dislike the idea that I'm currently the gatekeeper for all EIPs and my opinion is the only one that matters. I liked it better when you just had to get one of n editors to agree to your EIP's layout/wording to get merged.

Sadly, n seems to be 1 at the moment. 😢

[mudgen]

Thanks @MicahZoltu
I understand. One good editor is better than none. I appreciate your good work and help.

[MicahZoltu]

Would you mind bumping this up to 2020-09-03? I know it isn't your fault, but this should really be 2 weeks after merged so people have enough time after automated notification goes out.

[MicahZoltu]

Change this to 2 weeks after whatever date you manage to get around to updating the date. 😬

[mudgen]

Yes, this is done now. Thank you!