Skip to content
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

[FVM] Handle cadence ParentErrors #5272

Merged
merged 17 commits into from
Jan 29, 2024

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented Jan 23, 2024

Cadence errors.ParentErrors contain a list of errors encountered during execution:
https://github.com/onflow/cadence/blob/5ce1f36f95bb458f05d03283c0af307ff7f6fe1e/runtime/errors/errors.go#L101-L104

These errors have an Unwrap() method that returns an []error instead of a plain error. This results in a tree of errors instead of a chain. While this is supported by the standard errors.Is and errors.As methods, the CodedError type is overloaded for use by both normal errors and failures. This means that if there is a failure mixed with other errors, the failure could be missed since searching would stop at the first instance of a CodedError.

This PR refactors the FVM CodedError to add a new type for failures: CodedFailure. This allows errors.As to find any instance of a failure within the error tree without custom logic.

fvm/errors/errors.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (ed23d21) 55.59% compared to head (8a9af22) 55.62%.

Files Patch % Lines
fvm/errors/errors.go 92.50% 5 Missing and 1 partial ⚠️
fvm/errors/execution.go 33.33% 4 Missing ⚠️
fvm/errors/failures.go 69.23% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5272      +/-   ##
==========================================
+ Coverage   55.59%   55.62%   +0.03%     
==========================================
  Files        1002     1002              
  Lines       96658    96708      +50     
==========================================
+ Hits        53733    53795      +62     
+ Misses      38862    38845      -17     
- Partials     4063     4068       +5     
Flag Coverage Δ
unittests 55.62% <86.66%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fvm/errors/errors.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
@@ -61,3 +65,107 @@ func TestErrorHandling(t *testing.T) {
require.True(t, IsFailure(e1))
})
}

func TestHandleRuntimeError(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

fvm/errors/errors_test.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
fvm/errors/errors.go Outdated Show resolved Hide resolved
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
@ramtinms
Copy link
Contributor

I have to spend some time reviewing how the new error handling (coded) works on the FVM side before reviewing this, but before that out of curiosity why not fixing this on the cadence runtime?

fvm/errors/errors.go Outdated Show resolved Hide resolved
@peterargue
Copy link
Contributor Author

peterargue commented Jan 26, 2024

I have to spend some time reviewing how the new error handling (coded) works on the FVM side before reviewing this, but before that out of curiosity why not fixing this on the cadence runtime?

So there were 2 issues addressed here:

  1. Some cadence errors contain a list of child errors and don't implement an Unwrap() method, so we need to handle them explicitly. This should be fixed in cadence. I tried to make the handler code in this PR lightweight and easy to remove later. (Update: this is fixed in cadence now, so I removed those bits)
  2. Given some errors can have a list of child errors, there is an error tree instead of an error chain. The logic we have today expects a chain and would not find Failures if they were in another branch from a different CodedError.

for example, in the below case the existing code would only see CheckerError. After cadence is updated to unwrap these, the existing FVM code would only find someFVMError, but miss someFVMFailure since As and Is stop searching at the first instance of an error.

runtime.Error
└── sema.CheckerError
    ├── someCadenceError
    ├── someFVMError
    └── someFVMFailure

By separating CodedError and CodedFailure types, the stdlib's methods can now find any CodedFailure in the tree.

@turbolent
Copy link
Member

I don't know enough about FVM, so can't really review the PR, but in general, thank you for cleaning up and improving the categorization of errors 🙏

As far as I understand, "errors" are implementation errors (e.g. file can't be opened) and "failures" are user errors (e.g. the user ran out of computation units)?

@peterargue
Copy link
Contributor Author

peterargue commented Jan 29, 2024

As far as I understand, "errors" are implementation errors (e.g. file can't be opened) and "failures" are user errors (e.g. the user ran out of computation units)?

It's the opposite. "failures" are exceptions, and "errors" are recoverable errors.

@peterargue peterargue self-assigned this Jan 29, 2024
@peterargue peterargue added this pull request to the merge queue Jan 29, 2024
Merged via the queue into master with commit c665488 Jan 29, 2024
51 checks passed
@peterargue peterargue deleted the petera/handle-cadence-parent-errors-fvm branch January 29, 2024 19:57
peterargue added a commit that referenced this pull request Jan 29, 2024
…ors-fvm

[FVM] Handle cadence ParentErrors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants