-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Allow adding compilations to an existing ProjectDecoder
#5646
Conversation
There was a problem hiding this 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 😅
d54a307
to
7057cce
Compare
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. |
There was a problem hiding this 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?
There was a problem hiding this 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.
packages/decoder/lib/decoders.ts
Outdated
for (let i = 0; i < compilations.length; i++) { | ||
for (let j = i + 1; j < compilations.length; j++) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this 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
This PR addresses #5602 by making it possible to add compilations to an already existing
ProjectDecoder
. (It doesn't do this forContractDecoder
orContractInstanceDecoder
.) To do this, it adds two methods toProjectDecoder
, namely,addCompilations
andaddAdditionalProjectInfo
.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 theProjectDecoder
a new state variable which serves as a nonce to be incorporated into the ID and which is incremented each time you calladdAdditionalProjectInfo
. 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 asObject.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.