[Access] Improve script exec compare logging #4936
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4936 +/- ##
=======================================
Coverage 55.77% 55.78%
=======================================
Files 953 953
Lines 88420 88445 +25
=======================================
+ Hits 49317 49336 +19
+ Misses 35398 35394 -4
- Partials 3705 3715 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| execParts := strings.Split(execErr.Error(), "failed to execute script at block") | ||
| localParts := strings.Split(localErr.Error(), "failed to execute script at block") |
There was a problem hiding this comment.
can we make this string into a constant
| localErr error, | ||
| blockID flow.Identifier, | ||
| script []byte, | ||
| arguments [][]byte, | ||
| insecureScriptHash [md5.Size]byte, | ||
| ) { | ||
| // record errors caused by missing local data | ||
| if localErr != nil && status.Code(localErr) == codes.OutOfRange { |
There was a problem hiding this comment.
nit: I wonder if it makes sense to create an error model for this since the logic is getting quite complex, something like: scriptExecutionError and then we could have exeErr.isOutOfRange() and lower exeErr.compare(err) which would encapsulate all messy details of splitting error strings. If you think it's fine this way it's fine with me.
| Str("script", string(script)). | ||
| Hex("script_hash", insecureScriptHash[:]) | ||
| Strs("args", args) |
There was a problem hiding this comment.
is there a case where args might contain some confidential information or secret values? In case you have your own AN deployment and you might not be aware things are getting logged. Just thinking out loud.
|
@sideninja thanks for your comments. I refactored a bit to address them in a separate PR: #4956 |
[Access] Improve script exec compare logging
This PR improves the log messages emitted when comparing script exec between ANs and ENs.