fix(migration): metadata migration was incorrect #7525
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tac0turtle
requested review from
colin-axner,
AdityaSripal,
damiannolan,
chatton and
DimitrisJim
as code owners
|
There is some failing unit tests, I will try to look into later today if you don't have time |
damiannolan
added
backport-to-v9.0.x
backport-to-v8.4.x
backport-to-v8.5.x
priority
|
|
Is there a bug report we could link to this pr? I am having a hard time understanding the exact issue and the solution to it |
damiannolan
reviewed
Oct 30, 2024
There was a problem hiding this comment.
Looked at this for quite a while and tried to exhaust scenarios in which this can overwrite denom metadata. I don't understand how it could without some more info. I mentioned the only scenario I can think of which would allow possible overwrite resulting in reduced number of DenomUnits. It would require some underlying store error or corrupt state.
We can look at testing this e2e to 100% verify the code path, but rn its unclear to me how it would be any different to how we understand it.
Comment on lines
+153
to
+159
| for _, dunit := range denomMetadata.DenomUnits { | ||
| du = append(du, &banktypes.DenomUnit{ | ||
| Denom: dunit.Denom, | ||
| Exponent: dunit.Exponent, | ||
| Aliases: dunit.Aliases, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't understand how this code is reachable.
Comment on lines
-46
to
-47
| if !m.keeper.bankKeeper.HasDenomMetaData(ctx, dt.IBCDenom()) { | ||
| m.keeper.setDenomMetadataWithDenomTrace(ctx, dt) |
There was a problem hiding this comment.
If there exists a denom metadata we will not overwrite it.
The only scenario in which this can overwrite an existing denom metadata, entering the if branch here is:
There was a problem hiding this comment.
I wonder if it is possible that the key constructed in an old version differs from the key constructed in a new version. We should verify that all non-ibc denom metadata's are queryable
|
I have tested an e2e chain upgrade to ensure our migration code works as expected (specifically when existing metadata with denom units > 1 are stored in x/bank state). I will link below code diffs and outline for anyone who would like to sanity check this. Steps to reproduce:
E2E chain upgrade config---
chains:
# the entry at index 0 corresponds to CHAIN_A
- chainId: chainA-1
numValidators: 1
numFullNodes: 1
image: ghcr.io/cosmos/ibc-go-simd # override with CHAIN_IMAGE
tag: v7.8.0-dev-e2e # override with CHAIN_A_TAG
binary: simd # override with CHAIN_BINARY
# the entry at index 1 corresponds to CHAIN_B
- chainId: chainB-1
numValidators: 1
numFullNodes: 1
image: ghcr.io/cosmos/ibc-go-simd # override with CHAIN_IMAGE
tag: v7.8.0-dev-e2e # override with CHAIN_B_TAG
binary: simd # override with CHAIN_BINARY
activeRelayer: hermes # override with RELAYER_ID
relayers:
- id: hermes
image: ghcr.io/informalsystems/hermes
tag: "1.10.0"
- id: rly
image: ghcr.io/cosmos/relayer
tag: "latest"
- id: hyperspace
image: ghcr.io/misko9/hyperspace
tag: "20231122v39"
cometbft:
logLevel: info
debug:
# setting this value to true will force log collection even if the test passes.
dumpLogs: false
# settings this value to true will keep the containers running after the test finishes.
keepContainers: true
# Required only for upgrade tests.
# Chain A will be upgraded the specified tag.
# The plan name must be registered as an upgrade handler in the given tag.
upgrade:
planName: "v8" # override with CHAIN_UPGRADE_PLAN
tag: "v8.5.0" # override with CHAIN_UPGRADE_TAG
Test passes successfully. Test output |
could we get a bug report/slack message that explains how they are breaking? Is it possible the issue is elsewhere? |
|
in testing it seems this is not where the issue is coming from |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
this pr fixes an issue in which ibc was migrating metadata but not setting the values of the denom units, this has been breaking indexers and causing issues across users
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.