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

transient store missed to implement store type interface #2037

Merged
merged 2 commits into from
Sep 9, 2018
Merged

transient store missed to implement store type interface #2037

merged 2 commits into from
Sep 9, 2018

Conversation

HaoyangLiu
Copy link

Currently transient store doesn't implement store type interface. As a result, when calculating commitStores, the transient store will also be included, which is unexpected.

func commitStores(version int64, storeMap map[StoreKey]CommitStore) commitInfo {
	storeInfos := make([]storeInfo, 0, len(storeMap))

	for key, store := range storeMap {
		// Commit
		commitID := store.Commit()

		if store.GetStoreType() == sdk.StoreTypeTransient {
			continue
		}

		// Record CommitID
		si := storeInfo{}
		si.Name = key.Name()
		si.Core.CommitID = commitID
		// si.Core.StoreType = store.GetStoreType()
		storeInfos = append(storeInfos, si)
	}

	ci := commitInfo{
		Version:    version,
		StoreInfos: storeInfos,
	}
	return ci
}

Now, we have two params store. One is IAVL store and another one is transient.

		keyParams:        sdk.NewKVStoreKey("params"),
		tkeyParams:       sdk.NewTransientStoreKey("params"),

They have the same name. And the second one should not be included in commitInfo. However, due to the bugs I mentioned above, both the two store are included in commitInfo.
@jaekwon @adrianbrink

@alexanderbez
Copy link
Contributor

Reference: #2013

@HaoyangLiu
Copy link
Author

HaoyangLiu commented Aug 15, 2018

https://github.com/cosmos/cosmos-sdk/pull/2013/files
The above PR still doesn't add store type interface implementation, though it renamed the transient parameter store name.

@alexanderbez
Copy link
Contributor

@HaoyangLiu correct, I'm just adding it as a reference as that was mentioned in the description of your PR ;-)

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Minor failing in the linter (which needs to be fixed before merge) but other than that it looks good!

@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #2037 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2037      +/-   ##
===========================================
- Coverage    64.86%   64.84%   -0.03%     
===========================================
  Files          115      115              
  Lines         6863     6864       +1     
===========================================
- Hits          4452     4451       -1     
- Misses        2127     2129       +2     
  Partials       284      284

@HaoyangLiu
Copy link
Author

I have fixed the issue you mentioned @rigelrozanski

@cwgoes
Copy link
Contributor

cwgoes commented Aug 22, 2018

Try merging develop, might fix CI.

@rigelrozanski
Copy link
Contributor

looks like test_cover failing due to non-determinism, should merge in develop to see if fixes

@ValarDragon
Copy link
Contributor

Re ran, and tests now pass. This should be good to merge

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Thanks for spotting this!

@rigelrozanski rigelrozanski merged commit 3cf3ab1 into cosmos:develop Sep 9, 2018
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.

5 participants