-
Notifications
You must be signed in to change notification settings - Fork 81
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
*: natives update #2417
*: natives update #2417
Conversation
Otherwise decoding error may be returned which can be misleading.
Codecov Report
@@ Coverage Diff @@
## master #2417 +/- ##
==========================================
- Coverage 85.14% 85.08% -0.06%
==========================================
Files 290 289 -1
Lines 36139 36099 -40
==========================================
- Hits 30770 30716 -54
- Misses 4088 4097 +9
- Partials 1281 1286 +5
Continue to review full report at Codecov.
|
pkg/core/native/ledger.go
Outdated
if err != nil || !isTraceableBlock(ic.Chain, h) { | ||
return stackitem.Make(vm.NoneState) | ||
} | ||
aer, err := ic.DAO.GetAppExecResults(tx.Hash(), trigger.Application) |
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.
Not very efficient since we have to go to the DB again with the same request.
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.
Yes, that's because (dao *Simple) GetAppExecResults
doesn't return the transaction height. The problem is that (dao *Simple) GetAppExecResults
can return both block and transaction+height, so do we need to split it into two separate methods?
Or maybe it's even better to create some (dao *Simple) GetTxExecResult (uint32, *transaction.Transaction, state.AppExecResult)
method?
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 had an idea for creating it way back when I'd changed the DB format, but there was no real use case. Now it looks like we have it. There are cases where we only need transaction/height and we don't want to spend time on execution result decoding, but if we have to decode everything then it's worth returning everything from some method.
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.
Fixed. Let's first come up to the final decision about GetTxExecResult
, and after that I'll update gomods.
e0df528
to
7b0e750
Compare
7b0e750
to
7811e39
Compare
7811e39
to
cf2593d
Compare
cf2593d
to
10c3db8
Compare
10c3db8
to
d41f711
Compare
d41f711
to
72ec354
Compare
Close #2343, close #2415.