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

ledger: refactor the ledger Totals #2846

Merged
merged 13 commits into from
Sep 29, 2021

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Sep 6, 2021

Summary

The plan to change the tracker database to maintain the latest 320 rounds for the online accounts only have some other required modification; one of them is the semantics of the Totals method:

  • At this time, it supports retrieving AccountTotals for any of the recent 320 rounds.
  • We need the method to support the AccountTotals for the latest round only, as well as provide the circulating supply for latest-320 rounds ( i.e. the circulating supply is a subset of the AccountTotals ).

Once the database implementation is complete, the database would contain the AccountTotals information for the latest round only, plus the online circulation for the past 320 rounds.

In order to support that, I've broken up the interface into:

  • LatestTotals, which would return the latest totals as well as the latest round associated with these totals.
  • OnlineTotals, which receive a round number and return the online totals associated with that round.

This change is focused around the Ledger interface. Further changes would be required in the accounts update before the high-level goals could be achieved.

Test Plan

Unit tests updated.

@tsachiherman tsachiherman changed the title refactor the ledger Totals ledger: refactor the ledger Totals Sep 10, 2021
@tsachiherman
Copy link
Contributor Author

@tolikzinovyev - this change would impact the indexer due to the change in ledgerForEvaluator. I expect that the corresponding change in the indexer would be trivial, though.

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2021

Codecov Report

Merging #2846 (c0129dd) into master (412aef5) will decrease coverage by 0.01%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2846      +/-   ##
==========================================
- Coverage   47.63%   47.61%   -0.02%     
==========================================
  Files         358      358              
  Lines       57887    57906      +19     
==========================================
- Hits        27573    27572       -1     
- Misses      27190    27209      +19     
- Partials     3124     3125       +1     
Impacted Files Coverage Δ
daemon/algod/api/server/v1/handlers/handlers.go 0.62% <0.00%> (+<0.01%) ⬆️
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
data/ledger.go 24.08% <0.00%> (ø)
data/pools/transactionPool.go 43.50% <0.00%> (-1.01%) ⬇️
ledger/ledgercore/error.go 0.00% <0.00%> (ø)
node/node.go 3.78% <0.00%> (ø)
ledger/ledger.go 64.77% <14.28%> (-1.62%) ⬇️
ledger/eval.go 77.80% <33.33%> (-0.23%) ⬇️
ledger/acctupdates.go 62.77% <100.00%> (-0.24%) ⬇️
ledger/evalIndexer.go 43.42% <100.00%> (+4.18%) ⬆️
... and 8 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 412aef5...c0129dd. Read the comment docs.

@tsachiherman tsachiherman requested a review from cce September 10, 2021 16:14
func (au *accountUpdates) latestTotalsImpl() (basics.Round, ledgercore.AccountTotals, error) {
offset := len(au.deltas)
rnd := au.dbRound + basics.Round(len(au.deltas))
return rnd, au.roundTotals[offset], nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be more straight-forward to do au.roundTotals[len(au.roundTotals) - 1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you wrote is correct; but it's less consistent with the rest of the code that is using the len(au.deltas).
There is also the theoretical risk of having a negative value that doesn't apply here.

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.

Looks OK to merge

@tsachiherman tsachiherman merged commit 1202d32 into algorand:master Sep 29, 2021
@tsachiherman tsachiherman deleted the tsachi/refactorcirculation branch September 29, 2021 13:46
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.

4 participants