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

Problem: versiondb multistore can't query mem store state #1230

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Oct 26, 2023

Closes: #1226

Solution:

  • share the mem store instance with the main store

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

Summary by CodeRabbit

  • Bug Fix: Resolved an issue with the memory store in the version database, enhancing the reliability of the software.
  • Refactor: Streamlined the initialization process of the application, improving the software's performance and maintainability.
  • Refactor: Enhanced the modularity of the setupVersionDB function, allowing for more flexible memory store mounting.
  • Test: Adjusted the test_call function in integration tests to remove the gas limit for transactions, potentially leading to more comprehensive testing outcomes.

Please note that these changes are primarily under-the-hood improvements and may not result in noticeable changes for end-users. However, they contribute to the overall performance, reliability, and maintainability of the software.

@yihuang yihuang requested a review from a team as a code owner October 26, 2023 02:30
@yihuang yihuang requested review from mmsqe and devashishdxt and removed request for a team October 26, 2023 02:30
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2023

Walkthrough

This pull request introduces significant changes to the setup and management of memory and transient stores in the versiondb and app modules. It enhances the modularity and flexibility of the setupVersionDB function, and modifies the MultiStore struct to improve the handling of transient stores. The changes also affect the initialization process of the App struct.

Changes

File(s) Summary
app/app.go, app/versiondb.go, app/versiondb_placeholder.go The initialization process of the App struct has been modified. The setupVersionDB function is now a method of the App struct and has been enhanced for better modularity and flexibility.
versiondb/multistore.go The MultiStore struct has been modified to improve the handling of transient stores. The MountMemoryStores and MountTransientStores functions now store an empty struct in the transientStores map instead of creating a new store.
CHANGELOG.md The changelog includes a fix for the mem store in the versiondb multistore.

"In the land of code, where logic is king, 🤴
Changes were made, improvements they bring. 🚀
Memory stores dance, and transient ones sing, 🎵
In the versiondb's court, a new spring! 🌸
Celebrate, dear rabbits, let your joy ring! 🐰🎉"


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f9d69a7 and 0b03964.
Files selected for processing (5)
  • CHANGELOG.md (1 hunks)
  • app/app.go (2 hunks)
  • app/versiondb.go (2 hunks)
  • app/versiondb_placeholder.go (1 hunks)
  • versiondb/multistore.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • versiondb/multistore.go
Additional comments: 4
app/versiondb_placeholder.go (1)
  • 13-14: The function setupVersionDB has been changed from a package-level function to a method on the App struct. Ensure that all calls to this function throughout the codebase have been updated to match the new method call.
app/versiondb.go (2)
  • 16-17: The function signature has been changed from setupVersionDB to (app *App) setupVersionDB. This change makes setupVersionDB a method of the App struct instead of a standalone function. Ensure that all calls to this function throughout the codebase have been updated to match the new method call.

  • 43-47: The new code creates a map of memory stores from the app.CommitMultiStore(). This is a change from the previous implementation where the memory stores were directly mounted from the memKeys. This change allows the memory stores to be shared between the main store and the versioned database, which can improve the ability to query the mem store state. However, it's important to ensure that the memory stores are not modified in a way that could affect the main store.

app/app.go (1)
  • 402-407: The initialization of the App struct has been simplified by removing the setup of the versiondb's StreamingService and MultiStore from this section. This change improves the readability and maintainability of the code by reducing the complexity of the App struct initialization.

CHANGELOG.md Outdated Show resolved Hide resolved
Closes: crypto-org-chain#1226

Solution:
- share the mem store instance with the main store
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f9d69a7 and 2fa24ca.
Files selected for processing (5)
  • CHANGELOG.md (1 hunks)
  • app/app.go (2 hunks)
  • app/versiondb.go (2 hunks)
  • app/versiondb_placeholder.go (1 hunks)
  • versiondb/multistore.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • versiondb/multistore.go
