-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
chore(x/group,x/nft,x/mint): use cosmossdk.io/core/codec
instead of github.com/cosmos/cosmos-sdk/codec
#23293
Conversation
…ithub.com/cosmos/cosmos-sdk/codec
📝 WalkthroughWalkthroughThis pull request introduces a systematic update to the codec dependency across multiple modules in the Cosmos SDK. The changes primarily involve replacing the codec import from Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
cosmossdk.io/core/codec
instead of github.com/cosmos/cosmos-sdk/codec
cosmossdk.io/core/codec
instead of github.com/cosmos/cosmos-sdk/codec
cosmossdk.io/core/codec
instead of github.com/cosmos/cosmos-sdk/codec
cosmossdk.io/core/codec
instead of github.com/cosmos/cosmos-sdk/codec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/nft/simulation/decoder.go (1)
22-27
: Consider adding context to panic messages.While the error handling is improved by using
Unmarshal
instead ofMustUnmarshal
, the panic messages could be more descriptive to aid in debugging simulation failures.- if err := cdc.Unmarshal(kvA.Value, &classA); err != nil { - panic(err) + if err := cdc.Unmarshal(kvA.Value, &classA); err != nil { + panic(fmt.Errorf("failed to unmarshal NFT class A: %w", err))Also applies to: 31-36
x/group/simulation/decoder.go (1)
22-27
: Consider adding context to panic messages.While the error handling is consistent across all struct types, the panic messages could be more descriptive. Consider wrapping errors with context about which type failed to unmarshal.
- if err := cdc.Unmarshal(kvA.Value, &groupA); err != nil { - panic(err) + if err := cdc.Unmarshal(kvA.Value, &groupA); err != nil { + panic(fmt.Errorf("failed to unmarshal GroupInfo A: %w", err))Also applies to: 33-38, 44-49, 55-60, 66-71
x/group/keeper/genesis.go (1)
15-17
: Consider using errors.Wrap for consistency.The error handling in InitGenesis could be more consistent with the rest of the file by using
errors.Wrap
instead of panic.- if err := cdc.UnmarshalJSON(data, &genesisState); err != nil { - panic(err) + if err := cdc.UnmarshalJSON(data, &genesisState); err != nil { + return errors.Wrap(err, "failed to unmarshal genesis state") + }x/nft/module/module.go (1)
88-92
: Consider returning error instead of panic in DefaultGenesis.While the error handling is improved by using
MarshalJSON
instead ofMustMarshalJSON
, consider returning an error instead of panicking to be consistent with other genesis-related methods in the module.-func (am AppModule) DefaultGenesis() json.RawMessage { +func (am AppModule) DefaultGenesis() (json.RawMessage, error) { data, err := am.cdc.MarshalJSON(nft.DefaultGenesisState()) if err != nil { - panic(err) + return nil, errors.Wrap(err, "failed to marshal default genesis state") } - return data + return data, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
x/group/internal/orm/types.go
(1 hunks)x/group/keeper/genesis.go
(1 hunks)x/group/module/module.go
(2 hunks)x/group/simulation/decoder.go
(2 hunks)x/mint/module.go
(2 hunks)x/nft/module/module.go
(2 hunks)x/nft/simulation/decoder.go
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
x/mint/module.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/simulation/decoder.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/keeper/genesis.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/internal/orm/types.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/nft/simulation/decoder.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/nft/module/module.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/module/module.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (10)
x/nft/simulation/decoder.go (1)
7-7
: LGTM! Import statement updated correctly.The codec import has been correctly updated to use
cosmossdk.io/core/codec
.x/group/simulation/decoder.go (1)
7-7
: LGTM! Import statement updated correctly.The codec import has been correctly updated to use
cosmossdk.io/core/codec
.x/group/keeper/genesis.go (1)
7-7
: LGTM! Import statement updated correctly.The codec import has been correctly updated to use
cosmossdk.io/core/codec
.x/nft/module/module.go (1)
11-11
: LGTM! Import statement updated correctly.The codec import has been correctly updated to use
cosmossdk.io/core/codec
.x/group/internal/orm/types.go (2)
12-12
: LGTM! Import change aligns with module modernization.The change from
github.com/cosmos/cosmos-sdk/codec
tocosmossdk.io/core/codec
is part of the broader effort to modernize the codebase.
Line range hint
98-116
: LGTM! Well-structured error handling with explicit codec usage.The implementation properly handles errors and uses the provided codec for unmarshaling data.
x/mint/module.go (2)
13-13
: LGTM! Consistent import modernization.The change to use
cosmossdk.io/core/codec
aligns with the module modernization effort.
113-117
: LGTM! Improved error handling in DefaultGenesis.The change from
MustMarshalJSON
to explicit error handling provides better error context while maintaining the same panic behavior.x/group/module/module.go (2)
13-13
: LGTM! Consistent import modernization.The change to use
cosmossdk.io/core/codec
aligns with the module modernization effort.
121-125
: LGTM! Improved error handling in DefaultGenesis.The change from
MustMarshalJSON
to explicit error handling provides better error context while maintaining the same panic behavior. This is consistent with the changes in other modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
To merge into #23313 |
Description
Partially addresses #23253
Summary by CodeRabbit
Dependency Updates
cosmossdk.io/core
Error Handling Improvements
MustMarshalJSON
with explicit error checkingCode Maintenance