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

bump up mods.irisnet.org/modules/nft to cosmos-sdk v0.50.10 #451

Merged
merged 21 commits into from
Nov 13, 2024

Conversation

wangjiulian
Copy link
Collaborator

@wangjiulian wangjiulian commented Nov 11, 2024

Closed: #441

Summary by CodeRabbit

Release Notes

  • New Features

    • Updated import paths for NFT-related functionalities to enhance integration with the new service-oriented architecture.
    • Introduced a new method AddressCodec() in the AccountKeeper interface for improved address handling.
  • Bug Fixes

    • Enhanced error handling for unsupported class metadata in NFT processing.
  • Documentation

    • Updated test cases to reflect new import paths and ensure consistency with the updated NFT module structure.
  • Refactor

    • Transitioned from using sdk.Context to context.Context in key interfaces, streamlining context management.
    • Replaced storeKey with storeService across various modules for improved store access.
    • Simplified function signatures in simulation operations by removing unnecessary parameters.
  • Chores

    • Upgraded dependencies to their latest versions for better performance and security.

Copy link

coderabbitai bot commented Nov 11, 2024

Walkthrough

The changes in this pull request involve significant updates to the modules/nft directory, transitioning from the Cosmos SDK to the Cosmossdk.io structure. Key modifications include updating import paths, changing data types from specific keys to service-oriented types for store interactions, and upgrading the Go version and dependencies in the go.mod file. The changes enhance the flexibility and modularity of the NFT module's architecture while maintaining existing functionalities across various components.

Changes

File Path Change Summary
modules/nft/depinject.go Updated import from github.com/cosmos/cosmos-sdk/store/types to cosmossdk.io/core/store. Changed Key field to StoreService in Inputs struct and updated ProvideModule function accordingly.
modules/nft/go.mod Upgraded Go version from 1.19 to 1.21, updated dependencies including cosmossdk.io/api, cosmossdk.io/core, and github.com/cosmos/cosmos-sdk. Added new dependencies and modified replace directives.
modules/nft/keeper/denom.go Changed import from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft. Updated methods to use new import without changing core logic.
modules/nft/keeper/depinject_test.go Removed imports for capabilitymodulev1 and capabilitytypes. Added imports for evidencetypes, feegrant, and upgradetypes. Updated module initialization order.
modules/nft/keeper/grpc_query.go Updated import from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft. Logic for querying NFTs remains unchanged.
modules/nft/keeper/keeper.go Replaced storeKey with storeService in Keeper struct. Updated constructor function NewKeeper to use storeService.
modules/nft/keeper/keeper_test.go Removed tmproto import and simplified context instantiation in SetupTest method.
modules/nft/keeper/migrations.go Updated Migrate1to2 method to replace storeKey with storeService for migration process.
modules/nft/keeper/nft.go Changed import from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft. Core logic for NFT management remains unchanged.
modules/nft/migrations/v2/keeper.go Replaced storeKey with storeService and changed cdc from codec.Codec to codec.BinaryCodec. Updated methods for new store access method.
modules/nft/migrations/v2/store.go Transitioned from cometbft logging/store to cosmossdk.io. Updated Migrate and migrateToken functions to use storeService.
modules/nft/module.go Updated NFT-related imports to cosmossdk.io. Changed RegisterStoreDecoder method signature.
modules/nft/simulation/operations.go Removed cdc parameter from WeightedOperations and simplified return statements in simulation functions.
modules/nft/types/builder.go Changed import from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft. Enhanced handling of NFT and class metadata in ClassBuilder and TokenBuilder.
modules/nft/types/builder_test.go Updated import path for NFT-related types. Adjusted test functions to utilize new import paths.
modules/nft/types/expected_keepers.go Updated AccountKeeper and BankKeeper interfaces to use context.Context instead of sdk.Context. Added AddressCodec() method to AccountKeeper.

Possibly related issues

Possibly related PRs

Suggested reviewers

  • dreamer-zq
  • mitch1024

🐰 "In the meadow where the code does flow,
Changes abound, and new paths we sow.
From keys to services, a leap we take,
With every update, a new dawn we make.
Hopping through imports, we dance with glee,
In the world of NFTs, come join me!" 🌼

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 golangci-lint

level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar."
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7dc2263 and b2ae56d.

📒 Files selected for processing (1)
  • modules/nft/go.mod (2 hunks)
🔇 Additional comments (2)
modules/nft/go.mod (2)

6-29: LGTM! Dependencies properly aligned with cosmos-sdk v0.50.10

The direct dependencies have been correctly updated to support cosmos-sdk v0.50.10, including the necessary cosmossdk.io packages. This aligns well with the transition to the new Cosmossdk.io structure.


216-217: Verify the simapp replacement path

The TODO comment and relative path replacement for simapp needs verification:

// todo replace version
mods.irisnet.org/simapp => ../../simapp

Please ensure that:

  1. The relative path ../../simapp is valid in all build environments
  2. The TODO is addressed with a specific version

Let's verify the simapp directory existence and its go.mod file:


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • 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/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@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.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (14)
modules/nft/keeper/keeper.go (1)

