-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Enable stack traces in the EDR provider #6214
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@galargh as I changed how the solc inputs are generated, some build ids changed, and that broke a test. Can you check if my fix is correct? |
51fc3f0
to
0e77fd4
Compare
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.
Looks good, but left a question regarding performance.
} catch (error) { | ||
assert.ok(error instanceof Error, "Expected an error to be thrown"); | ||
|
||
assert.ok( |
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.
no need to change it, but there's an assert.match that better captures this type of assertion.
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! I looked for something with "includes" semantics and couldn't find any 😅
@@ -204,6 +214,10 @@ export class NetworkManagerImplementation implements NetworkManager { | |||
chainType: resolvedChainType as ChainType, | |||
}, | |||
jsonRpcRequestWrapper, | |||
tracingConfig: { |
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.
have you seen any performance hit after adding this? I thought it would be disabled by default and enabled through config.
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.
It's meant to be enabled by default, so if there's any performance issue we should address it.
Right now it will have a small performance problem, which is being address in EDR. Note that we are parsing and transforming the build infos, just to pass it to EDR.
In the near future we'll just pass the Uint8Array
read from the files, directly.
0e77fd4
to
c40d264
Compare
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.
While cleaning up the
ArtifactsManager
interface in #6213 I noticed that we may have a misalignment between the build info formats that EDR was expecting and the ones we generated, so I took a stab at this.This PR those a few things:
node-types
, as for some reason EDR is using someany
s.While this closes #5981, there's going to be a bit of follow up work.
@ignored/edr
, we should do something similar for the solidity tests. Except without transforming the build infos into HH2 build infos.@ignored/edr-optimism
, we should stop parsing and transforming the build infos in theNetworkManager
.