Bump 2678 to Review status

[njgheorghita]

Bump EIP 2678 (ethpm v3) to Review status.

[njgheorghita]

@MicahZoltu Is it necessary to stop through the Review status before moving onto Last Call? If so, what's the typical duration of the Review stage?

[eip-automerger]

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

• File assets/eip-2678/package.spec.json is not an EIP

[MicahZoltu]

Recommend adding something to the rationale indicating why these are minified. There doesn't seem to be any obvious benefit to doing so as JSON doesn't minify particularly well (you only can clear out newlines and tabs, both of which gzip will compress quite well).

The "Guiding Principles" subsection of the specification should be in the Motivation section. The specification should be purely a technical specification and not discuss any of the "why".

The first paragraph of the specification should be moved to motivation or removed (I think it is redundant).

The second paragraph of the specification should be removed. It isn't part of the specification and is redundant with EIP process documentation and likely to become out of date if/when the EIP repository or hosted site moves.

Can this specification just be a JSON schema with comments? Having it written as an english language specification seems like it just makes it much longer and more confusing than necessary.

The document constraints should all have an entry in the rationale section as they seem unnecessarily restrictive to the naive reader, thus are deserving of explanation as to why they exist. Separately, I recommend removing most of the constraints and accepting any valid JSON file, which is already a well specified format.

Recommend adding a rationale entry for the constraints on the package name field. This is 2020, every reasonable system supports full UTF-8 strings for pretty much everything so constraining to a subset of UTF-8 is deserving of an explanation.

Don't link externally to package.spec.json. Instead, put a copy in ../assets/eip-2678/package.spec.json and link to it as a relative path link.

Literal values are defined as a byte string, but that isn't defined in this specification. I believe the concept of a byte string is a python thing, so not all developers will be familiar with it and thus I think it is worth defining explicitly.

The source checksum should be mentioned in the Rationale section. JSON is essentially always sent over a transport that has built-in error checking/checksumming so having another one seems redundant. On top of that, only having a checksum on the source but not the rest of the file is surprising. From a security standpoint, a checksum that is distributed with the source is pointless as if one is compromised then so is the other.

This specification seems to have a lot of overlap with the Solidity compiler input JSON. Has there been any attempt to work with them to have a single consolidated manifest file rather than two competing specifications?

Recommend adding a rationale entry for why BIP122 was chosen. Since this specification is ETH specific (EthPM), it seems unnecessary at first glance to use BIP122 which is not generally used within this ecosystem.

The current contents of the Rationale should be moved to the Motivation section. The Rationale is for giving reasoning why specific design choices were made, while the motivation section is for explaining why this specification may be useful to someone. Use cases are a great piece of content for the motivation section.

Try to avoid linking externally, such as to EthPM repository. Instead, it is recommended to inline or use the assets directory. IF this specification is tightly coupled with EthPM, consider just including it in the EthPM documentation rather than using the EIPs repository for it.

Remove the Implementations section. It is no longer part of the EIP template.

Further Work isn't part of the EIP template and isn't appropriate for a technical specification. Consider putting such content in a blog, documentation site, project tracker, etc. rather than in the technical spec.

Acknowledgements shouldn't be included in EIPs. EIPs are CC0 licensed which means no attribution is necessary. Consider including shoutouts, acknowledgements, kudos, attributions, etc. in a Tweet or launch post rather than the technical document.

Remove this line from the Security Considerations and instead re-write (copy/paste if necessary) any security considerations for this EIP rather than making people go hunt them down elsewhere:

The same security considerations from v2 are still relevant for EthPM v3.

[MicahZoltu]

@MicahZoltu Is it necessary to stop through the Review status before moving onto Last Call? If so, what's the typical duration of the Review stage?

Yeah, review phase is necessary. There is no prescribed time limit, ideally you will use this phase to get external feedback from other people such as potential users of this standard. It also gives us (editors) an opportunity to do a more thorough review of the EIP once it has stabilized.

[njgheorghita]

@MicahZoltu Thanks for the feedback! I appreciate the thoughts, let me know if there's anything else I can help clear up or change.

Can this specification just be a JSON schema with comments? Having it written as an english language specification seems like it just makes it much longer and more confusing than necessary.

I'd agree that what you suggest would be an easier to read format, but if this document is to act as the de-facto definition of the packaging standard, isn't it important that it contains a detailed explanation of the specification?

This specification seems to have a lot of overlap with the Solidity compiler input JSON. Has there been any attempt to work with them to have a single consolidated manifest file rather than two competing specifications?

Yes! A lot of discussion with the solidity team went into the changes from v2 to v3. Finding a common data format for EthPM and solidity compiler metadata was the primary motivation for updating the ethpm spec to v3. Compatibility for the input json has been discussed, but for v3 we decided to focus just on metadata.

The source checksum should be mentioned in the Rationale section. JSON is essentially always sent over a transport that has built-in error checking/checksumming so having another one seems redundant. On top of that, only having a checksum on the source but not the rest of the file is surprising. From a security standpoint, a checksum that is distributed with the source is pointless as if one is compromised then so is the other.

I'm not sure I follow. The json file (manifest) is stored on a content-addressable filesystem (ipfs), whose URI contains a checksum - ensuring the validity of the file contents, source checksums included.

The document constraints should all have an entry in the rationale section as they seem unnecessarily restrictive to the naive reader, thus are deserving of explanation as to why they exist. Separately, I recommend removing most of the constraints and accepting any valid JSON file, which is already a well specified format.

I've added a line to the rationale explaining that since these manifests are published on content-addressable filesystems, where they are identified by a content hash, it is important to have these constraints to ensure that any given set of contract assets will always resolve to the same content-addressed URI.

[MicahZoltu]

I'd agree that what you suggest would be an easier to read format, but if this document is to act as the de-facto definition of the packaging standard, isn't it important that it contains a detailed explanation of the specification?

My recommendation is JSON-Schema with comments, where the comments are in the description. This is a weak recommendation, and it is up to you (the author) to decide if this would make everything more readable/usable.

I'm not sure I follow. The json file (manifest) is stored on a content-addressable filesystem (ipfs), whose URI contains a checksum - ensuring the validity of the file contents, source checksums included.

I'm specifically referring to the source checksum which explicitly says that it is required if there is no URI (meaning the file is not available on a content-addressable filesystem). Either you have a URI like ipfs://... or swarm://..., or you don't. In the case that you do, the checksum field is redundant. In the case that you don't, the checksum field is useless.

I've added a line to the rationale explaining that since these manifests are published on content-addressable filesystems, where they are identified by a content hash, it is important to have these constraints to ensure that any given set of contract assets will always resolve to the same content-addressed URI.

Excellent! This is exactly the piece I was missing, and what makes for a great rationale item. 😁

[MicahZoltu]

As version 3 of this specification, this standard seeks to address a number of areas of improvement found for the previous version (defined in EIP-1123). This version:

Final EIPs should only be referencing other final EIPs. Since EIP-1123 was withdrawn, this means that it will most likely never make it to final, which means such a reference will end up being a blocker for this EIP.

In general, I recommend that EIPs be standalone technical standards unless they are expressly upgrading or mutating an existing final EIP. In this case, just deleting that section of the Motivation section should be enough.

Also, replaces header field should be removed for the same reason.

---

Package name regex does not match the human description. Regex is limited to ascii-7 letters, not all lowercase letters (which is a huge set in UTF-8, which is the document encoding).

[njgheorghita]

I'm specifically referring to the source checksum which explicitly says that it is required if there is no URI (meaning the file is not available on a content-addressable filesystem). Either you have a URI like ipfs://... or swarm://..., or you don't. In the case that you do, the checksum field is redundant. In the case that you don't, the checksum field is useless.

In the case that you don't have a URI - it is required to include the contract source as a string (under the "content" key) in which case the checksum field is also required, containing a checksum object with the algorithm used to generate a hash and the resolved hash of the inlined source.

[MicahZoltu]

I continued the discussion about the hash over in the discussion-to thread, which is a more appropriate place for that conversation. This EIP looks technically sound and properly structured at this point.

I do still contend that the content hash requirement should be removed or it should be clarified why it is necessary in the rationale section. However, this can go into Review with that left outstanding.