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: move pieces of accountdb.go into a storage package #4776

Merged
merged 6 commits into from
Nov 21, 2022

Conversation

icorderi
Copy link
Contributor

@icorderi icorderi commented Nov 9, 2022

Summary

Picks up on #4428 but with an incremental approach to the refactoring.

Test Plan

Existing tests, this is code refactor.

@icorderi icorderi added quality Improve code quality or style Team Carbon-11 labels Nov 9, 2022
@icorderi icorderi self-assigned this Nov 9, 2022
@@ -0,0 +1,702 @@
package store
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reviewers:

98% of the changes originate here.

Moving this structs to be public, and some of their fields in turn to be public caused a lot of upercasing changes all across the ledger package.

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #4776 (8febfaf) into master (5b45539) will decrease coverage by 0.43%.
The diff coverage is 56.69%.

@@            Coverage Diff             @@
##           master    #4776      +/-   ##
==========================================
- Coverage   54.63%   54.20%   -0.44%     
==========================================
  Files         417      419       +2     
  Lines       53734    53734              
==========================================
- Hits        29358    29126     -232     
- Misses      21940    22225     +285     
+ Partials     2436     2383      -53     
Impacted Files Coverage Δ
ledger/persistedaccts_list.go 92.00% <ø> (ø)
ledger/persistedonlineaccts_list.go 92.00% <ø> (ø)
ledger/tracker.go 78.72% <ø> (+4.68%) ⬆️
ledger/store/sql.go 3.11% <3.11%> (ø)
ledger/acctupdates.go 69.51% <62.96%> (+0.24%) ⬆️
ledger/onlineaccountscache.go 88.00% <66.66%> (ø)
ledger/accountdb.go 68.79% <90.07%> (-3.56%) ⬇️
ledger/acctonline.go 77.60% <93.33%> (-0.53%) ⬇️
ledger/store/data.go 93.59% <93.59%> (ø)
ledger/catchpointtracker.go 59.02% <100.00%> (ø)
... and 21 more

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

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.

A big part of the prev PR was in introducing PersistedAccountData and etc interfaces and have these types non-exported from the package. Please consider doing the same.

ledger/store/testing/helpers.go Show resolved Hide resolved
}

// SetCoreAccountData setter for core account data.
func (ba *BaseAccountData) SetCoreAccountData(ad *ledgercore.AccountData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At least move struct type declaration to right above the functions for the type; maybe move the declaration and its functions to a dedicated file, e.g. base_account_data.go

Same for each type and its functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Brain

}

// AccountsInitDbQueries constructs an AccountsReader backed by sql queries.
func AccountsInitDbQueries(q db.Queryable) (*accountsDbQueries, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's weird that a public function returns a private type. If it returns an instance of an interface then return the interface type? weird public/private mix happens several times below

@icorderi icorderi force-pushed the refactor/accountdb branch 3 times, most recently from 88ef255 to 2dd380c Compare November 18, 2022 19:53
algorandskiy
algorandskiy previously approved these changes Nov 21, 2022
ledger/store/data.go Outdated Show resolved Hide resolved
ledger/store/data.go Outdated Show resolved Hide resolved
}

// SetCoreAccountData setter for core account data.
func (ba *BaseAccountData) SetCoreAccountData(ad *ledgercore.AccountData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Brain

@@ -964,29 +988,6 @@ func TestAccountsReencoding(t *testing.T) {
require.NoError(t, err)
}

// TestAccountsDbQueriesCreateClose tests to see that we can create the accountsDbQueries and close it.
// it also verify that double-closing it doesn't create an issue.
func TestAccountsDbQueriesCreateClose(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where did this test go to? can't see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I pasted it back here in the wrong location, its a the top of the file now.
I tried taking it out but had some other dependencies that needed moving and decided to bring it back for now.

ledger/acctupdates_test.go Outdated Show resolved Hide resolved
@algorandskiy algorandskiy merged commit 60d0c09 into algorand:master Nov 21, 2022
@algorandskiy algorandskiy changed the title refactor: start moving pieces of accountdb.go into a storage package ledger: move pieces of accountdb.go into a storage package Nov 21, 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.

None yet

3 participants