-
Notifications
You must be signed in to change notification settings - Fork 492
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: move StartEvaluator parameters into a parameters object #3030
ledger: move StartEvaluator parameters into a parameters object #3030
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3030 +/- ##
==========================================
+ Coverage 43.65% 43.67% +0.02%
==========================================
Files 391 391
Lines 86764 86779 +15
==========================================
+ Hits 37875 37901 +26
+ Misses 42861 42854 -7
+ Partials 6028 6024 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. @tolikzinovyev may be interested in the changes to eval-for-indexer
ledger/internal/eval.go
Outdated
func (x *roundCowBase) initializeAccountsCache(accts map[basics.Address]*basics.AccountData) { | ||
for address, accountData := range accts { | ||
if accountData == nil { | ||
x.accounts[address] = basics.AccountData{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can we have nil entries on init?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indexer would preload the touched accounts. If an account doesn't exist, it would place nil
instead of an empty AccountData{}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we change this code in the indexer preloaded and get rid of initializeAccountsCache
func from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean implementing the caching layer in indexerLedgerConnector
instead ?
What is the reason to have an additional caching layer? |
We're refactoring the ledger, and the evaluator refactoring is part of that. The extra "layer" of the indexer would hopefully help the encapsulation, as some of the underlying data structures are going to be updated and it would reduce the volatility of the indexer interface. In terms of performance, I don't believe it would be noticible. |
…rand#3030) move StartEvaluator parameters into an object. Given that most of the time the parameters are using a "default" state, it reduce the number of passed parameters, improving the caller readability. Unit tests updated.
Summary
move StartEvaluator parameters into an object. Given that most of the time the parameters are using a "default" state, it reduce the number of passed parameters, improving the caller readability.
Test Plan
Unit tests updated.