Conversation
gnidan
left a comment
There was a problem hiding this comment.
Thanks for doing the write-up here, this is exactly what I was attempting to convey :)
|
this is great, because now I can see what I think is a problem... As it stands:
I think this ?unintentionally? removes the option to ship contract types with their bytecode pre-linked. It may not be explicitely stated, and it probably should be explicitely stated but I think this is an important use case to support. Without this, it becomes impossible to ship ready-to-use contracts which make use of libraries. Am I missing something? |
|
|
||
| The ``link_references`` field defines the locations in the corresponding | ||
| bytecode which require |linking|. | ||
| bytecode which require |linking|. If bytecode contains no link references, this field should be an empty array. |
I'd say that if a package doesn't want to expose already-linked types, it can just say I agree though that this should be explicitly stated: "if you want to link your contracts and not tell EthPM about it, just say there are no link references." I don't think this requires any special consideration, in the form of a polymorphic bytecode object. |
I might be missing something, but does this cover if a contract type does want to expose an already-linked library (maybe for validation purposes), yet still have unlinked bytecode/link references? |
Hm, for validation purposes? This is probably only covered via the use of the compiler settings property, to indicate that specific link values were provided to the compiler ahead of time. |
pipermerriam
left a comment
There was a problem hiding this comment.
I'd say that if a package doesn't want to expose already-linked types, it can just say
link_references: []
I think you two are saying similar things but I don't like the proposed solution of relying either on compiler settings or just not exposing what was linked and how.
My thoughts:
- Exposing a
ContractTypethat is pre-linked is likely to be a very common use case. - If the data for "how it was pre-linked" is not in a machine readable format then I don't think we've fulfilled this use case because these specs are designed to be machine readable.
- Using compiler settings to expose link information isn't adequate because you then lose semantic information about what packages it was linked against, etc.
This is my reasoning for why I'm 👎 on a change that doesn't allow us to include link_dependencies in the bytecode for ContractType objects.
a24d552 to
a2c6ea6
Compare
a2c6ea6 to
7d17b9c
Compare
|
@pipermerriam Added |
|
So for contract types that have pre-linked value, does that bytecode still have zeroes? If that's true, then it requires additional processing pre-publish, to convert them to zeroes. If it's not true, well, that's another required spec update. |
|
Another thing: what if a contract only wants to pre-link some of its link refs? So then |
|
Feels weird to convert the linked bytecode value to Can we just create a new "type" of link value? |
|
Is it a requirement that |
|
And we can change the zeroes requirement from must to should, that way we can try to get the tools to zero out pre-fill values, but it's not entirely necessary, really. But now I'm unsure of the distinction that types are unlinked and instances are linked. Is there even a useful distinction there anymore? All we know is that instances must have values for all link references. |
|
It also might be somewhat confusing to have an |
|
Edited for updates brought up in gitter: So after reading through this and the spec and my code over and over, here's what seems to make sense in my brain. We go back to a single bytecode object (this is where offsets live anyway) and require all links be listed in a mapping with labels. Then these labels could be referred to in an offset mapping. These links can then be one of three types: linked, literal, or reference. Then from these types we can determine the rest of the information as follows: Then, in the spec, we could make a should recommendation that if one is We could additionally require that a bytecode object within a Also, this would mean that both |
Natural language spec update for proposed #123
Hope I'm understanding this correctly, very open to all suggested changes/feedback to improve understanding/clarity.