Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bytecode object in docs #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

njgheorghita
Copy link
Contributor

Natural language spec update for proposed #123

Hope I'm understanding this correctly, very open to all suggested changes/feedback to improve understanding/clarity.

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the write-up here, this is exactly what I was attempting to convey :)

@pipermerriam
Copy link
Member

this is great, because now I can see what I think is a problem... As it stands:

  • contract instances: linked bytecode
  • contract type: unlinked bytecode

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?

Link References: ``link_references``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/should/must/

@gnidan
Copy link
Contributor

gnidan commented Aug 24, 2018

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.

I'd say that if a package doesn't want to expose already-linked types, it can just say link_references: []. This doesn't change the underlying unlinkedness of the object, it just means that a particular piece of bytecode doesn't have any public link references.

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.

@njgheorghita
Copy link
Contributor Author

I'd say that if a package doesn't want to expose already-linked types, it can just say link_references: [].

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?

@gnidan
Copy link
Contributor

gnidan commented Aug 24, 2018

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.

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ContractType that 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.

@njgheorghita
Copy link
Contributor Author

@pipermerriam Added link_dependencies option to UnlinkedBytecodeObject. @gnidan I'd appreciate a final look over to make sure everything looks good

@gnidan
Copy link
Contributor

gnidan commented Aug 24, 2018

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.

@gnidan
Copy link
Contributor

gnidan commented Aug 24, 2018

Another thing: what if a contract only wants to pre-link some of its link refs? So then link_dependencies would have a subset of values and not have the same array length as link_references.

@njgheorghita
Copy link
Contributor Author

Feels weird to convert the linked bytecode value to 0s.

Can we just create a new "type" of link value?

@njgheorghita
Copy link
Contributor Author

Is it a requirement that link_dependencies and link_references are the same length? As long as all link_dependencies reference a link_reference, it should be fine to have link_references that don't reference a link_dependencies? 🤷‍♂️

@gnidan
Copy link
Contributor

gnidan commented Aug 24, 2018

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.

@njgheorghita
Copy link
Contributor Author

It also might be somewhat confusing to have an UnlinkedBytecodeObject that can actually be (partially) linked in certain cases. A better name could help though

@cbdotguru
Copy link

cbdotguru commented Sep 1, 2018

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:

bytecode_object: {
  bytecode: [string always required],
  links: {
    [int defining offset]: [string as a link label]
  },
  link_objects: {
     [link label string]: {
        type: ['linked','literal','reference'],
        // if 'linked' then value is a name, if 'literal' then value is the literal string, if 'reference' then value is the reference name
        value: [required string], 
        length: [required int if type is 'linked'],
      } 
    }
  }
}

Then, in the spec, we could make a should recommendation that if one is linked then all should be linked.. OR we could just make it a must. So we have a single polymorphic object, and we reduce redundancy and complexity I think, then our core tool could enforce that if you're providing a linked bytecode, then we're enforcing it to be complete.

We could additionally require that a bytecode object within a contract_instance have only linked bytecode. Then contract_type’s could be either/or.

Also, this would mean that both linked and reference use the name to a contract object, it’s just that if type is linked the bytecode has an embedded link and if it’s reference the bytecode has 0’s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants