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

chore(all): dependency inject databases and tables #2994

Closed
wants to merge 21 commits into from

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Dec 6, 2022

Changes

Improves codebase health + simplify migration to newer database interface

  • Dependency inject to storeInitialValues
  • Dependency inject to createRuntimeStorage
  • Dependency inject to NewGrandpaState
  • Dependency inject to NewEpochStateFromGenesis
  • Dependency inject to NewEpochState
  • Dependency inject to NewGrandpaStateFromGenesis
  • Dependency inject to NewBlockStateFromGenesis
  • Dependency inject to NewBlockState
  • Remove LoadChainDB, LoadBadgerDB, runtime.NewInMemoryDB and utils.SetupDatabase, newInMemoryGrandpaDatabase
  • Minor test database setup simplifications
    • dot/state: make newInMemoryDB test only
    • Remove unneeded datadir setting for in memory databases
    • Add test .Close() no error assertions
    • Add missing test database .Close() calls
    • Use in memory database when amount is not big (TestTrieDiff)
    • Do not create unneeded trie table in lib/trie's newTestDB
    • dot/state: use t.TempDir() as data dir directly
    • lib/trie: do not create table for TestTrieDiff

Tests

Issues

Fixes #2981

Primary Reviewer

@qdm12 qdm12 changed the base branch from development to qdm12/local-interfaces December 6, 2022 16:26
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2994 (7d33f1a) into development (39e3150) will increase coverage by 0.27%.
The diff coverage is 38.29%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #2994      +/-   ##
===============================================
+ Coverage        51.50%   51.78%   +0.27%     
===============================================
  Files              221      221              
  Lines            28281    28267      -14     
===============================================
+ Hits             14566    14637      +71     
+ Misses           12399    12306      -93     
- Partials          1316     1324       +8     

@qdm12 qdm12 force-pushed the qdm12/local-interfaces branch 3 times, most recently from c6e9924 to d08d1da Compare December 15, 2022 09:54
Base automatically changed from qdm12/local-interfaces to development December 16, 2022 18:35
@qdm12 qdm12 force-pushed the qdm12/dep-inject-db branch 3 times, most recently from 2368954 to f9c6efb Compare January 3, 2023 13:43
@qdm12 qdm12 marked this pull request as ready for review January 3, 2023 14:08
@qdm12 qdm12 changed the title chore(all): dependency inject databases chore(all): dependency inject databases and tables Jan 3, 2023
@qdm12 qdm12 force-pushed the qdm12/dep-inject-db branch 5 times, most recently from 67d3a5c to 2376b3a Compare January 9, 2023 16:37
Copy link
Contributor

@timwu20 timwu20 left a comment

Choose a reason for hiding this comment

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

Not sure why we need to merge this PR if you're going to be swapping out the DB entirely.

cmd/gossamer/config.go Show resolved Hide resolved
@qdm12
Copy link
Contributor Author

qdm12 commented Jan 18, 2023

Not sure why we need to merge this PR if you're going to be swapping out the DB entirely.

Since we dependency inject the database/tables as locally-defined interfaces, it means there will be less changes to be made (no need to repeat update database/table constructors)

@qdm12
Copy link
Contributor Author

qdm12 commented May 19, 2023

I can't push to this branch, it would be nice if someone can pick it up and

git checkout development
git pull
git checkout qdm12/dep-inject-db
git rebase development
git push -f

Thanksss

@timwu20 timwu20 closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency inject databases and tables
4 participants