18-20: LGTM: Clean transition to service-oriented store access

The replacement of storeKey with storeService represents a good architectural improvement, providing better abstraction through the service-oriented approach.

modules/nft/depinject.go (1)

Line range hint 1-62: Consider documenting architectural changes and performance implications

The transition to a service-based store architecture is a significant change that:

  1. Improves modularity and maintainability
  2. Aligns with Cosmos SDK v0.50.x architecture
  3. May have different performance characteristics

Consider:

  • Adding documentation about the architectural changes
  • Conducting performance testing to ensure no regression
  • Updating integration tests to cover the new service-based interactions
modules/nft/keeper/denom.go (1)

Line range hint 15-156: Implementation maintains compatibility with new SDK version.

The methods correctly use the nft.Class type from the new import while preserving their original functionality. The error handling and data structures remain appropriate for the SDK version upgrade.

Consider adding integration tests to verify the compatibility with the new SDK version, especially focusing on:

  1. Cross-module interactions
  2. State migrations
  3. Blockchain upgrades
modules/nft/keeper/grpc_query.go (1)

Line range hint 89-157: Consider adding context timeout handling.

The Collection method performs multiple keeper operations. Consider adding context timeout handling to prevent long-running queries.

Example improvement:

 func (k Keeper) Collection(c context.Context, request *types.QueryCollectionRequest) (*types.QueryCollectionResponse, error) {
+    // Add timeout to prevent long-running queries
+    ctx, cancel := context.WithTimeout(c, 10*time.Second)
+    defer cancel()
+
     ctx := sdk.UnwrapSDKContext(c)
     denom, err := k.GetDenomInfo(ctx, request.DenomId)
     if err != nil {
modules/nft/module.go (1)

Line range hint 54-57: Consider documenting the dual NFT implementation strategy.

The module implements both custom NFT types and cosmos-sdk NFT types, as evidenced by the dual registration in both RegisterGRPCGatewayRoutes and RegisterServices. Consider adding documentation to clarify:

  • The relationship between custom and SDK NFT implementations
  • When to use each implementation
  • Migration path for users

Also applies to: 115-118

modules/nft/keeper/keeper_test.go (1)

Line range hint 1-400: Consider adding integration tests for store service interactions

While the basic test setup is maintained, given the transition to a service-oriented approach for store interactions (as mentioned in the AI summary), consider adding test cases that specifically verify the store service behavior.

modules/nft/keeper/depinject_test.go (2)

Line range hint 52-56: Remove outdated capability module comment

The comment about the capability module needing to occur first is no longer relevant as the capability module has been removed.

- // NOTE: Capability module must occur first so that it can initialize any capabilities
- // so that other modules that want to create or claim capabilities afterwards in InitChain
- // can do so safely.

Based on the analysis of the code and search results, I can now provide a final response:

The module initialization order in this PR is incorrect and needs adjustment

The current module order in the PR does not satisfy two critical initialization requirements:

  1. The genutils module (genutiltypes.ModuleName) appears after staking module (stakingtypes.ModuleName) which is correct.

  2. However, the capability module is missing entirely from the initialization sequence, while other test files in the codebase indicate two important requirements:

    • "Capability module must occur first so that it can initialize any capabilities"
    • "Capability module's beginblocker must come before any modules using capabilities"

The module order should be updated to include the capability module at the beginning of the sequence.

🔗 Analysis chain

Line range hint 61-65: Verify module initialization order dependencies

The module order has been updated with new modules (evidence, feegrant, upgrade). Please verify that this order respects all inter-module dependencies, particularly for the newly added modules.


Based on the search results, I found two key initialization order requirements:

  1. "The genutils module must occur after staking so that pools are properly initialized"
  2. "Capability module's beginblocker must come before any modules using capabilities (e.g. IBC)"

Let me verify if these requirements are satisfied in the current module order by checking the full module initialization sequence.


Let me check if there are any capability module references in the codebase to verify if the second requirement is applicable.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any module initialization order requirements in Go docs and comments

# Look for initialization order requirements in comments
rg -i "must (occur|be initialized|come) (before|after)" --type go

# Look for potential dependency hints in init functions
ast-grep --pattern 'func init() {
  $$$
}'

Length of output: 270198


Script:

#!/bin/bash
# Get the full module initialization sequence
rg -A 10 "^[[:space:]]*authtypes.ModuleName" modules/nft/keeper/depinject_test.go

Length of output: 1305


Script:

#!/bin/bash
# Check for capability module references
rg -i "capability" --type go

Length of output: 8658

modules/nft/types/builder_test.go (1)

Line range hint 15-279: Test implementation looks good with SDK v0.50.10.

The test cases properly handle the new NFT types and maintain compatibility with the upgraded SDK. All core functionalities are well-tested:

  • Token building and metadata handling
  • Class operations with various data inputs
  • IBC asset scenarios
  • Proper encoding registration

Consider adding test cases for any new features or behaviors introduced in SDK v0.50.10, such as:

  • New validation rules if any
  • Changes in metadata handling
  • Updated IBC compatibility features
modules/nft/types/builder.go (1)

Line range hint 71-73: Consider enhancing error messages for type assertions.

While the error handling is functional, consider making the error messages more specific to help with debugging:

-		return "", errors.New("unsupport classMetadata")
+		return "", fmt.Errorf("expected *DenomMetadata, got %T", message)

-		return "", errors.New("unsupport classMetadata")
+		return "", fmt.Errorf("expected *NFTMetadata, got %T", message)

Also applies to: 190-192

modules/nft/go.mod (1)

218-220: Address TODO comment for simapp replacement

The TODO comment for simapp replacement should be tracked and addressed.

Would you like me to:

  1. Create a GitHub issue to track this TODO?
  2. Help specify the correct version for the simapp replacement?
modules/nft/simulation/operations.go (1)

Line range hint 195-560: Consider refactoring common error handling patterns

The simulation functions contain duplicated error handling logic for transaction generation and delivery. Consider extracting this into a helper function to improve maintainability and reduce code duplication.

Example refactor:

func handleSimulationTx(
    r *rand.Rand,
    app *baseapp.BaseApp,
    msg sdk.Msg,
    account sdk.AccountI,
    simAccount simtypes.Account,
    fees sdk.Coins,
    chainID string,
    eventType string,
) (simtypes.OperationMsg, []simtypes.FutureOperation, error) {
    txGen := moduletestutil.MakeTestEncodingConfig().TxConfig
    tx, err := simtestutil.GenSignedMockTx(
        r,
        txGen,
        []sdk.Msg{msg},
        fees,
        simtestutil.DefaultGenTxGas,
        chainID,
        []uint64{account.GetAccountNumber()},
        []uint64{account.GetSequence()},
        simAccount.PrivKey,
    )
    if err != nil {
        return simtypes.NoOpMsg(
            types.ModuleName,
            msg.Type(),
            "unable to generate mock tx",
        ), nil, err
    }

    if _, _, err = app.SimDeliver(txGen.TxEncoder(), tx); err != nil {
        return simtypes.NoOpMsg(
            types.ModuleName,
            eventType,
            err.Error(),
        ), nil, err
    }

    return simtypes.NewOperationMsg(msg, true, ""), nil, nil
}
modules/nft/migrations/v2/store.go (1)

91-101: Refactor iterator initialization and closure in migrateToken for clarity

Consider simplifying the iterator handling in migrateToken by initializing the iterator where it's assigned and deferring its closure immediately. This enhances readability and ensures proper resource management.

Apply this diff to refactor the iterator handling:

 func migrateToken(
 	ctx sdk.Context,
 	k keeper,
 	logger log.Logger,
 	denomID string,
 ) (int64, error) {
-	var iterator storetypes.Iterator
-	defer func() {
-		if iterator != nil {
-			_ = iterator.Close()
-		}
-	}()
 
 	store := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
+	iterator := storetypes.KVStorePrefixIterator(store, KeyNFT(denomID, ""))
+	defer iterator.Close()
 
 	total := int64(0)
-	iterator = storetypes.KVStorePrefixIterator(store, KeyNFT(denomID, ""))
 	for ; iterator.Valid(); iterator.Next() {
 		var baseNFT types.BaseNFT
 		k.cdc.MustUnmarshal(iterator.Value(), &baseNFT)
modules/nft/migrations/v2/keeper.go (1)

62-62: Consider refactoring store initialization into a helper method

The initialization of the store using runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)) is repeated across multiple methods (setOwner, GetTotalSupply, updateTotalSupply, getClassStoreByOwner, and getNFTStore). To improve code maintainability and reduce duplication, consider creating a helper method within the keeper struct to handle store retrieval.

For example, you can add the following method to the keeper:

func (k keeper) getStore(ctx sdk.Context) sdk.KVStore {
    return runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
}

Then, update the methods to use this helper:

-func (k keeper) setOwner(ctx sdk.Context, classID, nftID string, owner sdk.AccAddress) {
-    store := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
+func (k keeper) setOwner(ctx sdk.Context, classID, nftID string, owner sdk.AccAddress) {
+    store := k.getStore(ctx)

This refactoring enhances code readability and simplifies future updates.

Also applies to: 76-76, 82-82, 88-88, 94-94

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a2bb6fe and 7dc2263.

⛔ Files ignored due to path filters (1)
  • modules/nft/go.sum is excluded by !**/*.sum
📒 Files selected for processing (16)
  • modules/nft/depinject.go (3 hunks)
  • modules/nft/go.mod (2 hunks)
  • modules/nft/keeper/denom.go (1 hunks)
  • modules/nft/keeper/depinject_test.go (2 hunks)
  • modules/nft/keeper/grpc_query.go (1 hunks)
  • modules/nft/keeper/keeper.go (1 hunks)
  • modules/nft/keeper/keeper_test.go (1 hunks)
  • modules/nft/keeper/migrations.go (1 hunks)
  • modules/nft/keeper/nft.go (1 hunks)
  • modules/nft/migrations/v2/keeper.go (3 hunks)
  • modules/nft/migrations/v2/store.go (2 hunks)
  • modules/nft/module.go (2 hunks)
  • modules/nft/simulation/operations.go (7 hunks)
  • modules/nft/types/builder.go (1 hunks)
  • modules/nft/types/builder_test.go (1 hunks)
  • modules/nft/types/expected_keepers.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • modules/nft/keeper/nft.go
🔇 Additional comments (34)
modules/nft/keeper/migrations.go (2)

Line range hint 1-21: Verify other migration functions for consistency

Since this is part of a larger architectural change, we should ensure all migration-related code is consistently using the new storeService pattern.

#!/bin/bash
# Description: Check for other migration functions that might need similar updates
# Expected: All migration functions should use storeService instead of storeKey

# Look for other migration functions in the NFT module
ast-grep --pattern 'func $_ Migrate$_to$_($$$) $_ {
  $$$
}'

# Check for any remaining uses of storeKey in migration code
rg -g "**/migrations/**/*.go" "storeKey"

21-21: LGTM! Architectural improvement aligned with Cosmos SDK v0.50.x

The change from storeKey to storeService aligns with the Cosmos SDK's transition to a more service-oriented architecture in v0.50.x.

Let's verify the migration testing coverage:

✅ Verification successful

Migration test coverage is comprehensive and well-structured

The migration from storeKey to storeService is properly tested with:

  • Comprehensive test coverage in modules/nft/migrations/v2/store_test.go
  • Test scenarios covering all data structures (denoms, NFTs, collections)
  • Data integrity verification through check() function that validates post-migration state
  • Supply invariant checks to ensure consistency
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for migration tests to ensure data integrity
# Expected: Find test files covering the v1->v2 migration

# Look for migration tests
rg -g "**/*test.go" -A 5 "TestMigrate.*1to2|TestMigrator.*1to2"

# Check for any migration-related test helpers
rg -g "**/*test.go" "setupMigration|migrationTest"

Length of output: 2182


Script:

#!/bin/bash
# Let's check for migration tests in the NFT module specifically
rg -g "modules/nft/**/*test.go" -A 10 "TestMigrate|func.*Migrate"

# Also check the v2 migration implementation and tests
fd "store_test.go" modules/nft/migrations/v2/ --exec cat {}

Length of output: 6407

modules/nft/types/expected_keepers.go (2)

4-6: LGTM: Import changes align with cosmos-sdk v0.50.10

The addition of context and cosmossdk.io/core/address imports reflects the SDK's move towards using standard Go contexts and modular address handling.


19-19: Confirm BankKeeper interface reduction is intentional

The interface has been significantly simplified, retaining only SpendableCoins. While this follows the interface segregation principle by keeping only what's needed, we should verify this doesn't break existing functionality.

#!/bin/bash
# Search for any remaining usage of removed methods
rg "GetAllBalances|GetBalance|LockedCoins" --type go

# Search for SpendableCoins usage to ensure context parameter is updated
ast-grep --pattern 'SpendableCoins($_ context.Context, $_)'
modules/nft/keeper/keeper.go (3)

6-9: LGTM: Import paths correctly updated to cosmossdk.io structure

The import changes align with the PR objective of upgrading to cosmos-sdk v0.50.10, following the new cosmossdk.io structure.


25-32: LGTM: Constructor properly updated for store service integration

The constructor changes correctly implement the new store service pattern. However, let's verify the integration with other parts of the codebase.

#!/bin/bash
# Description: Verify that all NewKeeper calls are updated to use store service
# and no old storeKey usage remains

# Check for any remaining storeKey references in keeper initialization
rg -l "storeKey.*NewKeeper" --type go

# Check for updated store service usage pattern
rg -l "storeService.*NewKeeper" --type go

Line range hint 1-45: Verify migration and backward compatibility

Since this is part of a significant SDK upgrade with architectural changes:

  1. Ensure proper migration paths exist for existing chain state
  2. Verify that the new store service maintains the same consistency guarantees
  3. Confirm that existing queries and transactions continue to work as expected
✅ Verification successful

Based on the analysis of the shell script outputs, I can now provide a final response regarding the migration and backward compatibility concerns:

Migration paths and compatibility are properly maintained

The codebase shows proper migration handling:

  1. The NFT module has a clear migration path through Migrate1to2 with store migrations
  2. The change from storeKey to storeService maintains compatibility by:
    • Using the same key prefixes and store structure
    • Properly wrapping the store service with runtime.KVStoreAdapter
    • Maintaining consistent store access patterns

The architectural changes are safe because:

  • All modules register their migrations via RegisterMigration
  • Store keys and prefixes remain unchanged
  • The store service is a thin wrapper around the existing KVStore interface
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for migration handlers and potential breaking changes

# Check for migration handlers
rg -A 5 "Migrate.*to.*" --type go

# Look for potential breaking changes in store access patterns
rg -A 3 "store\.(Get|Set|Delete|Has)" --type go

Length of output: 83


Script:

#!/bin/bash
# Let's verify the migration and compatibility aspects more thoroughly

# Check for any migration-related code
rg -A 5 "Migration|Upgrade" --type go

# Look for store key changes and store access patterns
rg -A 3 "KVStore|store\." --type go

# Check for any breaking changes in interfaces or function signatures
ast-grep --pattern 'type $_ interface {
  $$$
}'

# Look for any test files that might indicate backward compatibility testing
fd -e go -E "test" --exec grep -l "TestMigration\|TestUpgrade\|TestBackward" {}

Length of output: 317179

modules/nft/depinject.go (3)

5-5: LGTM: Import path update aligns with SDK v0.50.x

The migration from github.com/cosmos/cosmos-sdk/store/types to cosmossdk.io/core/store is correct and follows the new modular architecture introduced in Cosmos SDK v0.50.x.


33-35: LGTM: Improved store handling architecture

The transition from KVStoreKey to KVStoreService is a positive architectural change that:

  • Improves modularity and flexibility
  • Follows service-oriented design principles
  • Aligns with Cosmos SDK v0.50.x best practices

Let's verify that this architectural change is consistently applied across the module:

✅ Verification successful

Let me check the actual usage context to ensure the migration is complete and consistent.


Migration to KVStoreService is complete and consistent

The verification shows that:

  • KVStoreService is properly integrated across all relevant components (keeper, migrations, and dependency injection)
  • No remaining references to the old KVStoreKey implementation exist
  • The architectural change has been consistently applied throughout the module
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to KVStoreKey in the module
# and verify consistent usage of KVStoreService

echo "Checking for any remaining KVStoreKey references..."
rg -l "KVStoreKey" modules/nft/

echo "Verifying KVStoreService usage..."
rg -l "KVStoreService" modules/nft/

Length of output: 382


Script:

#!/bin/bash
# Check the actual usage context of both KVStoreKey and KVStoreService
echo "Checking KVStoreService usage context..."
rg "KVStoreService" modules/nft/ -B 2 -A 2

echo -e "\nChecking any remaining KVStoreKey references..."
rg "KVStoreKey" modules/nft/ -B 2 -A 2

Length of output: 1859


54-55: Verify keeper initialization with new store service

The update to use StoreService in the keeper initialization looks correct, but we should verify the keeper's constructor implementation.

Let's check the keeper's constructor implementation:

✅ Verification successful

Let me try a different approach to find the keeper's constructor implementation.


Let me check the nftkeeper implementation since it's being used in the constructor.


Let me try to find the correct path for the nftkeeper package.


Let me try to find the nftkeeper package in a different location since it might be imported from an external module.


Based on the findings, I can now generate the final response:

Keeper initialization with StoreService is correctly implemented

The keeper's constructor properly initializes using the new StoreService pattern:

  • The main keeper correctly receives and stores the StoreService
  • It properly initializes the underlying cosmos-sdk NFT keeper (nftkeeper.NewKeeper) with the StoreService
  • The implementation maintains proper dependency injection through the constructor parameters
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify keeper's constructor implementation with StoreService

echo "Checking keeper's constructor implementation..."
ast-grep --pattern 'func NewKeeper($_) {
  $$$
}' modules/nft/keeper/keeper.go

Length of output: 188


Script:

#!/bin/bash
# Search for NewKeeper function using ripgrep for better context
rg -A 10 "func NewKeeper" modules/nft/keeper/

Length of output: 624


Script:

#!/bin/bash
# Search for the nftkeeper's NewKeeper implementation
rg -A 10 "func NewKeeper" modules/nft/keeper/nftkeeper/

Length of output: 127


Script:

#!/bin/bash
# Find the nftkeeper directory
fd nftkeeper

# Also search for NewKeeper in any Go files
rg -A 10 "func NewKeeper" --type go

Length of output: 6493


Script:

#!/bin/bash
# Check the imports in the NFT keeper file to find where nftkeeper is coming from
rg "nftkeeper" modules/nft/keeper/keeper.go -A 5 -B 5

Length of output: 1034

modules/nft/keeper/denom.go (1)

5-5: LGTM! Import path updated correctly.

The change from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft aligns with the Cosmos SDK v0.50.x modularization.

Let's verify the consistency of this import change across the codebase:

✅ Verification successful

Import path migration is complete and consistent

The verification shows that:

  • No instances of the old import path github.com/cosmos/cosmos-sdk/x/nft remain in the codebase
  • The new import path cosmossdk.io/x/nft is consistently used across all NFT module files
  • The module's dependencies are properly updated in go.mod and go.sum
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old import paths and verify new import usage
echo "Checking for any remaining old cosmos-sdk nft imports..."
rg "github.com/cosmos/cosmos-sdk/x/nft"

echo "Verifying usage of new cosmossdk.io nft imports..."
rg "cosmossdk.io/x/nft"

Length of output: 1169

modules/nft/keeper/grpc_query.go (3)

Line range hint 190-206: LGTM! Error handling follows best practices.

The use of errorsmod.Wrapf with specific error types and descriptive messages is appropriate and follows Cosmos SDK best practices.


Line range hint 36-86: Verify NFT keeper interface compatibility with v0.50.10.

While the code structure remains sound, please ensure:

  1. The NFT keeper interface (k.nk) methods remain compatible with v0.50.10
  2. The response types from the new NFT module are correctly mapped to your custom types
  3. Pagination handling remains consistent with the new SDK version

Let's verify the keeper interface compatibility:

#!/bin/bash
# Description: Check for any breaking changes in the NFT keeper interface
# Expect: Consistent interface usage across the module

# Check NFT keeper interface usage
ast-grep --pattern 'type Keeper struct {
  $$$
  nk $_ 
  $$$
}'

