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

[Bug]: App v2 sets "tx" as store key and this fails on the chain upgrade #20317

Closed
1 task done
alpe opened this issue May 8, 2024 · 5 comments · Fixed by #20409
Closed
1 task done

[Bug]: App v2 sets "tx" as store key and this fails on the chain upgrade #20317

alpe opened this issue May 8, 2024 · 5 comments · Fixed by #20409
Assignees
Labels

Comments

@alpe
Copy link
Contributor

alpe commented May 8, 2024

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

App v2 sets "tx" as store key and this fails on the chain upgrade with the error: version of store tx mismatch

(app v1 is not affected)

Cosmos SDK Version

main

How to reproduce?

Run chain upgrade with system tests for app v2

@alpe alpe added the T:Bug label May 8, 2024
@julienrbrt
Copy link
Member

It is only a problem when a chain goes from not using depinject to using depinject right?

@julienrbrt
Copy link
Member

We could update runtime to purposely skip tx when calling

func registerStoreKey(wrapper *AppBuilder, key storetypes.StoreKey) {
, but this is an issue for all modules that aren't actually modules or that just don't need to save state.

Is that really a problem? If so then we would need to add a field to denotate that a module doesn't have a store key in app_config (

)

@tac0turtle
Copy link
Member

We could update runtime to purposely skip tx when calling

func registerStoreKey(wrapper *AppBuilder, key storetypes.StoreKey) {

, but this is an issue for all modules that aren't actually modules or that just don't need to save state.
Is that really a problem? If so then we would need to add a field to denotate that a module doesn't have a store key in app_config (

)

hmm, is there anyway to make this part of depinject? seems we are uncovering edge cases that may prevent users from using depinject. IBC also has a case

@julienrbrt
Copy link
Member

hmm, is there anyway to make this part of depinject?

This has nothing to do with depinject, but a runtime v1 assumption, which requires a runtime change.
Personally, I prefer the option 2, as I can imagine multiple modules having no store.

@tac0turtle
Copy link
Member

option 2 seems the best, we just need to document it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🥳 Done
Development

Successfully merging a pull request may close this issue.

3 participants