chore: move modules #25090
chore: move modules #25090
Conversation
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/auth
C:x/feegrant
C:x/genutil
📝 WalkthroughWalkthroughThis change set systematically migrates the Cosmos SDK's Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant ContribModule as contrib/x/{circuit,evidence,group,nft}
participant SDK as Cosmos SDK Core
App->>ContribModule: Import and use contrib/x/ module APIs
ContribModule->>SDK: Use SDK interfaces and base types
Note right of ContribModule: All logic and API signatures mirror original x/ modules, only import paths and types changed.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
contrib/x/evidence/types/evidence.go (1)
52-57: Fix theValidatorEvidenceinterface signature to match the new method
TheEquivocation.GetConsensusAddressmethod was updated to take anaddress.Codecparameter, but theValidatorEvidenceinterface (and related docs) still defineGetConsensusAddress()with no arguments. This mismatch breaks interface compliance and will prevent compilation.Areas to update:
- contrib/x/evidence/exported/evidence.go
• ChangetoGetConsensusAddress() sdk.ConsAddressGetConsensusAddress(consAc address.Codec) sdk.ConsAddress- contrib/x/evidence/README.md (around line 66)
• Update theValidatorEvidenceinterface’sGetConsensusAddress()signature to includeconsAc address.Codec.- docs/architecture/adr-009-evidence-module.md (around line 63)
• Reflect the new parameter in theGetConsensusAddress(consAc address.Codec) ConsAddresssignature.contrib/x/group/internal/orm/index_test.go (1)
276-282: Nil-pointer risk whenexpErrorisnil
spec.expError.Is(err)is executed unconditionally.
For the happy-path cases whereexpError == nil, this dereferences anilpointer and panics, so these tests will never reach the assertions that follow.- it, err := spec.method(store, spec.start, spec.end) - require.True(t, spec.expError.Is(err), "expected #+v but got #+v", spec.expError, err) - if spec.expError != nil { - return - } + it, err := spec.method(store, spec.start, spec.end) + if spec.expError != nil { + require.True(t, spec.expError.Is(err), "expected #+v but got #+v", spec.expError, err) + return + } + require.NoError(t, err)contrib/x/group/internal/orm/auto_uint64_test.go (1)
152-160: Nil-pointer risk identical toindex_test.goThe test repeats the unsafe call pattern and will panic on success cases.
- it, err := spec.method(store, spec.start, spec.end) - require.True(t, spec.expError.Is(err), "expected #+v but got #+v", spec.expError, err) - if spec.expError != nil { - return - } + it, err := spec.method(store, spec.start, spec.end) + if spec.expError != nil { + require.True(t, spec.expError.Is(err), "expected #+v but got #+v", spec.expError, err) + return + } + require.NoError(t, err)contrib/x/group/migrations/v2/gen_state_test.go (1)
57-76: Add the missing v3 migration packageThe v2 gen_state tests reference
v3.GroupPolicyTablePrefixandv3.MigrateGenState, but there is nocontrib/x/group/migrations/v3directory or package in the repo. You need to provide the v3 migration code (and corresponding constants) before those tests can pass.Please address the following:
- Create
contrib/x/group/migrations/v3/
• DefineGroupPolicyTablePrefixand any other needed constants.
• ImplementMigrateGenState(and, if applicable, aMigrate2to3migration in keeper migrations).- Verify imports in
contrib/x/group/migrations/v2/gen_state_test.goare correct once the v3 package exists.- Update module-level migration chain (
keeper/migrations.go) to include the new v2→v3 step.- Document the v2→v3 migration in your README or migration docs.
Affected file:
- contrib/x/group/migrations/v2/gen_state_test.go (lines 57 & 76)
♻️ Duplicate comments (3)
contrib/x/evidence/genesis.go (1)
7-10: Consistent, meaningful aliasingAs elsewhere, consider switching from
types2to a clearer alias (evidencetypes) for better readability and grep-ability throughout the migrated module.contrib/x/evidence/keeper/msg_server.go (1)
8-9: Alias naming nitSame alias-naming observation as above; replacing
types2with something explicit (e.g.evidencetypes) will make stack traces and IDE tooltips clearer.contrib/x/evidence/keeper/keeper.go (1)
17-19: Alias readabilityRepeating the alias-readability nit for the keeper file to keep the discussion local to the change.
🧹 Nitpick comments (50)
Makefile (2)
394-395: Echo suppression looks good – consider turninglint-installinto a prerequisite insteadAdding the
@keeps the console output tidy while still surfacing any error from the subshell, so the change is fine.
For even cleaner semantics, you could declarelint-installas a dependency of thelinttarget instead of invoking it inside the recipe:-lint: - @echo "--> Running linter on all files" - @$(MAKE) lint-install - @./scripts/go-lint-all.bash --timeout=15m +lint: lint-install + @echo "--> Running linter on all files" + @./scripts/go-lint-all.bash --timeout=15mThat lets Make decide whether
lint-installneeds to run and avoids an extra nestedmakecall.
400-401: Same optimisation opportunity forlint-fixThe
@prefix is fine here as well. Consider the same dependency-based approach:-lint-fix: - @echo "--> Running linter" - @$(MAKE) lint-install - @./scripts/go-lint-all.bash --fix +lint-fix: lint-install + @echo "--> Running linter" + @./scripts/go-lint-all.bash --fixRemoves the redundant nested
makeand keeps output quiet.contrib/x/circuit/keeper/keeper.go (1)
10-12: Questionable aliastypes2– consider renamingThe alias
types2is slightly obscure and may confuse future readers. Unless there is a conflictingtypesimport in this file, prefer a clearer alias such ascircuittypesor simplytypes.-import ( - ... - types2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types" +import ( + ... + circuittypes "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types" )Then adjust usages accordingly.
contrib/x/circuit/keeper/query_test.go (2)
8-10: Avoid cryptictypes2alias – use descriptive alias insteadThe
types2alias gives no hint that the package holds circuit-module proto types. A descriptive alias such ascircuittypes(or drop the alias entirely) improves readability and prevents the “types / types2 / types3 …” escalation we’ve seen in previous PRs.- types2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types" + circuittypes "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types"
27-30: Use enum identifier instead of magic number3Hard-coding the numeric enum value makes the test brittle; if the proto enum ordering ever changes the test will silently mis-assert. Prefer the generated constant:
- require.Equal(t, res.Permission.Level, types2.Permissions_Level(3)) + require.Equal(t, res.Permission.Level, types2.Permissions_LEVEL_SUPER_ADMIN)contrib/x/circuit/keeper/msg_server_test.go (1)
11-13: Replace generictypes2with semantic aliasSame remark as in
query_test.go: renametypes2to something meaningful (circuittypes). This will greatly ease grepping and future onboarding.contrib/x/circuit/module.go (1)
20-22: Alias names here leak to every importer – make them meaningful
keeper2/types2propagate through re-exports (AppModule,ProvideModule). Using semantic aliases (e.g.circuitkeeper,circuittypes) keeps downstream files cleaner and avoids the numeric suffix antipattern.- keeper2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/keeper" - types2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types" + circuitkeeper "github.com/cosmos/cosmos-sdk/contrib/x/circuit/keeper" + circuittypes "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types"contrib/x/circuit/keeper/query.go (1)
6-11: Follow the same aliasing guidelineFor consistency with other comments, rename
types2tocircuittypesand update the_implementation assertion.No functional change – purely readability.
contrib/x/nft/keeper/nft_batch_test.go (2)
61-69: Fix typos in test-case descriptions
"faild"is misspelled in three test-case titles. While this does not break the tests, it pollutes greps and dashboards.- "faild with repeated nft", + "failed with repeated nft",Apply the same correction for the other two occurrences.
Also applies to: 73-81, 85-97
88-91: Possible test flakiness due to non-deterministicrand.Intn
rand.Intnuses the global RNG without a seed, so the failing-path test will produce a different index on every run. If the underlying keeper logic ever becomes order–sensitive this may introduce nondeterministic failures. Consider seeding with a constant or removing randomness altogether.contrib/x/nft/keeper/keys.go (1)
54-64: Nit: small allocation optimisationBuilding the composite key performs four separate
copycalls. Abytes.Join(or a simpleappendchain) can be clearer and allocate slightly less, but this is purely cosmetic given the hot-path nature of key construction is low-level.No change required if readability is preferred.
contrib/x/nft/README.md (1)
7-8: Clarify maintenance statusThe new banner states the package “is not actively maintained by Interchain Labs”. If another team (e.g. community contributors) is expected to maintain it, adding a short sentence with the point-of-contact or governance process would help users decide whether to depend on the module.
contrib/x/nft/simulation/decoder_test.go (1)
38-44: Avoid shadowing the importednftpackage
nft := nft.NFT{ ... }re-assigns the identifiernft, shadowing the package alias and preventing further package references in the same scope. Rename the value tonftObj(or similar) to improve readability and avoid accidental misuse.- nft := nft.NFT{ + nftObj := nft.NFT{contrib/x/nft/simulation/decoder.go (1)
8-9: Align alias naming with the rest of the contrib NFT code for consistencyOther sub-packages (
keeper,simulation/genesis.go, tests) import the same path with thenft2alias, while this file relies on the defaultnftalias. Mixing aliases slows grepping and can be confusing during large sweeping refactors.Consider adopting a single convention (either drop the alias everywhere or always use
nft2).
If you decide on the latter, the change is a one-liner:- "github.com/cosmos/cosmos-sdk/contrib/x/nft" + nft2 "github.com/cosmos/cosmos-sdk/contrib/x/nft"contrib/x/nft/simulation/genesis.go (1)
8-8: Alias choice diverges from neighbouring filesThis file aliases the import as
nft2, whilesimulation/decoder.gouses the default alias.
Pick one style across the module to keep the codebase uniform.contrib/x/nft/keeper/msg_server_test.go (1)
6-7: Minor: unify NFT package alias across keeper testsMost keeper files alias the contrib NFT package as
nft2; tests follow the same pattern here, which is good.
Make sure any remaining test files inkeeper/andmodule/use the identical alias to avoid import churn later.contrib/x/nft/keeper/keeper.go (1)
8-9: Consistent aliasingSame note as in other files: decide whether to keep
nft2or drop the alias for readability. Otherwise the Keeper constructor updates look correct.Also applies to: 15-25
contrib/x/nft/keeper/keeper_test.go (1)
51-74: ResetencodedAddrsslice inSetupTestto avoid data leakage between test cases
SetupTestruns before every test method, buts.encodedAddrsis only appended to, never reset.
Because the sameTestSuiteinstance is reused, the slice grows across test cases and may introduce order-sensitive flakiness.func (s *TestSuite) SetupTest() { // suite setup s.addrs = simtestutil.CreateIncrementalAccounts(3) + s.encodedAddrs = nil // ensure clean state for each test run s.encCfg = moduletestutil.MakeTestEncodingConfig(module.AppModuleBasic{})contrib/x/nft/keeper/grpc_query_test.go (1)
52-57: Avoid invoking other test methods inside malleate blocksCalling
s.TestMint()/s.TestSaveClass()from within other tests couples them implicitly and hides the required fixture logic.
This breaks the “tests should be independent” rule and makes failures harder to trace.Extract the shared setup into helper functions (e.g.
s.seedDefaultClassAndNFT()) and call those helpers instead of full test methods.Also applies to: 141-146, 206-210
contrib/x/nft/keeper/class.go (1)
18-21: Update log / error messages to match new package nameThe strings
"Marshal nft.Class failed"still reference the old package.
Replacing them prevents confusion during debugging.- return errors.Wrap(err, "Marshal nft.Class failed") + return errors.Wrap(err, "marshal Class failed")(similar change below)
Also applies to: 33-34
contrib/x/evidence/simulation/genesis.go (1)
6-8: Prefer a descriptive or no alias for the evidence types packageThe alias
types2is rather opaque and inconsistent with the rest of the codebase (most files import the same path as plaintypes). Consider either:
- Dropping the alias altogether (
types) – the import path is already specific toevidence.- Using a meaningful alias such as
evtypes.This keeps call-sites readable and avoids “types / types2” drift throughout the module.
contrib/x/evidence/keeper/msg_server_test.go (1)
6-32: Unify the evidence types alias across the codebaseHere the contrib evidence package is aliased as
types2, while other files either omit the alias or use a different one. Mixing aliases (types,types2, etc.) adds mental overhead when grepping and reviewing. Recommend standardising on a single convention (ideally plaintypesorevtypes) across all tests and production code.contrib/x/evidence/types/evidence_test.go (1)
14-65: Consistent package aliasing improves test readabilitySimilar to the keeper tests, this file aliases the contrib evidence package as
types2. For readability and easier searchability, consider dropping the numeric suffix and using a uniform alias (typesorevtypes) throughout the module.contrib/x/evidence/simulation/genesis_test.go (1)
14-16: Align test imports with the agreed alias conventionThe test imports the contrib evidence package as
types2. For the sake of consistency with the rest of the codebase (and to avoidtypesvstypes2confusion), use a single, descriptive alias everywhere.contrib/x/evidence/keeper/keeper_test.go (3)
18-23: Prefer explicit package alias over numeric suffixUsing the generic alias
types2makes the codebase harder to scan.
Consider a descriptive alias such asevidencetypesto convey intent and avoid the “magic number” look.-import ( - … - types2 "github.com/cosmos/cosmos-sdk/contrib/x/evidence/types" -) +import ( + … + evidencetypes "github.com/cosmos/cosmos-sdk/contrib/x/evidence/types" +)
52-58: Remove unused parameter in helper factory
testEquivocationHandleraccepts an argument that is never referenced.
Dropping it eliminates a false signal to future readers that the value matters.-func testEquivocationHandler(_ interface{}) types2.Handler { +func testEquivocationHandler() types2.Handler {Corresponding call site:
-router = router.AddRoute(types2.RouteEquivocation, testEquivocationHandler(evidenceKeeper)) +router = router.AddRoute(types2.RouteEquivocation, testEquivocationHandler())
120-121: Avoid redundant encoder setup
MakeTestEncodingConfigis invoked twice inSetupTest.
Re-using the result from Line 87 keeps the test lighter and avoids hidden divergence if defaults ever change.contrib/x/nft/keeper/genesis.go (4)
3-8: Drop thenft2alias – keep the familiarnftidentifier for readabilityThe old path is gone, so there is no longer a clash. Using the
nft2alias forces every type reference in this file (and many others) to carry an unnecessary suffix, which hurts readability and sets up a precedent that new‐module code must diverge from the logical domain name.A simple alias-less import avoids this:
-import ( - "sort" - - nft2 "github.com/cosmos/cosmos-sdk/contrib/x/nft" - sdk "github.com/cosmos/cosmos-sdk/types" -) +import ( + "sort" + + "github.com/cosmos/cosmos-sdk/contrib/x/nft" + sdk "github.com/cosmos/cosmos-sdk/types" +)…and then drop the
nft2.prefix throughout the file.
No functional change, just cleaner code.
33-50: Same alias readability issue propagatesAll occurrences of
nft2.*can be reverted to plainnft.*once the alias is dropped (see earlier comment).
35-48: Minor: consider storing value slices instead of pointers
nftMapismap[string][]*nft2.NFT. Unless there is a strong need for mutation after export, storing value slices ([]nft.NFT) avoids heap allocations & pointer chasing:-nftMap := make(map[string][]*nft.NFT) +// store by value; GenesisState already contains []*NFT so conversion happens once +nftMap := make(map[string][]nft.NFT)Not critical, but can reduce GC pressure in large genesis states.
58-68: Nit: pre-allocateentrieswith deterministic order already availableYou already have
ownerssorted. You can initialiseentrieswith exact capacity and fill by index, avoiding repeated slice growth:-entries := make([]*nft.Entry, 0, len(nftMap)) -for _, owner := range owners { - entries = append(entries, &nft.Entry{ +entries := make([]*nft.Entry, len(owners)) +for i, owner := range owners { + entries[i] = &nft.Entry{ Owner: owner, Nfts: nftMap[owner], - }) + } }Micro-optimisation, but cheap to apply.
contrib/x/group/README.md (3)
7-8: Clarify maintenance status wordingThe new note says the package is “not actively maintained by Interchain Labs”.
Consider briefly indicating who is expected to maintain it (e.g. community-maintained) or that it’s provided “as-is”. This helps downstream consumers better understand support expectations.
2120-2129: Invalid JSON example due to trailing commaThe JSON snippet ends with a trailing comma after the last field, which is not valid JSON and may confuse readers/tools that copy-paste it.
- "vote_option_context": "", -} + "vote_option_context": "" +}
2138-2143: Same trailing comma issue in Vote metadata exampleRemove the trailing comma after
"justification": ""to keep the snippet valid JSON.contrib/x/group/internal/orm/indexer_test.go (1)
15-16: Avoid name collision with the stdliberrorspackageImporting the contrib errors package unaliased as
errorsmakes it easy to confuse witherrorsfrom the standard library (you already usestdErrorsabove for the latter).- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"and update uses (
errors.ErrORM...→grouperrors.ErrORM...).
This tiny alias greatly improves readability and prevents accidental mix-ups.contrib/x/group/internal/orm/indexer.go (1)
7-8: Alias contrib errors package to avoid confusionSame collision concern as in the test file: importing as plain
errorsshadows the stdlib package and hurts readability.- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"Then replace usages (
errors.ErrORMInvalidArgument,errors.ErrORMUniqueConstraint) withgrouperrors.*.Not mandatory, but it is a low-effort quality improvement.
contrib/x/group/types.go (1)
12-15: Consistent aliasing for contrib errors & internal packagesTo stay consistent with the suggestion above and improve clarity across the codebase, consider aliasing the contrib errors import:
- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"and update references (
errors.ErrEmpty,errors.ErrInvalid, …).
This keeps a clear distinction fromerrorsmodand stdliberrors.contrib/x/group/internal/orm/table_test.go (1)
16-17: Minor readability polish – alias contrib errorsSame comment on aliasing as in other files. Consistency avoids future confusion:
- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"contrib/x/group/internal/orm/types_test.go (1)
14-18: Avoid shadowing the standarderrorspackageImporting
github.com/cosmos/cosmos-sdk/contrib/x/group/errorsun-aliased introduces a symbol namederrorsthat shadows Go’s standard libraryerrorspackage. This hampers readability and tooling support when both need to be used in the same file.- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"(You would then update uses such as
errors.ErrORMEmptyKey→grouperrors.ErrORMEmptyKey.)contrib/x/group/internal/orm/index_test.go (1)
14-16: Same-name import hides stdliberrorsAs in the previous file, consider aliasing the contrib errors package to avoid clashing with the standard library.
contrib/x/group/internal/orm/auto_uint64_test.go (1)
15-17: Alias the contrib errors packageSame observation—alias to keep the stdlib
errorsavailable and avoid confusion.contrib/x/group/internal/orm/iterator_property_test.go (1)
12-13: Nit: alias contrib errors to prevent shadowingSame minor readability concern as in the other test files.
contrib/x/group/internal/orm/key_codec.go (1)
6-9: Avoid aliasing the contrib errors package to the bare nameerrorsUsing
errorsmod "cosmossdk.io/errors" "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"creates two separate imported identifiers named
errors*(errorsmodanderrors).
errorsshadows the Go standard-library packageerrors, which can be confusing when glancing at the code (e.g. “is this the std-lib or the contrib errors?”).A tiny rename improves readability and eliminates the shadowing risk:
-import ( - "fmt" - errorsmod "cosmossdk.io/errors" - "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" -) +import ( + "fmt" + errorsmod "cosmossdk.io/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" +)(and update the few in-file references).
No functional change, just clearer code.contrib/x/group/internal/math/dec.go (1)
9-12: Same shadowing issue as in key_codec.goFor consistency and to avoid clashing with the std-lib
errors, please give the import an explicit alias, e.g.grouperrors, and update the references:- errorsmod "cosmossdk.io/errors" - "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + errorsmod "cosmossdk.io/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"A quick
goimportsrun after the change will tidy everything.contrib/x/group/internal/orm/orm_scenario_test.go (1)
14-17: Prefer an explicit alias to avoid confusion with std-liberrorsSame recommendation as in the production files – rename the import to something like
grouperrorsto keep the code self-explaining:- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"(and update uses in the test).
contrib/x/group/internal/orm/sequence_test.go (1)
9-12: Alias the contrib errors package for clarity- "github.com/cosmos/cosmos-sdk/contrib/x/group/errors" + grouperrors "github.com/cosmos/cosmos-sdk/contrib/x/group/errors"This keeps the tests consistent with the production code and avoids accidental confusion with the std-lib
errors.contrib/x/group/internal/orm/iterator.go (1)
3-13: Rungo mod tidyafter large-scale path movesAfter sweeping import-path updates across many packages, stale
go.mod/go.sumentries frequently linger. A quickgo mod tidyin the root module will catch any orphaned dependencies introduced (or now unused) by this mass relocation.contrib/x/group/simulation/decoder_test.go (1)
9-14: Test imports updated; ensuregoimportsis runAll simulation test imports now target the new contrib paths, and the obsolete
//nolint:staticcheckannotations were removed—good cleanup. After the bulk move, rungoimports(ormake lint) to keep import ordering/style consistent project-wide.contrib/x/group/proposal_test.go (1)
8-9: Import paths updated correctly but package usage is inconsistent.The import paths have been correctly updated to use the new
contrib/x/grouplocation. However, the package references in the test code use directgroup.Proposalinstead of an alias likegroup2.Proposalas seen in other files in this migration.For consistency with the broader migration pattern, consider using an alias:
- "github.com/cosmos/cosmos-sdk/contrib/x/group" - "github.com/cosmos/cosmos-sdk/contrib/x/group/module" + group2 "github.com/cosmos/cosmos-sdk/contrib/x/group" + "github.com/cosmos/cosmos-sdk/contrib/x/group/module"And update the references accordingly:
- p1 := group.Proposal{Proposers: []string{"foo"}} - p2 := group.Proposal{Proposers: []string{"bar"}} + p1 := group2.Proposal{Proposers: []string{"foo"}} + p2 := group2.Proposal{Proposers: []string{"bar"}}contrib/x/group/simulation/decoder.go (1)
8-9: Import paths updated correctly but consider consistent aliasing.The import paths have been correctly updated to the new
contrib/x/grouplocations. However, for consistency with the broader migration pattern seen in other files, consider using thegroup2alias.- "github.com/cosmos/cosmos-sdk/contrib/x/group" + group2 "github.com/cosmos/cosmos-sdk/contrib/x/group"Then update the type references throughout the file:
- var groupA, groupB group.GroupInfo + var groupA, groupB group2.GroupInfo
| types2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| "github.com/cosmos/cosmos-sdk/x/circuit/types" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer a clearer alias than types2 for maintainability
types2 gives no hint about what the imported package represents and will feel awkward throughout the module.
Unless this file also imports another types package, use either the canonical types alias or something explicit like circuittype. This removes mental overhead for future readers.
-import (
- …
- types2 "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types"
- …
-)
+import (
+ …
+ circuittypes "github.com/cosmos/cosmos-sdk/contrib/x/circuit/types"
+ …
+)Follow-up: rename every types2. reference in this file (and its tests) accordingly.
🤖 Prompt for AI Agents
In contrib/x/circuit/keeper/msg_server.go around lines 12 to 15, the alias
`types2` for the imported package is unclear and reduces code readability.
Rename the alias to a more descriptive name such as `circuittype` or simply
`types` if no other `types` package is imported. Then update all references to
`types2.` throughout this file and its tests to use the new alias consistently.
| for _, nftID := range nftIDs { | ||
| if !k.HasNFT(ctx, classID, nftID) { | ||
| return errors.Wrap(nft.ErrNFTNotExists, nftID) | ||
| return errors.Wrap(nft2.ErrNFTNotExists, nftID) | ||
| } | ||
| if err := k.transferWithNoCheck(ctx, classID, nftID, receiver); err != nil { | ||
| return errors.Wrap(nft.ErrNFTNotExists, nftID) | ||
| return errors.Wrap(nft2.ErrNFTNotExists, nftID) | ||
| } |
There was a problem hiding this comment.
Incorrect error wrapping masks real failure in BatchTransfer
Any failure returned by transferWithNoCheck is replaced with ErrNFTNotExists, losing the original context (e.g. wrong owner, insufficient balance).
- if err := k.transferWithNoCheck(ctx, classID, nftID, receiver); err != nil {
- return errors.Wrap(nft2.ErrNFTNotExists, nftID)
- }
+ if err := k.transferWithNoCheck(ctx, classID, nftID, receiver); err != nil {
+ return err // propagate the actual cause
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, nftID := range nftIDs { | |
| if !k.HasNFT(ctx, classID, nftID) { | |
| return errors.Wrap(nft.ErrNFTNotExists, nftID) | |
| return errors.Wrap(nft2.ErrNFTNotExists, nftID) | |
| } | |
| if err := k.transferWithNoCheck(ctx, classID, nftID, receiver); err != nil { | |
| return errors.Wrap(nft.ErrNFTNotExists, nftID) | |
| return errors.Wrap(nft2.ErrNFTNotExists, nftID) | |
| } | |
| for _, nftID := range nftIDs { | |
| if !k.HasNFT(ctx, classID, nftID) { | |
| return errors.Wrap(nft2.ErrNFTNotExists, nftID) | |
| } | |
| if err := k.transferWithNoCheck(ctx, classID, nftID, receiver); err != nil { | |
| return err // propagate the actual cause | |
| } |
🤖 Prompt for AI Agents
In contrib/x/nft/keeper/nft_batch.go around lines 80 to 86, the error returned
by transferWithNoCheck is incorrectly wrapped with ErrNFTNotExists, which masks
the real failure cause. To fix this, wrap the error returned by
transferWithNoCheck directly without replacing it, preserving the original error
context for accurate debugging.
contrib/x/nft/simulation/genesis.go
Outdated
| func genClasses(r *rand.Rand, accounts []simtypes.Account) []*nft2.Class { | ||
| classes := make([]*nft2.Class, len(accounts)-1) | ||
| for i := 0; i < len(accounts)-1; i++ { | ||
| classes[i] = &nft.Class{ | ||
| classes[i] = &nft2.Class{ | ||
| Id: simtypes.RandStringOfLength(r, 10), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent negative slice lengths when the simulator has ≤1 accounts
make([]*nft2.Class, len(accounts)-1) (and the equivalent for entries) panics if len(accounts) is 0 or 1, which is legal in a fuzzed simulation. Add a fast-exit or guard clause before allocating:
-func genClasses(r *rand.Rand, accounts []simtypes.Account) []*nft2.Class {
- classes := make([]*nft2.Class, len(accounts)-1)
+func genClasses(r *rand.Rand, accounts []simtypes.Account) []*nft2.Class {
+ if len(accounts) < 2 {
+ return nil
+ }
+ classes := make([]*nft2.Class, 0, len(accounts)-1)Apply the same pattern to genNFT.
This avoids sporadic panics during go test -run=Simulation.
Also applies to: 28-38
🤖 Prompt for AI Agents
In contrib/x/nft/simulation/genesis.go around lines 14 to 18 and 28 to 38, the
code creates slices with length len(accounts)-1, which causes a panic if
len(accounts) is 0 or 1. To fix this, add a guard clause at the start of
genClasses and genNFT functions that returns early if len(accounts) is less than
or equal to 1, preventing negative slice lengths and avoiding panics during
simulation tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
some stuff we should remove also:
- .github/pr_labeler/yml: has instances of the removed module paths
- simapp/app_test.go: we still use some of the removed modules here. should we axe?
- dependabot.yml: references some dead modules path now
- scripts/mockgen.sh: references defunct module paths
also i think atp we should just remove their protos from the proto dir. if we don't plan on making any changes/maintaining these modules, its probably best to just clean out that directory as to not post these to buf.build (if we do that)
tests/e2e/group/suite.go
Outdated
There was a problem hiding this comment.
do we still want these tests at all?
|
|
## Overview This PR updates the previously removed dead link to `x/circuit` in the Cosmos SDK docs. As suggested in [this review](#5616 (review)), instead of deleting the link we can point it to the correct location. The `x/circuit` module was not removed entirely but moved to `contrib/x/circuit` in the Cosmos SDK ([#25090](cosmos/cosmos-sdk#25090)). Therefore, the documentation now references the updated path: `https://github.com/cosmos/cosmos-sdk/tree/main/contrib/x/circuit` This way, the context of the original ADR remains intact while avoiding a broken link. #5616
Description
move the following modules to contrib
Remove said apps from
app.goused in testingMove proto generation paths and validate
Remove dead
x/crisisproto (module was removed in v53Remove dead migration code in
./contrib/migrateSummary by CodeRabbit