-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat: Simplify op-deployer scripts: Add runtime ABI validation [2/N] #15111
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #15111 +/- ##
===========================================
- Coverage 46.31% 42.30% -4.01%
===========================================
Files 1209 1038 -171
Lines 102317 92144 -10173
===========================================
- Hits 47383 38986 -8397
+ Misses 51566 49968 -1598
+ Partials 3368 3190 -178
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bcc89ee
to
f480193
Compare
039df48
to
849fad3
Compare
e70987a
to
dc2c828
Compare
f656312
to
1fc0a0a
Compare
e5502ed
to
34514c6
Compare
34514c6
to
214d9ab
Compare
} | ||
|
||
// We check for arrays first (i.e. fixed length slices like uint256[2]) | ||
if abiType.T == abi.ArrayTy { |
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.
Could reduce a few lines of code if you use a switch abiType.T
statement or if/else instead of several independent if blocks.
if test.err == "" { | ||
require.NoError(t, err) | ||
} else { | ||
require.EqualError(t, err, test.err) |
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.
Could you use require.ErrorIs
to compare to custom error instance instead of require.EqualError
? Would be less brittle than the current string matching approach
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.
Okay I'll change, I used EqualError
since the errors are wrapped using fmt.Errorf
and the other assertions kept failing on the wrapping part - since I am mostly interested in the message I went with the string approach
Description
ABI runtime validation utilities to be used when loading deploy scripts:
matchTypes
ensures that a given ABI type matches a Go typematchArguments
ensures that a list of ABI arguments (function params or return values) matches Go typesThese validators will be executed when a script ABI is loaded in runtime and will halt any execution if the Go types do not match the loaded ABI.
Unfortunately, this kind of validation is not provided out of the box by
go-ethereum
.Considered alternatives
A notable alternative is to use
Pack
andUnpack
methods to validate the ABI <-> Go type mapping. Unfortunately, this means that besides the struct types, the consumer of this code would need to provide two additional values per every ABI method to be validated - an example input and an example output.Besides the additional code required, the validation would only be correct if all the example fields provided had different values. This is due to a quirk of the ABI decoding - the fields will be decoded in the order of definition and their names might not match. As an example:
Using these two as an example, the ABI decoder would place the solidity value of
fieldA
intoFieldB
of the go struct. If this was not the case (which it might not be after we pull upstream), we could simplify this validation code to minimum since this peculiarity is the only reason this code has been introduced.Relates to #14976