Skip to content
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

Extended logic eval error #2975

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Conversation

algorandskiy
Copy link
Contributor

Summary

Return a bit more details on eval failure for app calls.

Test Plan

Added EvalContext.PcDetails() method and tests.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

Codecov Report

Merging #2975 (b351f0a) into master (734e727) will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2975      +/-   ##
==========================================
+ Coverage   47.61%   47.62%   +0.01%     
==========================================
  Files         358      358              
  Lines       57906    57928      +22     
==========================================
+ Hits        27570    27589      +19     
- Misses      27208    27212       +4     
+ Partials     3128     3127       -1     
Impacted Files Coverage Δ
ledger/ledgercore/error.go 0.00% <0.00%> (ø)
data/transactions/logic/eval.go 85.70% <86.66%> (+<0.01%) ⬆️
ledger/appcow.go 82.16% <100.00%> (+0.21%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (-1.73%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
ledger/acctupdates.go 62.94% <0.00%> (-0.34%) ⬇️
network/wsNetwork.go 61.09% <0.00%> (ø)
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
util/metrics/counter.go 80.95% <0.00%> (+2.38%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 734e727...b351f0a. Read the comment docs.

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the pc is set to the end or maybe even 1 past the end for errors that don't get detected until after the first pass (when jumps are resolved). Could you put in a test to show things work ok when PcDetails is called then?

jannotti
jannotti previously approved these changes Sep 30, 2021
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty clear you would just get an empty string, so that seems ok.

@algorandskiy
Copy link
Contributor Author

Could you put in a test to show things work ok when PcDetails is called then?
added a test

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@tsachiherman tsachiherman merged commit fbd75f7 into algorand:master Sep 30, 2021
@barnjamin barnjamin mentioned this pull request Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants