Basic support for viaIR #3205
Basic support for viaIR #3205
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 92ac52b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| } | ||
|
|
||
| return latestInstructionRevertFrame; |
There was a problem hiding this comment.
So now this function just falls off and returns undefined? i had to look up and confirm that that's what js does. Maybe use an explicit return undefined?
There was a problem hiding this comment.
Yes, that's what's happening. We could add an explicit return or return undefined, but I think the idiomatic thing (or at least what we normally do in the rest of the codebase) is to not do anything.
|
|
||
| if (sourceReference !== undefined) { | ||
| return sourceReference; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not following what the change here is. What am I missing? sourceLocationToSourceReference() can return undefined, and we were passing that along before, and now we're making sure not to return its undefined return value from this function, right? but before, it was just falling through to the return undefined; 2 lines later, right? so does this commit have any "functional" change?
There was a problem hiding this comment.
I think the previous version of the code was making this assumption: if a step has a location, then we can get a source reference from that location. viaIR seems to break that assumption. What this change is saying is "if you can't get a source reference from this location, then keep looking at the rest of the steps".
For the record 1: this did fix a failing test in OZ, so I'm pretty sure it has a functional change.
For the record 2: this was a very instinctive change, I don't understand in depth why that assumption broke with viaIR.
| return this._getCallMessageStackTrace(maybeDecodedMessageTrace); | ||
| } | ||
|
|
||
| log("Trace is not decoded"); |
There was a problem hiding this comment.
should this message not also convey "trace is not a precompile" and "trace does not have an error object"? why not log("Trace message not recognized") like the function name?
There was a problem hiding this comment.
I don't have a strong preference. "Decoded" in this context has (at least to me) a clear meaning, about the trace not having extra information added by Hardhat. I don't mind changing it though, maybe to "Trace is not a precompile nor decoded" if you think that's better.
packages/hardhat-core/src/internal/hardhat-network/stack-traces/error-inferrer.ts
Outdated
Show resolved
Hide resolved
|
Actually, I did have a question about testing this. How did you? I didn't notice anything in here. Do any of our fixture projects already enable |
…s/error-inferrer.ts Co-authored-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>
|
Closing in favor of #3415. |
Closes #2724.