Mount every single store with its own DB #726
Conversation
Our tests are at best probabilistic deterministic.
Codecov Report
@@ Coverage Diff @@
## develop #726 +/- ##
==========================================
+ Coverage 64.04% 64.15% +0.1%
==========================================
Files 43 43
Lines 2022 2028 +6
==========================================
+ Hits 1295 1301 +6
Misses 615 615
Partials 112 112 |
|
Testet locally. It allows you to restart the app without wiping the database. |
There was a problem hiding this comment.
Awesome thank you.
We have unit testing that captures this but is just commented out.
eg. https://github.com/cosmos/cosmos-sdk/blob/master/baseapp/baseapp_test.go#L154
and
https://github.com/cosmos/cosmos-sdk/blob/master/examples/basecoin/app/app_test.go#L145
Maybe we can write a proper specific unit test for it too ?
baseapp/baseapp.go
Outdated
| } | ||
| } | ||
|
|
||
| // Mount a store to the provided key in the BaseApp multistore | ||
| func (app *BaseApp) MountStore(key sdk.StoreKey, typ sdk.StoreType) { | ||
| app.cms.MountStoreWithDB(key, typ, app.db) | ||
| func (app *BaseApp) MountStore(key sdk.StoreKey, typ sdk.StoreType, db dbm.DB) { |
There was a problem hiding this comment.
let's leave the old MountStore, mark it as broken like above, and add a new MountStoreWithDB method to BaseApp
examples/basecoin/app/app.go
Outdated
| capKeyIBCStore *sdk.KVStoreKey | ||
| capKeyStakingStore *sdk.KVStoreKey | ||
|
|
||
| // Manage getting and setting accounts | ||
| accountMapper sdk.AccountMapper | ||
| } | ||
|
|
||
| func NewBasecoinApp(logger log.Logger, db dbm.DB) *BasecoinApp { | ||
| func NewBasecoinApp(logger log.Logger, dbMain, dbAcc, dbIBC, dbStaking dbm.DB) *BasecoinApp { |
There was a problem hiding this comment.
lets start a convention here where we pass in a map[string]dbm.DB and then we can use that to fill in the capKeys, rather than this hardcoded list.
Then we can add more dbs (governance, etc.) without breaking the API
There was a problem hiding this comment.
Once #532 is merged we will only need one database and it feels strange that we want to pass in multiple databases. I think it provides a strange API if we allow a map of databases to be passed in, because then we also need to have some convention around how those map to capKeys.
There was a problem hiding this comment.
The convention is extremely simple: use the same string.
I have the following facts:
- the DB will be fixed an unknown amount of time in the future (call it months)
- we will add new stores soon (call it weeks)
- we have users of this API (including ourselves, in tests)
- the current approach will lead to more breakages of this API
I would think the above signals strongly that we should take an approach that can last for a bit and will require the least amount of maintenance work. Why set us up to do more unnecessary work in the future ?
examples/basecoin/app/app.go
Outdated
| cdc: MakeCodec(), | ||
| capKeyMainStore: sdk.NewKVStoreKey("main"), | ||
| capKeyAccountStore: sdk.NewKVStoreKey("acc"), |
There was a problem hiding this comment.
what is the account key used for ? currently we just put it in the main I think
There was a problem hiding this comment.
Yes, we used the main store also as the account store. However it seems cleaner if the accounts get their own capKey instead of just using the main one. In the future we might want to give someone access to the accounts without giving them access to governance.
|
Still need to address the comments and get unit tests in place but would be great to merge and release this today if possible |
I hope this is correct. I'm feely pretty dizzy right now from the fish food.
NewBasecoinApp takes a map[string]dbm.DB . This stabilises the API, since it allows us to add more stores without a breaking change. The convention is that the keys of the dbs correspond to the names of the capKeys.
|
I'm going to uncomment the other test I mentioned as well and push to this branch, then merge |
|
Oh I missed that one. Sorry, was travelling all of yesterday for the Easter celebrations. |
#722