Transient Stores and Store Refactor #1481

mossid · 2018-06-29T23:34:56Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md
Updated Gaia/Examples
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
Added appropriate labels to PR (ex. wip, ready-for-review, docs)

Closes: #1326

codecov · 2018-06-30T00:07:29Z

Codecov Report

Merging #1481 into develop will increase coverage by 0.02%.
The diff coverage is 75%.

@@             Coverage Diff             @@
##           develop    #1481      +/-   ##
===========================================
+ Coverage    63.42%   63.45%   +0.02%     
===========================================
  Files          117      118       +1     
  Lines         6940     6972      +32     
===========================================
+ Hits          4402     4424      +22     
- Misses        2283     2292       +9     
- Partials       255      256       +1

jaekwon · 2018-06-30T01:57:55Z

store/rootmultistore.go

 // Commits each store and returns a new commitInfo.
 func commitStores(version int64, storeMap map[StoreKey]CommitStore) commitInfo {
-	storeInfos := make([]storeInfo, 0, len(storeMap))
+	storeInfos := make([]storeInfo, 0, len(storeMap)/2)


should be len(storeMap)/2*2

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix

in progress in progress fix all keeper/handler/types, start working on test finalize rebase fixing clients add linear_cli.go, refactor relayer cli fix examples add wire, msg naming format, fix examples in progress in progress add client/store

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix in progress move transient to KVStore fix syntax errors finalize rebase remove NewMemDBStoreAdapter for lint

mossid · 2018-07-04T04:38:53Z

In this PR every operation that is applied to a KVStore has become methods of KVStore, including Transient(), Gas(), Prefix(). With this, we don't have to make complex methods on RootMultiStore to cover the possibilities. One problem here is that Transient() is meaningful on only commitstores so Transient() actually have to be in CommitKVStore. But while trying to do it, I found Context is based on MultiStore and CommitMultiStore. Since the only base layer KVStore we have that does not implement Commiter is dbStoreAdapter, I just make it panic on Transient. It should be fixed later.

mossid · 2018-07-04T22:40:41Z

Ready for review.

ebuchman · 2018-07-05T21:13:43Z

store/cachemultistore.go

@@ -73,7 +73,12 @@ func (cms cacheMultiStore) GetKVStore(key StoreKey) KVStore {
 	return cms.stores[key].(KVStore)
 }

+// Implements Multistore.
+func (cms cacheMultiStore) GetTransientStore(key StoreKey) KVStore {
+	return cms.stores[key].(KVStore)


is this supposed to be identical to GetKVStore?

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix in progress move transient to KVStore fix syntax errors finalize rebase remove NewMemDBStoreAdapter for lint apply requests apply requests apply requests add CHANGELOG add tests fix getter in progress finalize rebase fix lint partially apply requests assert -> require apply requests fix test Changelog => Pending add TransientStoreKey fix lint

in progress in progress fix all keeper/handler/types, start working on test finalize rebase fixing clients add linear_cli.go, refactor relayer cli fix examples add wire, msg naming format, fix examples in progress in progress add client/store in progress in progress cleanup finalize cleanup fix errors refactor ibc

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix in progress move transient to KVStore fix syntax errors finalize rebase remove NewMemDBStoreAdapter for lint apply requests apply requests apply requests add CHANGELOG add tests fix getter in progress finalize rebase fix lint partially apply requests assert -> require apply requests fix test Changelog => Pending add TransientStoreKey fix lint fix lint

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix in progress move transient to KVStore fix syntax errors finalize rebase remove NewMemDBStoreAdapter for lint apply requests apply requests apply requests add CHANGELOG add tests fix getter in progress finalize rebase fix lint partially apply requests assert -> require apply requests fix test Changelog => Pending add TransientStoreKey fix lint fix lint refactor *Store from context, add ReadKVStore delete immutablestore fix KVStore fix TransientStore remove StoreGetter fix errors

cwgoes

utACK, another review would be good

alexanderbez · 2018-07-25T13:36:56Z

I'll take a look @cwgoes

alexanderbez

I left some general feedback, but nothing that would withstand a merge/approval.

alexanderbez · 2018-07-25T18:45:38Z

PENDING.md

@@ -30,6 +30,7 @@ FEATURES
 * [baseapp] Initialize validator set on ResponseInitChain
 * [cosmos-sdk-cli] Added support for cosmos-sdk-cli tool under cosmos-sdk/cmd	
   * This allows SDK users to initialize a new project repository.
+* [store] Add transient store


@cwgoes is the convention to also link the PR # here? I think that'll be helpful in the CHANGELOG

alexanderbez · 2018-07-25T18:52:17Z

types/gas.go

+
+// Default gas config for TransientStores
+func TransientGasConfig() GasConfig {
+	// TODO: reduce the gas cost in transient store


To go along with the TODO, why not just return DefaultGasConfig() for now as the values are identical?

alexanderbez · 2018-07-25T18:56:02Z

store/transientstore.go

+
+// Implements CommitStore
+func (ts *transientStore) SetPruning(pruning PruningStrategy) {
+	// Do nothing


Dont think we need this redundant comment 👍

alexanderbez · 2018-07-25T18:59:34Z

store/rootmultistore.go

@@ -440,6 +440,10 @@ func commitStores(version int64, storeMap map[StoreKey]CommitStore) commitInfo {
 		// Commit
 		commitID := store.Commit()

+		if commitID.IsZero() {


Was this ever addressed? It does indeed seem that the TransientStore always returns zero. I can't think of a case where other commits would result in zero, but at least it'd be helpful to add a comment here 👍

Could we leverage GetStoreType here instead?

alexanderbez · 2018-07-25T19:12:22Z

store/rootmultistore.go

@@ -322,10 +319,23 @@ func (rs *rootMultiStore) loadCommitStoreFromParams(id CommitID, params storePar
 		// TODO: id?
 		// return NewCommitMultiStore(db, id)
 	case sdk.StoreTypeIAVL:
+		_, ok := key.(*sdk.KVStoreKey)


Definitely out of scope of this PR, but I really think we should avoid using named return variables in large and/or complex functions/methods.

move gasconfig to types, make GetKVStoreWithGas take GasConfig fix lint modify transientstore in progress add test for transientstore fix errors fix test fix errors and lint last fix in progress move transient to KVStore fix syntax errors finalize rebase remove NewMemDBStoreAdapter for lint apply requests apply requests apply requests add CHANGELOG add tests fix getter in progress finalize rebase fix lint partially apply requests assert -> require apply requests fix test Changelog => Pending add TransientStoreKey fix lint fix lint refactor *Store from context, add ReadKVStore delete immutablestore fix KVStore fix TransientStore remove StoreGetter fix errors apply requests fix unused CommitStoreLoader error

in progress in progress fix all keeper/handler/types, start working on test finalize rebase fixing clients add linear_cli.go, refactor relayer cli fix examples add wire, msg naming format, fix examples in progress in progress add client/store in progress in progress cleanup finalize cleanup fix errors refactor ibc

mossid added the wip label Jun 29, 2018

mossid requested a review from ebuchman as a code owner June 29, 2018 23:34

mossid force-pushed the joon/1326-transient-store branch from c6fb76e to b8660aa Compare June 30, 2018 00:07

jaekwon reviewed Jun 30, 2018

View reviewed changes

mossid force-pushed the joon/1326-transient-store branch from 224579d to bd0f7e2 Compare June 30, 2018 03:11

mossid force-pushed the joon/1326-transient-store branch from bd0f7e2 to c183fc9 Compare June 30, 2018 03:30

mossid force-pushed the joon/1326-transient-store branch from c183fc9 to daed85e Compare June 30, 2018 03:34

mossid force-pushed the joon/1326-transient-store branch from daed85e to e3de3e0 Compare July 3, 2018 00:47

mossid force-pushed the joon/1326-transient-store branch from e3de3e0 to 8f00ab4 Compare July 3, 2018 00:48

mossid force-pushed the joon/1326-transient-store branch from 8f00ab4 to 31786cc Compare July 3, 2018 01:05

mossid force-pushed the joon/1326-transient-store branch from 31786cc to 39509c7 Compare July 4, 2018 04:32

mossid changed the base branch from develop to unstable July 4, 2018 04:32

mossid changed the title ~~WIP: Transient Stores~~ Transient Stores and Store Refactor Jul 4, 2018

cwgoes changed the base branch from unstable to develop July 4, 2018 19:26

mossid added ready-for-review and removed wip labels Jul 4, 2018

ebuchman reviewed Jul 5, 2018

View reviewed changes

mossid force-pushed the joon/1326-transient-store branch from 39509c7 to a8a1acf Compare July 5, 2018 22:45

mossid force-pushed the joon/1326-transient-store branch from bff248e to 57ab2b3 Compare July 23, 2018 22:19

mossid force-pushed the joon/1326-transient-store branch from 57ab2b3 to f27d375 Compare July 23, 2018 22:20

mossid force-pushed the joon/1326-transient-store branch from f27d375 to 0cafe33 Compare July 24, 2018 00:05

mossid force-pushed the joon/1326-transient-store branch from 0cafe33 to 2c7a8b5 Compare July 24, 2018 00:08

mossid force-pushed the joon/1326-transient-store branch from 2c7a8b5 to f76e8a7 Compare July 24, 2018 00:21

mossid force-pushed the joon/1326-transient-store branch from f76e8a7 to 0936344 Compare July 24, 2018 00:28

mossid force-pushed the joon/1326-transient-store branch from 0936344 to 78164f7 Compare July 24, 2018 05:16

cwgoes approved these changes Jul 25, 2018

View reviewed changes

alexanderbez approved these changes Jul 25, 2018

View reviewed changes

mossid force-pushed the joon/1326-transient-store branch from 29b3945 to 44215b5 Compare July 26, 2018 00:04

mossid force-pushed the joon/1326-transient-store branch from 44215b5 to 17c5021 Compare July 26, 2018 23:40

mossid force-pushed the joon/1326-transient-store branch from 17c5021 to f9f8445 Compare July 27, 2018 00:33

cwgoes merged commit d46140a into develop Jul 27, 2018

cwgoes deleted the joon/1326-transient-store branch July 27, 2018 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transient Stores and Store Refactor #1481

Transient Stores and Store Refactor #1481

mossid commented Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 30, 2018 •

edited

Loading

jaekwon Jun 30, 2018

mossid commented Jul 4, 2018

mossid commented Jul 4, 2018

ebuchman Jul 5, 2018

cwgoes left a comment

alexanderbez commented Jul 25, 2018

alexanderbez left a comment

alexanderbez Jul 25, 2018

alexanderbez Jul 25, 2018

alexanderbez Jul 25, 2018

alexanderbez Jul 25, 2018

alexanderbez Jul 25, 2018

Transient Stores and Store Refactor #1481

Transient Stores and Store Refactor #1481

Conversation

mossid commented Jun 29, 2018 • edited Loading

codecov bot commented Jun 30, 2018 • edited Loading

Codecov Report

jaekwon Jun 30, 2018

Choose a reason for hiding this comment

mossid commented Jul 4, 2018

mossid commented Jul 4, 2018

ebuchman Jul 5, 2018

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Jul 25, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Jul 25, 2018

Choose a reason for hiding this comment

alexanderbez Jul 25, 2018

Choose a reason for hiding this comment

alexanderbez Jul 25, 2018

Choose a reason for hiding this comment

alexanderbez Jul 25, 2018

Choose a reason for hiding this comment

alexanderbez Jul 25, 2018

Choose a reason for hiding this comment

mossid commented Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 30, 2018 •

edited

Loading