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

Algod: Simulation PC exposure for App approval/clear-state transaction #5360

Merged
merged 83 commits into from Jun 1, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented May 4, 2023

Summary

This PR implements #5012, which exposes PC during simulation for each txn-group.

Test Plan

  1. simulation_eval_test.go:
    a. recursive inner calls that creates app, calls app and deletes itself.
    b. app-call transaction approved through a logic sig.
    c. a vanilla app-call transaction together with extra budget supply.
  2. restClient.go:
    a. an e2e test that tests recursive inner calls that creates app, calls app and deletes itself. See if trace work as expected

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #5360 (d392f99) into master (a10efe5) will increase coverage by 0.01%.
The diff coverage is 58.59%.

@@            Coverage Diff             @@
##           master    #5360      +/-   ##
==========================================
+ Coverage   55.31%   55.32%   +0.01%     
==========================================
  Files         452      452              
  Lines       63844    63936      +92     
==========================================
+ Hits        35313    35372      +59     
- Misses      26101    26132      +31     
- Partials     2430     2432       +2     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.82% <ø> (ø)
daemon/algod/api/server/v2/utils.go 10.42% <0.00%> (-1.42%) ⬇️
data/transactions/logic/eval.go 91.24% <0.00%> (-0.03%) ⬇️
node/node.go 4.04% <0.00%> (ø)
cmd/goal/clerk.go 8.88% <16.66%> (+0.05%) ⬆️
ledger/simulation/trace.go 84.44% <46.15%> (-5.80%) ⬇️
ledger/simulation/simulator.go 75.55% <83.33%> (-1.72%) ⬇️
ledger/simulation/tracer.go 92.25% <96.61%> (-1.37%) ⬇️
daemon/algod/api/server/v2/test/helpers.go 74.74% <100.00%> (ø)

... and 16 files with indirect coverage changes

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

@ahangsu ahangsu force-pushed the simulate-app-pc-expose branch 2 times, most recently from b820620 to 084a701 Compare May 6, 2023 01:32
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.

This is looking quite close. I mostly have some concerns about naming, and whether we can simplify the interface between "internal" structs and models. I may well be wrong about how pointers are handled in serialization. I think @jasonpaulos is a bit of an expert here.

cmd/goal/clerk.go Outdated Show resolved Hide resolved
cmd/goal/clerk.go Outdated Show resolved Hide resolved
ledger/simulation/simulator.go Outdated Show resolved Hide resolved
ledger/simulation/trace.go Outdated Show resolved Hide resolved
ledger/simulation/trace.go Outdated Show resolved Hide resolved
@ahangsu ahangsu requested a review from jannotti May 26, 2023 20:30
jannotti
jannotti previously approved these changes May 30, 2023
ledger/simulation/tracer.go Outdated Show resolved Hide resolved
test/e2e-go/restAPI/restClient_test.go Outdated Show resolved Hide resolved
ledger/simulation/simulation_eval_test.go Outdated 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.

Pending tests, looks good

@ahangsu ahangsu requested a review from jannotti May 31, 2023 14:17
@bbroder-algo bbroder-algo merged commit bad81ee into algorand:master Jun 1, 2023
24 checks passed
@ahangsu ahangsu deleted the simulate-app-pc-expose branch June 1, 2023 18:11
tzaffi pushed a commit to tzaffi/go-algorand that referenced this pull request Jun 1, 2023
algorand#5360)

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
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