-
Notifications
You must be signed in to change notification settings - Fork 140
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
Extended debug #451
Extended debug #451
Conversation
@m4ksio is this supposed to be released in the v0.9 release line? Why/how are the first three commits by me part of this? |
runtime/errors.go
Outdated
|
||
// ExtendedParsingCheckingError is a special error which aids in debugging checking problems | ||
// by providing extra information about the state of the environment | ||
// Separate package to prevent cyclic imports with checker tests |
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.
Not quite sure I understand this line, can you elaborate?
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.
Does this clarify cyclic imports enough? https://github.com/onflow/cadence/pull/451/files#diff-74ffa81b6938856f695ba112a721503dfc5ff2e852bc65d79d15ee4868f4bb97R4784-R4791
I branched off |
@m4ksio Can you please create a v0.9.3 branch and target this PR against it? |
runtime/errors.go
Outdated
RuntimeStorage *InterpreterRuntimeStorage | ||
Functions stdlib.StandardLibraryFunctions |
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.
How are these used for debugging? The fields in interpreterRuntimeStorage
are also not exported, so is exposing the storage here (and exporting interpreterRuntimeStorage
as InterpreterRuntimeStorage
below) useful?
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.
How are these used for debugging?
They are dumped to a file for further usage. I didn't analyze what was particularly useful, just grabbed all there is
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.
Given that the runtime storage value doesn't provide any info, OK if I remove it?
Or we should maybe export the storage's cache, which could be useful to look at for execution errors.
Also, the functions that are injected are always the same, so IDK how useful it it is to include them in the error.
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.
Given that the runtime storage value doesn't provide any info, OK if I remove it?
Feel free if you think it doesn't provide any value, and add what you think it's best - you are the best person to know what internal elements might be helpful
@m4ksio Pushed some improvements, feel free to convert the draft into a proper PR |
@m4ksio What does the linked issue (1291) refer to? The issue in dapperlabs/flow-go seems unrelated |
|
Part of changes to support dapperlabs/flow-go#1291
It's not the most elegant solution, but I assume this code is temporary anyway.
It supports returning extended information on case of execution errors so they can be recorded by the caller - currently if TopShot contract fails.
It was started from v0.9.2 branch as it was the latest one we use in Flow-go - I plan to update this PR accordingly once more mature version of Cadence is used in Flow-go