# Verify NFT keeper method calls
rg "k.nk\." -A 2 --type go

7-7: LGTM! Import path update aligns with Cosmos SDK v0.50.x.

The change from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft is correct and follows the Cosmos SDK's modularization efforts.

Let's verify that all NFT module imports are consistently updated:

✅ Verification successful

Import path update is correctly implemented across the NFT module

The verification confirms that:

  • All imports are consistently using the new cosmossdk.io/x/nft path
  • No instances of the old github.com/cosmos/cosmos-sdk/x/nft import remain
  • The module correctly integrates with the new SDK NFT module through the updated imports
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old NFT module imports
# Expect: No matches as all imports should be updated

# Check for any remaining old-style imports
rg "github.com/cosmos/cosmos-sdk/x/nft" --type go

# Verify new-style imports are used consistently
rg "cosmossdk.io/x/nft" --type go

Length of output: 4677


Script:

#!/bin/bash
# Let's check all Go files in the NFT module for imports
fd -e go -p 'modules/nft' --exec cat {} | rg '^import \($|"cosmossdk.io/x/nft"|"github.com/cosmos/cosmos-sdk/x/nft"' -B 1 -A 1

# Also check if there are any references to the old package path in go.mod
rg "github.com/cosmos/cosmos-sdk/x/nft" go.mod

# Check for any potential references in test files
fd -e go -p 'modules/nft.*test' --exec cat {} | rg 'cosmos-sdk/x/nft|cosmossdk.io/x/nft'

Length of output: 2466

modules/nft/module.go (3)

165-167: LGTM! Store decoder registration updated correctly.

The store decoder registration has been properly updated to use simtypes.StoreDecoderRegistry, which aligns with the consolidated simulation types in cosmos-sdk v0.50.x.


Line range hint 147-147: Verify migration path for consensus version 2.

The consensus version is set to 2, and I can see the corresponding migration handler is registered in the RegisterServices method. However, let's ensure all state migrations are properly handled.

Let's check for any additional state changes that might need migration:

#!/bin/bash
# Look for state schema changes that might require migration
echo "Checking for state-related changes..."
rg -A 5 "KVStore|StoreKey|Prefix|keeper" modules/nft/keeper/
rg -A 5 "Proto|Interface" modules/nft/types/

Also applies to: 120-124


8-9: LGTM! Import paths updated correctly.

The import paths have been properly updated to use the new cosmossdk.io paths, which aligns with the SDK's modularization efforts in v0.50.x.

Let's verify that all NFT-related imports are consistently updated across the module:

✅ Verification successful

Let me generate additional verification scripts to check for any remaining cosmos-sdk NFT imports:


Let me check the specific import in the keeper.go file:


All NFT imports have been properly updated to use cosmossdk.io paths

The only remaining cosmos-sdk reference is in a comment describing the return type of the NFTkeeper() method, which is not an actual import. All actual imports in the codebase have been correctly updated to use the new cosmossdk.io/x/nft paths.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining cosmos-sdk NFT imports that should be updated
rg -l "github.com/cosmos/cosmos-sdk/x/nft" modules/nft/

Length of output: 50475


Script:

#!/bin/bash
# Search specifically for any remaining github.com/cosmos/cosmos-sdk/x/nft imports
rg -l "github.com/cosmos/cosmos-sdk/x/nft" modules/nft/

# Check for any other cosmos-sdk NFT related imports that may need updating
rg -l "cosmos-sdk.*nft" modules/nft/

Length of output: 123


Script:

#!/bin/bash
# Check the specific NFT import in keeper.go
rg -A 5 "cosmos-sdk.*nft" modules/nft/keeper/keeper.go

Length of output: 215

modules/nft/keeper/keeper_test.go (1)

77-77: Verify context creation compatibility with cosmos-sdk v0.50.10

The simplified context creation aligns with the SDK update, but let's verify the usage across the codebase.

✅ Verification successful

Based on the search results, I can see that the NFT module's context creation has been updated to the new style (suite.ctx = app.BaseApp.NewContext(isCheckTx)), while other modules are still using the old style with tmproto.Header{}. Let me verify the correct approach in cosmos-sdk v0.50.10.


Let me check the dependencies and imports to understand the context better.


Let me check the test setup and dependencies in the simapp to understand the recommended pattern.


Based on the evidence gathered from multiple searches, I can now provide a final response:

Context creation change in NFT module is correct and aligned with SDK v0.50.x

The simplified context creation app.BaseApp.NewContext(isCheckTx) in the NFT module is correct as it matches the pattern used in the simapp test helpers, which serves as the reference implementation. The removal of tmproto.Header{} parameter aligns with the SDK's evolution towards a simpler API.

  • Other modules still using the old pattern with tmproto.Header{} should be updated to match this new pattern
  • The simapp's test helper confirms this is the correct approach: ctxCheck := app.BaseApp.NewContext(true)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances of NewContext usage to ensure consistent updates
rg -A 2 "BaseApp\.NewContext\(" --type go

# Search for any remaining tmproto.Header usage that might need updating
rg "tmproto\.Header{}" --type go

Length of output: 6220


Script:

#!/bin/bash
# Check go.mod for cosmos-sdk version
cat go.mod | grep cosmos-sdk

# Search for NewContext usage in test files to see the recommended pattern
rg -g "**/*_test.go" "NewContext" --type go

Length of output: 2868


Script:

#!/bin/bash
# Check imports in the NFT keeper test file
cat modules/nft/keeper/keeper_test.go | grep -A 10 "^import"

# Check if there are any other files in the NFT module using NewContext
rg -g "modules/nft/**/*.go" "NewContext" --type go

Length of output: 1107


Script:

#!/bin/bash
# Check simapp test helpers for context creation pattern
cat simapp/test_helpers.go | grep -A 10 "NewContext"

# Check if there's a go.work file that might point to cosmos-sdk version
cat go.work 2>/dev/null || echo "No go.work file found"

Length of output: 545

modules/nft/keeper/depinject_test.go (1)

27-30: LGTM: Import paths updated correctly for cosmos-sdk v0.50.10

The migration to cosmossdk.io paths for evidence, feegrant, and upgrade types is consistent with the SDK upgrade.

modules/nft/types/builder_test.go (1)

7-7: Verify consistent usage of the new NFT import path across the codebase.

The import path change from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft aligns with the SDK upgrade. Let's ensure this change is consistently applied across all files.

✅ Verification successful

Import path migration to cosmossdk.io/x/nft is consistently applied

The verification shows that:

  • No instances of the old import path github.com/cosmos/cosmos-sdk/x/nft remain in the codebase
  • The new import path cosmossdk.io/x/nft is consistently used across all relevant files in the NFT module
  • The module's dependencies are properly updated in go.mod and go.sum files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old NFT import paths and verify new import path usage

echo "Checking for old import paths..."
rg "github.com/cosmos/cosmos-sdk/x/nft"

echo "Verifying new import path usage..."
rg "cosmossdk.io/x/nft"

Length of output: 1089

modules/nft/types/builder.go (2)

Line range hint 63-106: LGTM: Builders maintain compatibility with NFT types.

The BuildMetadata methods in both ClassBuilder and TokenBuilder maintain compatibility with the NFT types from the new import path. The functionality remains unchanged, ensuring a smooth transition.

Also applies to: 182-225


10-10: Verify consistent usage of new NFT import path across the codebase.

The import path change from github.com/cosmos/cosmos-sdk/x/nft to cosmossdk.io/x/nft aligns with cosmos-sdk v0.50.x's modular architecture. Let's ensure this change is consistently applied across the codebase.

✅ Verification successful

Import path migration to cosmossdk.io/x/nft is complete and consistent

The verification shows that:

  • No instances of the old import path github.com/cosmos/cosmos-sdk/x/nft remain in the codebase
  • The new import path cosmossdk.io/x/nft is consistently used across all relevant files in the NFT module
  • The module's dependencies are properly updated in go.mod and go.sum files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old NFT import paths and verify new import usage
echo "Checking for old import paths..."
rg "github.com/cosmos/cosmos-sdk/x/nft"

echo -e "\nVerifying usage of new import path..."
rg "cosmossdk.io/x/nft"

Length of output: 1101

modules/nft/go.mod (3)

34-203: LGTM! Indirect dependencies are properly aligned

The indirect dependency updates are consistent with cosmos-sdk v0.50.10 requirements and use stable versions.


Line range hint 207-216: LGTM! Security-focused replacements

The replace directives appropriately address security concerns:

  • Using cosmos fork of keyring
  • Replacing deprecated jwt-go
  • Fixing GHSA-h395-qcrw-5vmq vulnerability in gin-gonic

8-31: LGTM! Verify dependency compatibility

The dependency updates align with the PR objective of upgrading to cosmos-sdk v0.50.10. All cosmossdk.io package versions are compatible with this version.

Let's verify there are no breaking changes in the upgrade path:

modules/nft/simulation/operations.go (2)

46-82: LGTM: Parameter cleanup in weight generation

The removal of the unused cdc parameter from appParams.GetOrGenerate calls is aligned with the cosmos-sdk v0.50.10 update while preserving the original weight values and simulation behavior.


195-195: LGTM: Updated simulation return statements

The removal of the nil argument from simtypes.NewOperationMsg calls across all simulation functions correctly aligns with the updated function signature in cosmos-sdk v0.50.10.

Also applies to: 266-266, 332-332, 395-395, 483-483, 560-560

modules/nft/migrations/v2/store.go (4)

6-10: Updated imports align with the new SDK structure

The imports have been updated to use cosmossdk.io modules, which is correct and aligns with the updated SDK packages.


18-19: Function signature updated to use storeService and codec.BinaryCodec

The Migrate function now correctly accepts storeService store.KVStoreService and cdc codec.BinaryCodec, reflecting the updates in the SDK's API.


26-28: Correctly refactored store initialization and iterator

The store is now opened using storeService.OpenKVStore(ctx), wrapped with runtime.KVStoreAdapter, and the iterator is correctly created using storetypes.KVStorePrefixIterator. This aligns with the new SDK patterns.


32-33: keeper initialization updated to use storeService

The keeper struct is initialized with storeService, which is appropriate given the shift to a service-oriented architecture.

modules/nft/migrations/v2/keeper.go (3)

7-9: Update import paths to cosmossdk.io modules

The import paths have been updated to use the new cosmossdk.io modules, which aligns with the migration to cosmos-sdk v0.50.10.


21-22: Update keeper struct to use storeService and BinaryCodec

The keeper struct now uses storeService of type store.KVStoreService instead of storeKey, and cdc is updated to use codec.BinaryCodec. These changes are consistent with the updates in cosmos-sdk v0.50.10 and reflect the new store and codec architectures.


13-13: 🛠️ Refactor suggestion

Consider updating import path for runtime module

While other imports have been updated to cosmossdk.io, the import for github.com/cosmos/cosmos-sdk/runtime remains unchanged. Verify if this should be updated to cosmossdk.io/runtime or another appropriate path as per the new module organization in cosmos-sdk v0.50.10.

If the runtime module has been moved in the latest SDK version, consider updating the import path accordingly to maintain consistency.

Comment on lines +13 to +14
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI
AddressCodec() address.Codec
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can now provide a final response:

Interface changes need to be updated across multiple modules

The AccountKeeper interface in modules/nft/types/expected_keepers.go has been updated with context.Context and AddressCodec, but other modules are still using the old interface with sdk.Context. Updates are needed in:

  • modules/token/types/expected_keepers.go
  • modules/service/types/expected_keeper.go
  • modules/record/types/expected_keepers.go
  • modules/oracle/types/expected_keepers.go
  • modules/random/types/expected_keepers.go
  • modules/mt/types/expected_keepers.go
  • modules/htlc/types/expected_keepers.go
  • modules/farm/types/expected_keeper.go
  • modules/coinswap/types/expected_keepers.go

These modules need to:

  1. Update GetAccount signature to use context.Context instead of sdk.Context
  2. Add the new AddressCodec() method to their AccountKeeper interface
🔗 Analysis chain

Verify the impact of AccountKeeper interface changes

The changes to GetAccount and addition of AddressCodec align with cosmos-sdk v0.50.10, but we should verify that all implementations are updated accordingly.


Let me generate additional scripts to search for the AccountKeeper interface implementations.


Let me try one more search to specifically look for the AccountKeeper interface implementation in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for implementations of AccountKeeper interface to ensure they're updated
# Look for GetAccount implementations
ast-grep --pattern 'func $_($_ context.Context, $_ sdk.AccAddress) sdk.AccountI'

# Look for AddressCodec implementations
ast-grep --pattern 'func $_() address.Codec'

Length of output: 12685


Script:

#!/bin/bash
# Search for files that might contain AccountKeeper implementations
rg -l "AccountKeeper" --type go

# Search for GetAccount implementations with context
ast-grep --pattern 'func ($_ $_) GetAccount($_ context.Context, $_ sdk.AccAddress) sdk.AccountI'

# Look for AddressCodec implementations in keeper files
ast-grep --pattern 'func ($_ $_) AddressCodec() address.Codec'

Length of output: 1863


Script:

#!/bin/bash
# Look for AccountKeeper interface implementation in simapp
rg -A 10 "type AccountKeeper" --type go

# Check for any mock implementations
rg -A 10 "Mock.*AccountKeeper" --type go

# Look for the actual implementation in test files
rg -A 10 "TestAccountKeeper" --type go

Length of output: 7548

Comment on lines 3 to 5
go 1.21

toolchain go1.23.1
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect toolchain version specification

There's a mismatch between the Go version and toolchain:

  • Go version is set to 1.21
  • Toolchain is set to go1.23.1 (which doesn't exist yet, latest Go version is 1.22)

This will cause build failures. The toolchain version should not be higher than the Go version being used.

Apply this diff to fix the issue:

go 1.21

-toolchain go1.23.1
+toolchain go1.21.0
📝 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.

Suggested change
go 1.21
toolchain go1.23.1
go 1.21
toolchain go1.21.0

@@ -392,7 +392,7 @@ func SimulateMsgBurnNFT(
return simtypes.NoOpMsg(types.ModuleName, types.EventTypeEditNFT, err.Error()), nil, err
}

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect event type in error message

The error message uses EventTypeEditNFT instead of EventTypeBurnNFT which could be confusing when debugging simulation failures.

-			return simtypes.NoOpMsg(types.ModuleName, types.EventTypeEditNFT, err.Error()), nil, err
+			return simtypes.NoOpMsg(types.ModuleName, types.EventTypeBurnNFT, err.Error()), nil, err

Committable suggestion skipped: line range outside the PR's diff.

@mitch1024 mitch1024 merged commit aa8cd8a into main Nov 13, 2024
7 checks passed
@mitch1024 mitch1024 deleted the feature/issues_441 branch November 13, 2024 09:10
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.

bump up mods.irisnet.org/modules/nft to cosmos-sdk v0.50.10
3 participants