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
Report transaction computation usage #667
Conversation
@@ -163,6 +163,9 @@ func (i *TransactionInvocator) Process( | |||
txError = NewTransactionStorageLimiter().Process(vm, ctx, proc, sth, programs) | |||
} | |||
|
|||
proc.Logs = append(proc.Logs, env.getLogs()...) |
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.
Are we sure we want to drop logs if tx fails? Aren't they useful for debugging?
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.
I thought the same thing, that's why I moved it to the above error check step. for transactions, this will be disabled anyway.
The broken test is actually due to this (onflow/cadence#895) |
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.
LGTM!
Codecov Report
@@ Coverage Diff @@
## master #667 +/- ##
==========================================
+ Coverage 56.42% 56.45% +0.02%
==========================================
Files 427 427
Lines 24975 24976 +1
==========================================
+ Hits 14092 14099 +7
+ Misses 8977 8974 -3
+ Partials 1906 1903 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sorry for my super late review, I didn't see this before |
This PR make sure the actual computation used by a transaction is captured by the transaction result.