Wrap host env errors with external errors #2683
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SupunS
force-pushed
the
supun/handle-external-errors
branch
from
43759e4
to
abb7402
Compare
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 181924e Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #2683 +/- ##
==========================================
- Coverage 78.57% 78.57% -0.01%
==========================================
Files 338 338
Lines 78216 78237 +21
==========================================
+ Hits 61456 61472 +16
- Misses 14474 14476 +2
- Partials 2286 2289 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
SupunS
force-pushed
the
supun/handle-external-errors
branch
from
b2cd9a8
to
fe2ba63
Compare
dsainati1
approved these changes
Jul 31, 2023
turbolent
reviewed
Jul 31, 2023
There was a problem hiding this comment.
The convention for runtime interface functions is that:
- Errors returned from the function are user errors
- Panics are internal errors (e.g. implementation bug)
For example, when a requested block does not exist, the implementation of GetBlockAtHeight can return an error indicating that.
Why do we wrap paniced errors (internal errors) as external errors? To differentiate that they did not originate in Cadence itself?
Why would we wrap returned errors, i.e. classify user errors, as external errors?
Both questions probably stem from my lack of understanding what external errors are used/needed for.
So the problem is this convention is not always followed (and is not guaranteed to be followed either). There can be situations where certain user errors are also panicked, while internal errors are returned. This is a really grey area, sometimes a 'user error' to the host-env, etc. could actually be wrong information being sent by Cadence such that it's actually an internal error for the user.
Cadence doesn't know if a returned/thrown error is actually an internal error or a user error (unless it was originated from Cadence). So there's this "external error" category where it just says it's an error that came from outside and Cadence can't determine which one it is. |
|
The real problem I think is, in the cadence code base, we use panics to propagate errors (rather than returning). So any un-categorized error creates a problem at the top of the call tree where all errors are handled. |
|
@SupunS Thank you for the great explanation, that makes sense! |
turbolent
approved these changes
Jul 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2682
Description
Wrap the errors returned from host-env using the external error.
This is already done for the errors panicked from host-env.
masterbranchFiles changedin the Github PR explorer