ethpm / ethpm-spec

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

Deficiency in how link dependencies are specified. #57

Closed pipermerriam closed 7 years ago

pipermerriam commented 7 years ago

Consider the case where we have three packages A, B, and C such that:

According to the spec link dependencies must either reference a deployed contract instance from within the same package, or from one of the dependencies which is declared in the build_dependencies. In this case, the address for the link dependency is actually sourced from a dependency of a dependency.

Source: https://github.com/ethpm/ethpm-spec/blob/master/release-lockfile.spec.md#value-value

In order to comply with the spec, C would have to be included as a dependency of A which is far from ideal.

pipermerriam commented 7 years ago

The simplest fix that I can think of is to allow specifying this link dependency like this.

{value: "B:C:SomeContractName", ...}

The above would mean:

"Use the SomeContractName deployed instance from dependency C from the dependencies from dependency B"

VoR0220 commented 7 years ago

That fix is messy....really messy. I vote 👎 on that fix. I was already planning on including all dependencies in my vendor so I don't think this affects my implementation. Whatever the solution, I would like backwards compatibility.

pipermerriam commented 7 years ago

That fix is messy....really messy

Can you expand on this? How is it messy?

VoR0220 commented 7 years ago

You're basically tying together a dependency's dependencies into one string....there's a lot of room for error here. For example, lets thing of how we import it when there are multiple contracts that B depends on. B depends on C, D, E, F, G....what does that look like?

pipermerriam commented 7 years ago

It'd be B:C:D:E:F:G:TheContractInstanceFromG. From my perspective this doesn't seem that complex. Yes, you have to reach down through each dependency lockfile and trace from dependency to dependency but it's totally deterministic.

nmushegian commented 7 years ago

I think you guys misinterpreted each other and so there is about to be confusion.

I think the right answer is, if B depends on all those, you can get B:C:Thing and then B:D:Thing. Piper's answer is if you meant that B depends on C, which depends on D, etc... you just have a traversal path B:C:D:Thing

VoR0220 commented 7 years ago

If this is the case, why not just go ahead and write down the filepath for it? That already kind of aligns with modern solidity does it not?

pipermerriam commented 7 years ago

So this doesn't have anything to do with imports. It's about how we reference deployed contract instances in other lockfiles that are somewhere in the dependency tree for a given package. Feels like we are talking about different things.

VoR0220 commented 7 years ago

I'm not saying anything about imports. Im talking about linking and indeed the filepath is a part of that now with modern solidity.

pipermerriam commented 7 years ago

So link dependencies in lockfiles don't have anything to do with file paths or solidity so I'm still failing to see the parallel. I don't understand what we're discussing. If you can give a concrete example of an alternate approach that would help me a lot.

VoR0220 commented 7 years ago

sure (and to be honest I think I may have misunderstood this at the beginning after reading it a few times over). Let me help by maybe laying out the problem as I see it, and you can correct me from there if I'm wrong.

We have Contract A, B, and C. B is specifying a linking address to C, and A relies on B. So you think that A should then have to include C as a dependency because it too needs to link B to C?

pipermerriam commented 7 years ago

Clarification/Correction

We have Contract A, B, and C. B is specifying a linking address to C contains a contract which uses a library from C, and A relies on contains a deployed instance of the contract from B.

So you think that A should then have to include C as a dependency because it too needs to link B to C?

No, I think that A should be able to leave C out of it's dependencies since it already exists in the dependency tree. In order to do this we would need to change the specification to allow link value objects to read arbitrarily deep into the dependency tree. Currently they are only allowed to reach down into direct dependencies.

nmushegian commented 7 years ago

In order to do this we would need to change the specification to allow link value objects to read arbitrarily deep into the dependency tree. Currently they are only allowed to reach down into direct dependencies.

That would be very nice actually.

pipermerriam commented 7 years ago

I'm going to get a PR open for this to make it a bit more tangible.

On Sun, Feb 19, 2017, 7:57 AM Nikolai Mushegian notifications@github.com wrote:

In order to do this we would need to change the specification to allow link value objects to read arbitrarily deep into the dependency tree. Currently they are only allowed to reach down into direct dependencies.

That would be very nice actually.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ethpm/ethpm-spec/issues/57#issuecomment-280924311, or mute the thread https://github.com/notifications/unsubscribe-auth/AAyTgs6Kax6ItyneGtYu0BBFCjL9lZ64ks5reFhsgaJpZM4MDo8H .

VoR0220 commented 7 years ago

Okay yeah my misunderstanding. This seems workable...though i wonder if perhaps one could not simply default to the link dependency specified in the package containing B unless overwritten in the current package by means of something like the solution you are proposing above.

jeffanthony commented 7 years ago

Perhaps I'm missing something too but: At the time B is deployed C must be referenced within. At the time A is deployed B must be referenced within. Does A call functions from C directly? perhaps it should be controlled by B?

gnidan commented 7 years ago

I am wondering about separation of concerns - if contracts had to list the entire tree of dependencies, that would get very unwieldy, when really, I would expect the maintainer of a contract only to be required to list the direct dependencies.

pipermerriam commented 7 years ago

@jeffanthony

Perhaps I'm missing something too but: At the time B is deployed C must be referenced within. At the time A is deployed B must be referenced within. Does A call functions from C directly? perhaps it should be controlled by B?

Hopefully this clears things up.

pipermerriam commented 7 years ago

I am wondering about separation of concerns - if contracts had to list the entire tree of dependencies, that would get very unwieldy, when really, I would expect the maintainer of a contract only to be required to list the direct dependencies.

It's not entirely clear to me what your issue (if any is). Smart contract packages inherently define a full tree of dependencies but you are correct that it would be unwieldy to require each package to specify the full tree. The current specification only requires a package to declare it's direct dependencies. This issue is meant to help make sure that remains the case by ensuring that packages aren't required to duplicate dependencies that are met further down the tree.

VoR0220 commented 7 years ago

Perhaps giving example with bytecode and trees would help explain this best.

VoR0220 commented 7 years ago

And even better the different lock files.

pipermerriam commented 7 years ago

@mhhf @iurimatias @tcoulter any objections to this going in (and being backfilled to the 1.0.0 spec since it is not a breaking change)?