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

AVM: Add support for Boxes #4149

Merged
merged 227 commits into from
Oct 31, 2022
Merged

AVM: Add support for Boxes #4149

merged 227 commits into from
Oct 31, 2022

Conversation

michaeldiamant
Copy link
Contributor

@michaeldiamant michaeldiamant commented Jun 17, 2022

Provides an integration branch PR to simplify tracking changes to feature/avm-box in support of #3890.

Zeph Grunschlag and others added 29 commits June 8, 2022 09:27
At least three four left to consider, as well as extensive testing.

Catchpoints, LRU cache, new hash costs
* README.md as committed to in the last retro
* See https://gist.github.com/tzaffi/d57018cb826f4766f8f4ce2ba4dbaec8 for the Justfile that was originally in the PR.
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #4149 (238906c) into master (ab87a8a) will decrease coverage by 0.31%.
The diff coverage is 61.12%.

@@            Coverage Diff             @@
##           master    #4149      +/-   ##
==========================================
- Coverage   54.49%   54.18%   -0.32%     
==========================================
  Files         407      413       +6     
  Lines       52425    53408     +983     
==========================================
+ Hits        28569    28939     +370     
- Misses      21472    22060     +588     
- Partials     2384     2409      +25     
Impacted Files Coverage Δ
catchup/ledgerFetcher.go 39.32% <0.00%> (-0.91%) ⬇️
cmd/goal/interact.go 3.62% <0.00%> (ø)
cmd/tealdbg/localLedger.go 64.81% <0.00%> (-0.41%) ⬇️
config/localTemplate.go 40.00% <ø> (ø)
daemon/algod/api/server/v2/dryrun.go 70.60% <0.00%> (-0.22%) ⬇️
daemon/algod/api/server/v2/utils.go 12.92% <0.00%> (-0.27%) ⬇️
data/basics/userBalance.go 29.72% <0.00%> (-1.41%) ⬇️
data/transactions/logic/doc.go 61.53% <ø> (ø)
data/transactions/logic/fields.go 75.12% <ø> (ø)
data/transactions/logic/fields_string.go 44.44% <ø> (ø)
... and 70 more

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

algorandskiy
algorandskiy previously approved these changes Oct 29, 2022
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Although I disagree with not storing any bits of boxes in ApplyData, everything else looks good for me.

@jannotti jannotti requested review from jasonpaulos and cce October 31, 2022 12:46
@michaeldiamant michaeldiamant changed the title AVM: Merge feature/avm box into master AVM: Add support for Boxes Oct 31, 2022
@michaeldiamant michaeldiamant mentioned this pull request Oct 31, 2022
5 tasks
@michaeldiamant
Copy link
Contributor Author

PR looks ready to merge. Since I opened it, I can't approve.

Note - Calling it out so it's not forgotten: #4704 is also needed for the release.

jasonpaulos
jasonpaulos previously approved these changes Oct 31, 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.

On the whole looks good. I left some nits but I don't view them as blockers

@@ -28,9 +28,8 @@ import (
"github.com/algorand/go-algorand/protocol"
)

type logicLedger struct {
cow cowForLogicLedger
Copy link
Contributor

Choose a reason for hiding this comment

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

just noticed, the cowForLogicLedger interface is now no longer used as a type anywhere in the code — I guess the interface listing below mostly exists as documentation (??) but could be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing. I thought I had - maybe it came back in a merge.

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.