Additional comments: 5
app/versiondb_placeholder.go (1)
  • 13-14: The function setupVersionDB has been changed from a standalone function to a method of the App struct. This change removes the need for the app *baseapp.BaseApp parameter, making the function signature cleaner. However, ensure that all calls to this function throughout the codebase have been updated to match the new method call.
app/versiondb.go (2)
  • 16-18: The function setupVersionDB has been changed from a standalone function to a method of the App struct. This change removes the need for the app *baseapp.BaseApp parameter, as the App struct can now be accessed directly within the method. This change simplifies the function signature and makes the function more encapsulated within the App struct.

  • 44-48: The new code creates a map of memory stores using the memory store keys and the app.CommitMultiStore().GetKVStore() function. This map is then used to mount the memory stores to the verDB using the verDB.MountMemoryStores() function. This change allows the setupVersionDB function to reuse the existing memory store instances, improving the efficiency and flexibility of the function.

CHANGELOG.md (1)
  • 9-9: The PR number is still missing in the changelog entry. Please ensure to add the correct PR number for the "Fix mem store in versiondb multistore" entry.
- [#]() Fix mem store in versiondb multistore.
+ [#<PR_NUMBER>]() Fix mem store in versiondb multistore.
Committable suggestion (Beta)
- [#<PR_NUMBER>]() Fix mem store in versiondb multistore.
app/app.go (1)
  • 402-407: The initialization of the App struct has been moved to this section. The BaseApp and cdc fields are being set here. The rest of the fields will be set later in the code. This change improves the readability and maintainability of the code by grouping related operations together.

app/app.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #1230 (96dc22b) into main (b9de162) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1230   +/-   ##
=======================================
  Coverage   36.95%   36.95%           
=======================================
  Files         115      115           
  Lines       10235    10235           
=======================================
  Hits         3782     3782           
  Misses       6080     6080           
  Partials      373      373           
Files Coverage Δ
app/versiondb.go 72.72% <100.00%> (ø)
app/versiondb_placeholder.go 0.00% <ø> (ø)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2fa24ca and ce0f364.
Files selected for processing (1)
  • store/rootmulti/store.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • store/rootmulti/store.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ce0f364 and 8555bd9.
Files selected for processing (1)
  • integration_tests/test_ica_precompile.py (1 hunks)
Additional comments: 1
integration_tests/test_ica_precompile.py (1)
  • 134-134: The "gas" key and value have been removed from the data dictionary. This change removes the gas limit for the transaction. Ensure that this change does not lead to excessive gas consumption or other unintended side effects.

Signed-off-by: yihuang <huang@crypto.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8555bd9 and e93009c.
Files selected for processing (1)
  • CHANGELOG.md (1} hunks)
Additional comments: 1
CHANGELOG.md (1)
  • 9-9: The new hunk adds a changelog entry for pull request #1230, which fixes an issue with the memory store in the versiondb multistore. This entry provides a brief summary of the changes made in the pull request. It's important to ensure that the link to the pull request is correct and that the description accurately reflects the changes made.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e93009c and 228cda0.
Files selected for processing (2)
  • app/versiondb.go (2} hunks)
  • versiondb/multistore.go (4} hunks)
Additional comments: 8
app/versiondb.go (2)
  • 16-17: The function signature has been changed from setupVersionDB to (app *App) setupVersionDB. This change makes setupVersionDB a method of the App struct instead of a standalone function. Ensure that all calls to this function throughout the codebase have been updated to match the new method call.

  • 40-42: The NewMultiStore function now takes an additional argument app.CommitMultiStore(). This change implies that the MultiStore is now created with the committed state of the application's multistore. Ensure that this change does not introduce any inconsistencies in the state of the application.

