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

tealdbg: use associated group index instead of global #3111

Merged
merged 5 commits into from
Oct 21, 2021

Conversation

barnjamin
Copy link
Contributor

Summary

The groupIndex variable is a global set by a flag and defaults to 0. In the main.go file its checked and then set in DebugParams.

I believe this is a bug that affects any grouped transactions where the groupIndex meant to be evaluated is > 0 and we should really be using the groupIndex passed in the evaluation struct.

Test Plan

Existing tests

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #3111 (a9425ed) into master (c2054e5) will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3111      +/-   ##
==========================================
+ Coverage   43.68%   43.70%   +0.01%     
==========================================
  Files         390      390              
  Lines       86681    86681              
==========================================
+ Hits        37868    37881      +13     
+ Misses      42796    42774      -22     
- Partials     6017     6026       +9     
Impacted Files Coverage Δ
cmd/tealdbg/main.go 25.39% <0.00%> (ø)
cmd/tealdbg/local.go 71.96% <100.00%> (+1.24%) ⬆️
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/blockqueue.go 82.18% <0.00%> (-1.73%) ⬇️
catchup/service.go 69.59% <0.00%> (-1.51%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
data/abi/abi_type.go 92.03% <0.00%> (-1.00%) ⬇️
ledger/internal/eval.go 69.94% <0.00%> (-0.27%) ⬇️
network/wsPeer.go 72.53% <0.00%> (-0.27%) ⬇️
ledger/acctupdates.go 64.75% <0.00%> (-0.10%) ⬇️
... and 5 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 c2054e5...a9425ed. Read the comment docs.

jasonpaulos
jasonpaulos previously approved these changes Oct 20, 2021
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Definitely a bug, thanks for fixing

@barnjamin barnjamin changed the title setting group index to be the one assocaited with the run tealdbg: use associated group index instead of global Oct 21, 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.

Looks good!

@jannotti jannotti merged commit 38b08a0 into algorand:master Oct 21, 2021
@barnjamin barnjamin deleted the tealdbg-group-idx branch October 21, 2021 17:24
cce pushed a commit to cce/go-algorand that referenced this pull request Oct 28, 2021
setting group index to be the one associated with the run
@egieseke egieseke mentioned this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants