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

feat!: ADR-040: Refactor App to use v2.MultiStore #10174

Closed
wants to merge 93 commits into from

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Sep 16, 2021

Description

Part of: #10192
Closes #10804

Refactors the App state store and supporting code to use the new store/v2.RootStore type introduced by #10430.
Specified by ADR-040.

Note:

Store behavior additions, changes (breakages?):

  • Substores must be registered/migrated at init
  • MultiStore must be Closed to resolve DB transactions
  • App must do CloseStore() to resolve its store
  • StoreTypePersistent for substores
  • StoreTypeSMT for SC-only smt.Store

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@roysc roysc changed the title !feat: ADR-040: Refactor root (multi) store feat!: ADR-040: Refactor root (multi) store Sep 21, 2021
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from 4f3a83d to 75d96b1 Compare October 14, 2021 10:14
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from 75d96b1 to 4303bba Compare October 21, 2021 13:15
Copy link
Contributor Author

@roysc roysc left a comment

Choose a reason for hiding this comment

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

Comments from online review with @robert-zaremba and @alexanderbez

store/v2/flat/root_store.go Outdated Show resolved Hide resolved
store/v2/flat/root_store.go Outdated Show resolved Hide resolved
store/v2/flat/root_store.go Outdated Show resolved Hide resolved
store/v2/flat/root_store.go Outdated Show resolved Hide resolved
store/v2/flat/root_store.go Outdated Show resolved Hide resolved
store/v2/flat/root_store.go Outdated Show resolved Hide resolved
@roysc roysc changed the title feat!: ADR-040: Refactor root (multi) store feat!: ADR-040: Refactor App to use RootStore Oct 25, 2021
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from 4303bba to f71f755 Compare October 29, 2021 10:39
@robert-zaremba robert-zaremba mentioned this pull request Nov 3, 2021
38 tasks
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from f71f755 to 8efa2af Compare November 11, 2021 12:14
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2021

This pull request introduces 1 alert when merging 8efa2af into 8fc9f76 - view on LGTM.com

new alerts:

  • 1 for Expression has no effect

mergify bot pushed a commit that referenced this pull request Dec 16, 2021
## Description

Part of: #10192

Introduces a new `RootStore` type in the `store/v2` package and an implementation, without yet replacing the `MultiStore` or refactoring its use within the SDK (which will happen in the follow up: #10174).
Specified by [ADR-040](https://github.com/cosmos/cosmos-sdk/blob/1326fa2a7dfc3d83cf23dc1c1f33ff131152ad60/docs/architecture/adr-040-storage-and-smt-state-commitments.md).

Fixes #10651
Fixes #10263

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@i-norden i-norden self-requested a review January 19, 2022 15:16
@roysc roysc changed the title feat!: ADR-040: Refactor App to use RootStore feat!: ADR-040: Refactor App to use v2.MultiStore Jan 20, 2022
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from 8efa2af to d94b249 Compare January 20, 2022 19:11
simapp, baseapp and dependencies
update db interface
options refactor
app.CloseStore()
LoadLatestVersion => Init
...
* remove iterator read lock; doesn't play with orm tests, so just rely on the stated contract: no store writes while iterator open
* use v2 store in orm tests
* clean up app boilerplate
* update NewCommitMultiStore()
* fix NewUncachedContext
rm superfluous app.cms rename

memdb imports

[wip]substore prefix conflict
encode names with varint length to make them unambiguous
@roysc roysc force-pushed the roysc/adr-040-refactor-multistore branch from 3afa273 to 1a9e194 Compare July 21, 2022 22:31
@github-actions github-actions bot added the C:CLI label Jul 22, 2022
@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #10174 (ef77ab2) into main (5019459) will decrease coverage by 0.14%.
The diff coverage is 48.90%.

❗ Current head ef77ab2 differs from pull request most recent head 24dd6c0. Consider uploading reports for the commit 24dd6c0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #10174      +/-   ##
==========================================
- Coverage   65.39%   65.24%   -0.15%     
==========================================
  Files         693      688       -5     
  Lines       70965    72132    +1167     
==========================================
+ Hits        46408    47066     +658     
- Misses      21992    22325     +333     
- Partials     2565     2741     +176     
Impacted Files Coverage Δ
baseapp/state.go 50.00% <0.00%> (+50.00%) ⬆️
baseapp/test_helpers.go 53.84% <0.00%> (-20.35%) ⬇️
db/adapter.go 0.00% <0.00%> (ø)
db/creator.go 0.00% <0.00%> (ø)
server/rollback.go 0.00% <0.00%> (ø)
simapp/simd/cmd/root.go 74.33% <0.00%> (+4.91%) ⬆️
store/v2alpha1/multi/compat.go 0.00% <0.00%> (ø)
store/v2alpha1/multi/sub_store.go 85.45% <ø> (-1.44%) ⬇️
store/v2alpha1/multi/v1asv2.go 0.00% <0.00%> (ø)
testutil/context.go 0.00% <0.00%> (ø)
... and 468 more

Comment on lines +579 to +583
for v, _ := range vm.vmap {
if v > target {
delete(vm.vmap, v)
}
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +69 to +71
for skey, ls := range opts.listeners {
cms.AddListeners(skey, ls)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
}

func (s *store1as2) GetVersion(ver int64) (v2.MultiStore, error) {
ret, err := s.CacheMultiStoreWithVersion(ver)

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
Comment on lines +43 to +45
for k := range backends {
keys = append(keys, string(k))
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +53 to +61
for _, key := range keys {
typ, err := types.StoreKeyToType(key)
if err != nil {
return err
}
if err = par.RegisterSubstore(key, typ); err != nil {
return err
}
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@tac0turtle
Copy link
Member

gonna close this for now. the work is still being reviewed

@tac0turtle tac0turtle closed this Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

ADR-040 Implementation: Incorporate v2.MultiStore into BaseApp
6 participants