[EVM] Handle EVM errors #5216

devbugging · 2024-01-09T15:47:36Z

Closes: #5149

This PR adds a type of EVMError that satisfies the UserError which is handled by Cadence at the same time embedding the CodedError.

The error output now looks like:

[Error Code: 1300] error caused by: 1 error occurred:
	* [Error Code: 1101] cadence runtime error: Execution failed:
 --> ff292f561c9cba05ef08abd74bc90cbaca1c217f737312d00b8fcf7ccafc405d:8:12
  |
8 | 				destroy acc.withdraw(balance: bal);
  | 				        ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: [Error Code: 1300] evm runtime error: insufficient total supply
  --> 8c5303eaa26202d6.EVM:89:12
   |
89 |             let vault <- InternalEVM.withdraw(
90 |                 from: self.addressBytes,
91 |                 amount: balance.flow
92 |             ) as! @FlowToken.Vault
   |             ^^^^^^^^^^^^^^^^^^^^^^

janezpodhostnik

Nice!

ramtinms · 2024-01-09T23:51:51Z

fvm/errors/errors.go

+
+func (e EVMError) IsUserError() {}


So is this the missing part ?

Basically, this makes it conform to the type of Cadence User Error which doesn't contain the stack and is handled gracefully.
Now we could maybe even go further and adding an EVM error type in Cadence handling which I think could be even nicer way to go, but this solves it for now and we can add it to the todo if you think so?

ramtinms · 2024-01-09T23:53:10Z

I think we need 3 tests

one for when a fatal error is happening down there
one for when a stateDB error is happening (non-fatal)
one for when a evm tx is failed

devbugging · 2024-01-10T13:17:08Z

I think we need 3 tests

one for when a fatal error is happening down there

one for when a stateDB error is happening (non-fatal)

one for when a evm tx is failed

Ok, will add these tests as well.

devbugging · 2024-01-10T13:23:26Z

happening down there

In the basement where the EVM is 😆

codecov-commenter · 2024-01-10T15:17:27Z

Codecov Report

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

Comparison is base (c1fdaa8) 56.55% compared to head (e2c455f) 56.35%.
Report is 69 commits behind head on master.

Files	Patch %	Lines
fvm/errors/errors.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5216      +/-   ##
==========================================
- Coverage   56.55%   56.35%   -0.21%     
==========================================
  Files         988      994       +6     
  Lines       94422    94810     +388     
==========================================
+ Hits        53403    53427      +24     
- Misses      37054    37408     +354     
- Partials     3965     3975      +10

Flag	Coverage Δ
unittests	`56.35% <0.00%> (-0.21%)`	⬇️

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.

ramtinms · 2024-01-10T19:01:05Z

fvm/fvm_test.go

+// evmErrorSnapshot will return an error once it gets to reading the EVM
+// registers for address allocator, that way we can at that point return a provided
+// error, that error type is then checked against the provider checker.
+type evmErrorSnapshot struct {
+	snapshotTree snapshot.SnapshotTree
+	err          error
+	errChecker   func(error) bool
+}
+
+func (s evmErrorSnapshot) Get(
+	id flow.RegisterID,
+) (
+	flow.RegisterValue,
+	error,
+) {
+	if id.Key == "AddressAllocator" {
+		return nil, s.err
+	}
+
+	return s.snapshotTree.Get(id)


hmm, I guess there should be a MockedSnapshot somewhere either created by mockery or implemented with func methods, If it doesn't exist we should create it in my view, that way we have more clear tests that are not dependent on a logic outside of the test.

@janezpodhostnik do you know if we have one ?

I agree on the approach. I've checked before for existing ones but only found, one in accounts that is just an implementation for tests here

flow-go/fvm/accounts_test.go

Line 22 in f17e073

type errorOnAddressSnapshotWrapper struct {

and one in util

flow-go/cmd/util/ledger/util/util.go

Line 67 in 0596ab0

type PayloadSnapshot struct {

So will generate one with mockery for now.

@ramtinms fixed

turbolent

Nice!

Gregor Gololicic added 5 commits January 9, 2024 15:00

comment typo

e7ffe88

fix comments

90700a8

add test to check for evm error

f4ee5de

improve test to check whether error is not internal

86d48b6

change evm error is it satisfies the user error and embeds coded error

998b3cc

devbugging requested review from ramtinms and janezpodhostnik as code owners January 9, 2024 15:47

devbugging self-assigned this Jan 9, 2024

Gregor Gololicic added 2 commits January 9, 2024 16:48

remove unneded workaround

a8e4edd

remove unneded workaround

398986d

janezpodhostnik approved these changes Jan 9, 2024

View reviewed changes

ramtinms reviewed Jan 9, 2024

View reviewed changes

Gregor Gololicic added 2 commits January 10, 2024 16:08

add tests for internal evm errors

e5511da

fix format

5c3d62f

ramtinms reviewed Jan 10, 2024

View reviewed changes

Gregor Gololicic added 4 commits January 11, 2024 13:53

add mock generation for storage snapshot

127021c

add mock for storage snapshot

db33d33

fix the tests to use mocks

6119064

import lint

e2c455f

turbolent approved these changes Jan 11, 2024

View reviewed changes

ramtinms approved these changes Jan 11, 2024

View reviewed changes

devbugging added this pull request to the merge queue Jan 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 11, 2024

devbugging added this pull request to the merge queue Jan 12, 2024

Merged via the queue into master with commit 6c6a7f6 Jan 12, 2024
51 checks passed

devbugging deleted the gregor/evm/runtime-error-handling branch January 12, 2024 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EVM] Handle EVM errors #5216

[EVM] Handle EVM errors #5216

devbugging commented Jan 9, 2024 •

edited

Loading

janezpodhostnik left a comment

ramtinms Jan 9, 2024

devbugging Jan 10, 2024

ramtinms commented Jan 9, 2024

devbugging commented Jan 10, 2024

devbugging commented Jan 10, 2024 •

edited

Loading

codecov-commenter commented Jan 10, 2024 •

edited

Loading

ramtinms Jan 10, 2024

ramtinms Jan 10, 2024

devbugging Jan 11, 2024 •

edited

Loading

devbugging Jan 11, 2024

turbolent left a comment

[EVM] Handle EVM errors #5216

[EVM] Handle EVM errors #5216

Conversation

devbugging commented Jan 9, 2024 • edited Loading

janezpodhostnik left a comment

Choose a reason for hiding this comment

ramtinms Jan 9, 2024

Choose a reason for hiding this comment

devbugging Jan 10, 2024

Choose a reason for hiding this comment

ramtinms commented Jan 9, 2024

devbugging commented Jan 10, 2024

devbugging commented Jan 10, 2024 • edited Loading

codecov-commenter commented Jan 10, 2024 • edited Loading

Codecov Report

ramtinms Jan 10, 2024

Choose a reason for hiding this comment

ramtinms Jan 10, 2024

Choose a reason for hiding this comment

devbugging Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

devbugging Jan 11, 2024

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

devbugging commented Jan 9, 2024 •

edited

Loading

devbugging commented Jan 10, 2024 •

edited

Loading

codecov-commenter commented Jan 10, 2024 •

edited

Loading

devbugging Jan 11, 2024 •

edited

Loading