Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Allow adding compilations to an existing ProjectDecoder #5646

Merged
merged 10 commits into from
Nov 15, 2022

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Oct 26, 2022

This PR addresses #5602 by making it possible to add compilations to an already existing ProjectDecoder. (It doesn't do this for ContractDecoder or ContractInstanceDecoder.) To do this, it adds two methods to ProjectDecoder, namely, addCompilations and addAdditionalProjectInfo.

The addCompilations interface assumes you are adding codec-style compilations. It also assumes that none of these has an ID that collides with the IDs of any of the compilations it already knows about. It's on the user to ensure that.

The addAdditionalProjectInfo interface is more general and allows also adding compile-common style compilations, or just a list of artifacts. If you use it to add codec-style compilations, it's still up to you to avoid ID collisions. If you use compile-common style compilations, or just a list of artifacts, then an ID will be assigned; to prevent collisions, I've given the ProjectDecoder a new state variable which serves as a nonce to be incorporated into the ID and which is incremented each time you call addAdditionalProjectInfo. I'm not sure how truly robust this system is, but it shouldn't encounter collisions in normal use at least, I figure.

Anyway, how does addCompilations work under the hood? Well, it's mostly pretty simple. It takes the new compilations and merges them with the existing ones. Then it performs the various decoder startup computations on the new compilations, and merges the results with what it already has. Usually the form of the merge is very simple, just [...old, ...new] or { ...old, ...new} (actually those latter cases are done as Object.assign(old, new) since we intend to alter things here!). There is one case where it's slightly more complicated, which is calldata allocations, but it's still pretty simple.

There were two cases, however -- returndata allocations and event allocations -- where correctly performing a merge on the results would have been quite difficult. So to handle those, instead of running the allocation computations on just the new compilations and then merging the results, it instead reruns the relevant computations on the whole set of both old and new compilations. Oh well, it'll do I figure.

And that's it! This PR also adds a quick test of this new capability.

Copy link
Contributor

@cliffoo cliffoo left a comment

Choose a reason for hiding this comment

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

YES, thanks @haltman-at ! addAdditionalProjectInfo looks perfect for dashboard's needs. I understand your point about merging / recomputing different parts of the decoder state. I don't know enough about codec stuff to know if all the utils functions are called and handled correctly inside addCompilations, I guess I'll wait for someone else to take a proper look at it 😅

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
…where complex

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
@haltman-at haltman-at force-pushed the add-compilations-simple branch from d54a307 to 7057cce Compare October 27, 2022 23:58
@haltman-at
Copy link
Contributor Author

OK, so (from talking to @gnidan, although it was apparently @cliffoo who first reported it) it seems that there was a problem where the decoder preferred to match bytecode to older contracts (in terms of the order they were added in) rather than newer ones. Ideally we'd fix this with some sort of timestamp system, but that would be a bunch of work (in that it would probably require changing a whole bunch of stuff internally). So as a quick-and-dirty fix, I've just reversed the order of assignment here. That said, we probably should add some sort of proper timestamp system in the future. I guess I'll put up an issue for that after this is merged.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

I have a question about the developer experience. What happens in the event a user provides an ID that already exists, will it be non-trivial to determine the cause?

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
@haltman-at haltman-at requested a review from cds-amal November 11, 2022 03:46

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

I have a couple of questions.

Comment on lines 172 to 173
for (let i = 0; i < compilations.length; i++) {
for (let j = i + 1; j < compilations.length; j++) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, they're expected to be in order! Can we assume the client breaks this guarantee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're not expected to be in order. Why do you say they are?

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me, pending CI

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

Successfully merging this pull request may close these issues.

None yet

3 participants