search

ethpm / ethpm-spec

Ethereum Package Manager http://ethpm.github.io/ethpm-spec/
165 stars 30 forks source link

Source ID mismatch in examples #170

Closed fubuloubu closed 2 years ago

fubuloubu commented 2 years ago

In the examples, under each ContractType object, the sourceId field does not match anything under the sources field. Here are the sourceId fields for the Escrow example: https://github.com/ethpm/ethpm-spec/blob/62a6ce2e7d6e10b28e02b0b42db56890f65486ce/examples/escrow/v3-pretty.json#L98 https://github.com/ethpm/ethpm-spec/blob/62a6ce2e7d6e10b28e02b0b42db56890f65486ce/examples/escrow/v3-pretty.json#L121

As you'll notice, these do not correspond with the keys of the sources object: https://github.com/ethpm/ethpm-spec/blob/62a6ce2e7d6e10b28e02b0b42db56890f65486ce/examples/escrow/v3-pretty.json#L155 https://github.com/ethpm/ethpm-spec/blob/62a6ce2e7d6e10b28e02b0b42db56890f65486ce/examples/escrow/v3-pretty.json#L162

The solution here should be to rectify these two fields together by removing the base path (e.g. ./), as the EIP specification does not mention an implicit assumption of the base path when referencing sourceId in the sources object.

njgheorghita commented 2 years ago

Good catch - I guess an alternative solution is to prepend ./ to the value of the "sourceId" fields. Both solutions are technically correct if I'm understanding correctly. Personally, your solution seems to be a bit cleaner.

fubuloubu commented 2 years ago

Good catch - I guess an alternative solution is to prepend ./ to the value of the "sourceId" fields. Both solutions are technically correct if I'm understanding correctly. Personally, your solution seems to be a bit cleaner.

I've noticed some of the other examples do not use the prepended ./. I don't think that the specification says one way or the other (not that it should), the only issue with this specific example is that they don't match. I'll fix this in a PR, just wasn't sure if there was something I missed in the spec or a particular guideline about how they should be specified

fubuloubu commented 2 years ago

The only thing I've noticed is https://eips.ethereum.org/EIPS/eip-2678#the-source-object where it notes that the installPath must start that way.

njgheorghita commented 2 years ago

Yup, and there's no requirement that the install path needs to match the key in the "sources" object. I'd rather remove the ./ from the "sources" key.

fubuloubu commented 2 years ago

Yup, and there's no requirement that the install path needs to match the key in the "sources" object. I'd rather remove the ./ from the "sources" key.

Alright, will do this