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

Dryrun: Split dryrun cost field into BudgetConsumed and BudgetAdded #3957

Merged
merged 20 commits into from
May 26, 2022

Conversation

algoidurovic
Copy link
Contributor

@algoidurovic algoidurovic commented May 6, 2022

Summary

Simple fix to allow reporting of budget increases, which reduce the net cost of the execution of an app call. This is needed for testing the use of inner app calls to increase the available budget.

The codegen infrastructure around the API spec has poor support for signed integers so the problem can't be solved by simply returning a negative net cost. To address this, the cost field is replaced by two unsigned integer fields representing the overall budget increase and decrease: net cost = BudgetConsumed - BudgetAdded.

Test Plan

Updated unit test and e2e test to enforce the correct execution cost increase/decrease is reported. The unit test exercises the BudgetAdded field by using inner app calls.

@algoidurovic algoidurovic requested a review from barnjamin May 6, 2022 19:37
@algoidurovic algoidurovic changed the title Change dryrun cost field to signed int from unsigned in Dryrun: Change dryrun cost field to signed int from unsigned in May 6, 2022
itxn_field TypeEnum
int DeleteApplication
itxn_field OnCompletion
byte 0x068101
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a comment inline here right? if so can you please just add #pragma version 6; int 1;

@algoidurovic algoidurovic requested a review from jannotti May 6, 2022 19:51
barnjamin
barnjamin previously approved these changes May 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #3957 (20e127f) into master (2df7468) will decrease coverage by 0.01%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #3957      +/-   ##
==========================================
- Coverage   54.48%   54.46%   -0.02%     
==========================================
  Files         390      390              
  Lines       48608    48619      +11     
==========================================
- Hits        26482    26480       -2     
- Misses      19902    19912      +10     
- Partials     2224     2227       +3     
Impacted Files Coverage Δ
cmd/goal/clerk.go 8.93% <0.00%> (-0.06%) ⬇️
daemon/algod/api/server/v2/dryrun.go 70.94% <100.00%> (+0.63%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
ledger/tracker.go 73.16% <0.00%> (-2.17%) ⬇️
network/wsPeer.go 68.49% <0.00%> (-1.92%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/service.go 68.88% <0.00%> (+0.74%) ⬆️
ledger/acctupdates.go 69.56% <0.00%> (+0.79%) ⬆️
data/transactions/verify/txn.go 45.02% <0.00%> (+0.86%) ⬆️
... and 2 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 2df7468...20e127f. Read the comment docs.

@algoidurovic algoidurovic requested a review from winder May 9, 2022 18:09
@algoidurovic algoidurovic changed the title Dryrun: Change dryrun cost field to signed int from unsigned in Dryrun: Split dryrun cost field into BudgetCredit and BudgetDebit May 11, 2022
Copy link
Contributor

@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.

Mostly looks good to me, just some minor comments

result.Cost = &cost64
// amount the budget was increased
budgetDebit := uint64(proto.MaxAppProgramCost * numInnerTxns(delta))
budgetCredit := uint64(cost) + budgetDebit
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't budgetCredted just cost? It is because cost is really credit - debit at this point? This might warrant a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll add a comment to clarify. You're right, cost = credit - debit

@@ -0,0 +1,2 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites></testsuites>
Copy link
Contributor

Choose a reason for hiding this comment

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

Added by mistake?

winder
winder previously approved these changes May 17, 2022
@algoidurovic algoidurovic removed the request for review from jannotti May 23, 2022 14:09
michaeldiamant
michaeldiamant previously approved these changes May 25, 2022
Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algoidurovic Thanks for preserving backwards compatibility + marking cost as deprecated. The new fields look good. 👍

@algoidurovic algoidurovic changed the title Dryrun: Split dryrun cost field into BudgetCredit and BudgetDebit Dryrun: Split dryrun cost field into BudgetConsumed and BudgetAdded May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants