feat(store/v2): RootStore Implementation #17577
Conversation
There was a problem hiding this comment.
Howdy howdy @alexanderbez? Just some drive-by code review comments, please take a look and anyways your PR is currently a draft. Thank you!
store/multistore/store.go
Outdated
|
|
||
| // Close closes the store and resets all internal fields. Note, Close() is NOT | ||
| // idempotent and should only be called once. | ||
| func (s *Store) Close() (err error) { |
There was a problem hiding this comment.
In that case why not just add a sync.Once called closeOnce and invoke it like this
func (s *Store) Close() (err error) {
s.closeOnce.Do(func() {
err = errors.Join(err, s.ss.Close())
...
})
return
}There was a problem hiding this comment.
Because this would make it idempotent which is not consistent with the rest of the store code. In fact, this was a comment you made previously (to make it not idempotent).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@odeke-em please refrain from reviews till the PR is marked R4R -- it makes my life more challenging because it's always going to be in flux until it's marked ready (e.g. some of the code you review might even be gone by the time it's ready). Thanks for the review though! |
|
@alexanderbez your pull request is missing a changelog! |
store/root/store.go
Outdated
| } | ||
|
|
||
| // commit SC | ||
| commitInfo, err := s.commitSC(version, changeSet) |
There was a problem hiding this comment.
it seems like the finalizeBlock step is missing, the current BaseApp flow looks like WriteBatch->WorkingHash->Commit. Maybe we need to separate the writing from Commit
|
overall, LGTM! |
alexanderbez
changed the title
Description
closes: #17629
ref: #17314
Doc/Changelog
RootStoreand supplementary interfacesRootStoreKVStoreNote:
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking change