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

Simulate: Add State Change to Exec Trace #5659

Merged
merged 19 commits into from Aug 18, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Aug 13, 2023

Summary

This PR implements #5015, which exposes app state (touches) changes in exec trace.

Test Plan

2 simulation eval test on boxes and app-local/global state.

1 rest client test on app-local/global state.

@ahangsu ahangsu self-assigned this Aug 13, 2023
@ahangsu ahangsu force-pushed the simulate-state-change branch 2 times, most recently from 6f47cb8 to ee27164 Compare August 13, 2023 02:07
@codecov
Copy link

codecov bot commented Aug 13, 2023

Codecov Report

Merging #5659 (3d64fce) into master (f8bc8c6) will decrease coverage by 0.03%.
The diff coverage is 21.70%.

@@            Coverage Diff             @@
##           master    #5659      +/-   ##
==========================================
- Coverage   55.18%   55.16%   -0.03%     
==========================================
  Files         465      466       +1     
  Lines       64989    65075      +86     
==========================================
+ Hits        35866    35896      +30     
- Misses      26729    26784      +55     
- Partials     2394     2395       +1     
Files Changed Coverage Δ
daemon/algod/api/server/v2/utils.go 7.61% <0.00%> (+0.18%) ⬆️
data/transactions/logic/opcodeExplain.go 0.00% <0.00%> (ø)
ledger/simulation/trace.go 76.92% <16.66%> (-6.13%) ⬇️
cmd/goal/clerk.go 9.33% <33.33%> (+0.08%) ⬆️
daemon/algod/api/server/v2/account.go 77.84% <100.00%> (ø)
data/transactions/logic/opcodes.go 86.77% <100.00%> (+18.68%) ⬆️
ledger/simulation/tracer.go 95.93% <100.00%> (+0.37%) ⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Mostly just naming thoughts so far.

cmd/goal/clerk.go Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/utils.go Outdated Show resolved Hide resolved
data/transactions/logic/opcodeExplain.go Show resolved Hide resolved
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.

I will take a closer look at testing tomorrow

daemon/algod/api/algod.oas2.json Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
ledger/simulation/trace.go Outdated Show resolved Hide resolved
ledger/simulation/trace.go Show resolved Hide resolved
data/transactions/logic/opcodeExplain.go Outdated Show resolved Hide resolved
data/transactions/logic/opcodeExplain.go Outdated Show resolved Hide resolved
data/transactions/logic/opcodeExplain.go Outdated Show resolved Hide resolved
@ahangsu ahangsu requested a review from jannotti August 16, 2023 00:49
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 good. I made some description language suggestions.

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas3.yml Outdated Show resolved Hide resolved
jannotti
jannotti previously approved these changes Aug 17, 2023
jasonpaulos
jasonpaulos previously approved these changes Aug 17, 2023
@ahangsu
Copy link
Contributor Author

ahangsu commented Aug 17, 2023

Guys, hold off a bit! Wait until #5658 merge in, for we need to extend some testcases from there

algorandskiy
algorandskiy previously approved these changes Aug 17, 2023
@algorandskiy
Copy link
Contributor

please re-merge the master

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.

None yet

4 participants