versiondb/multistore.go (6)
  • 5-10: The import for github.com/cosmos/cosmos-sdk/store/mem and github.com/cosmos/cosmos-sdk/store/transient has been removed. Ensure that this does not affect any other parts of the code that might be using these packages.

  • 20-32: The type of transientStores in the MultiStore struct has been changed from map[types.StoreKey]types.KVStore to map[types.StoreKey]struct{}. This change implies that the transientStores map no longer holds the actual stores but only the keys. The NewMultiStore function has been updated to reflect this change. Ensure that this change does not affect the functionality of the MultiStore struct.

  • 43-47: The cacheMultiStore function now retrieves the stores from the parent MultiStore instead of directly from the transientStores map. This change is consistent with the modification of the transientStores map to only hold keys instead of the actual stores. Ensure that the parent MultiStore always has the required stores.

  • 79-83: The GetKVStore function now retrieves the store from the parent MultiStore if the key is found in the transientStores map. This change is consistent with the modification of the transientStores map to only hold keys instead of the actual stores. Ensure that the parent MultiStore always has the required stores.

  • 86-90: The MountTransientStores function no longer creates new transient stores. Instead, it only adds the keys to the transientStores map. This change is consistent with the modification of the transientStores map to only hold keys instead of the actual stores. Ensure that the actual stores are created and added to the parent MultiStore elsewhere in the code.

  • 92-96: The MountMemoryStores function no longer creates new memory stores. Instead, it only adds the keys to the transientStores map. This change is consistent with the modification of the transientStores map to only hold keys instead of the actual stores. Ensure that the actual stores are created and added to the parent MultiStore elsewhere in the code.

Signed-off-by: yihuang <huang@crypto.com>
versiondb/multistore.go Outdated Show resolved Hide resolved
Co-authored-by: mmsqe <mavis@crypto.com>
Signed-off-by: yihuang <huang@crypto.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 228cda0 and 96dc22b.
Files selected for processing (1)
  • versiondb/multistore.go (4} hunks)
Files skipped from review due to trivial changes (1)
  • versiondb/multistore.go

@mmsqe mmsqe enabled auto-merge October 27, 2023 01:49
@mmsqe mmsqe added this pull request to the merge queue Oct 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 27, 2023
@yihuang yihuang added this pull request to the merge queue Oct 27, 2023
@yihuang yihuang removed this pull request from the merge queue due to a manual request Oct 27, 2023
@yihuang yihuang added this pull request to the merge queue Oct 27, 2023
Merged via the queue into crypto-org-chain:main with commit c05c9c6 Oct 27, 2023
31 of 32 checks passed
@yihuang yihuang deleted the fix-versiondb-memstore branch October 27, 2023 09:27
yihuang added a commit to yihuang/cronos that referenced this pull request Mar 11, 2024
…rypto-org-chain#1230)

* Problem: versiondb multistore can't query mem store state

Closes: crypto-org-chain#1226

Solution:
- share the mem store instance with the main store

* fix

* verify the fix

* Update CHANGELOG.md

Signed-off-by: yihuang <huang@crypto.com>

* fix mem store

* Update versiondb/multistore.go

Co-authored-by: mmsqe <mavis@crypto.com>
Signed-off-by: yihuang <huang@crypto.com>

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: mmsqe <mavis@crypto.com>
yihuang added a commit that referenced this pull request Mar 11, 2024
…1230) (#1339)

* Problem: versiondb multistore can't query mem store state (backport #1230)

* Problem: versiondb multistore can't query mem store state

Closes: #1226

Solution:
- share the mem store instance with the main store

* fix

* verify the fix

* Update CHANGELOG.md

Signed-off-by: yihuang <huang@crypto.com>

* fix mem store

* Update versiondb/multistore.go

Co-authored-by: mmsqe <mavis@crypto.com>
Signed-off-by: yihuang <huang@crypto.com>

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: mmsqe <mavis@crypto.com>

* update go.mod

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: mmsqe <mavis@crypto.com>
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.

Problem: VersionDB memStore not updating
2 participants