feat(nft): nft module #296
Conversation
- Added MintNFT method in keeper to handle NFT minting. - Introduced error handling for collection existence and minter validation. - Created expected_keeper interface for account and bank operations. - Defined new error types for collection management. - Added protobuf definitions for Collection and Nft types. - Established key prefixes for collections and supply management.
📝 WalkthroughWalkthroughAdds a new NFT module (protobufs for messages/txs/queries), module store keys, errors/events, an indexed schema-backed Keeper (collections, supply, NFTs, editions) with collection/mint/transfer/print APIs, Msg + gRPC servers, app keeper wiring, drop module primitives and rule engine, and extensive unit + gRPC tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Application
participant MsgSrv as MsgServer
participant Keeper as NFT Keeper
participant Store as KV Store
rect #E8F6FF
note right of User: Create collection (tx flow)
User->>App: submit MsgCreateCollection
App->>MsgSrv: CreateCollection
MsgSrv->>Keeper: CreateCollection(ctx, creator, minter, symbol,...)
Keeper->>Keeper: validateCollectionDenom / validate metadata
Keeper->>Store: setCollection(denom, collection)
Keeper->>Store: setSupply(denom, 0)
Keeper-->>MsgSrv: denom
MsgSrv-->>App: response
App-->>User: tx success
end
sequenceDiagram
autonumber
actor Minter
participant App as Application
participant MsgSrv as MsgServer
participant Keeper as NFT Keeper
participant Store as KV Store
rect #FFF4E6
note right of Minter: Mint + optional send (tx flow)
Minter->>App: submit MsgMintNFT
App->>MsgSrv: MintNFT
MsgSrv->>Keeper: MintNFT(ctx, minter, owner, collection,...)
Keeper->>Store: getCollection(collection)
Keeper->>Keeper: verify minter == collection.Minter
Keeper->>Store: setNft(key, nft)
Keeper->>Store: increment Supply
alt print edition requested
Minter->>App: submit MsgPrintEdition
App->>MsgSrv: PrintEdition
MsgSrv->>Keeper: PrintEdition(...)
Keeper->>Store: setEdition / incrementEdition
end
MsgSrv-->>App: response
App-->>Minter: tx success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
x/nft/types/expected_keeper.go (1)
11-15: Trim AccountKeeper to the minimum you use.If
GetAccountisn’t used by the NFT keeper, drop it to reduce coupling and future breakage risk with SDK upgrades.type AccountKeeper interface { GetModuleAddress(moduleName string) sdk.AccAddress - GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI AddressCodec() address.Codec }x/nft/keeper/collection_test.go (1)
15-20: Avoid relying on zero-value Keeper if the method ever touches state.If
createCollectionDenomdoesn’t use keeper fields, make it a package-level function (or a small pure helper) to prevent future nil derefs when the impl evolves.- func (k Keeper) createCollectionDenom(creator sdk.AccAddress, symbol string) string + func createCollectionDenom(creator sdk.AccAddress, symbol string) stringx/nft/types/keys.go (1)
5-14: Add brief GoDoc for exported constants/prefixes.Public identifiers without comments hurt discoverability and fail many linters/docs. Add 1-line comments for ModuleName/StoreKey/RouterKey and the prefixes.
x/nft/keeper/collection.go (2)
21-34: Metadata Display should likely be the base denom.Display typically matches the base denom when no exponent hierarchy exists. Using the human symbol for Display may confuse UI formatters.
Apply this diff:
- Symbol: coll.Symbol, - Display: coll.Symbol, + Symbol: coll.Symbol, + Display: denom,
61-66: Whitespace nit.Remove trailing whitespace on Line 64.
proto/bitsong/nft/v1beta1/nft.proto (2)
6-16: Add message-level comments; fix “autority” typo.Buf flags missing docs; also typo in commented field.
Apply this diff:
-message Collection { +// Collection defines an NFT collection metadata and mint authority. +message Collection { string symbol = 1; string name = 2; string description = 3; string uri = 4; string creator = 5; string minter = 6; uint64 num_tokens = 7; // bool is_mutable - // update_autority (who can update name, description and uri if is_mutable = true) + // update_authority (who can update name, description and uri if is_mutable = true) }
18-27: Add message-level comments; fix “autority” typo.Same as above for Nft; consider “NFT” naming for consistency, but not required.
Apply this diff:
-message Nft { +// Nft defines per-token display metadata. +message Nft { string name = 1; string description = 2; string uri = 3; // string owner = 5; // seller_fee_bps // payment_address // bool is_mutable - // update_autority (who can update name, description and uri if is_mutable = true) + // update_authority (who can update name, description and uri if is_mutable = true) }x/nft/keeper/keeper_test.go (6)
82-83: Drop noisy fmt.Println in tests (use logs if needed).These prints clutter CI output. Either remove or switch to suite.T().Logf.
Apply this diff to remove:
- fmt.Println("collectionDenom:", collectionDenom)- fmt.Println("nft1denom:", nft1denom)- fmt.Println("nft2denom:", nft2denom)- fmt.Println("collectionDenom:", collectionDenom)- fmt.Println("nftDenom for '"+tc.name+"':", nftDenom)Also applies to: 99-100, 113-114, 155-156, 214-215
3-18: Prune unused imports after the changes.After switching to getters and removing prints, these imports become unused.
Apply this diff:
-import ( - "fmt" - "testing" - - "cosmossdk.io/math" - simapp "github.com/bitsongofficial/go-bitsong/app" - apptesting "github.com/bitsongofficial/go-bitsong/app/testing" - fantokentypes "github.com/bitsongofficial/go-bitsong/x/fantoken/types" - "github.com/bitsongofficial/go-bitsong/x/nft/keeper" - "github.com/bitsongofficial/go-bitsong/x/nft/types" - "github.com/cometbft/cometbft/crypto/tmhash" - sdk "github.com/cosmos/cosmos-sdk/types" - bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" - types2 "github.com/cosmos/cosmos-sdk/x/bank/types" - "github.com/stretchr/testify/suite" -) +import ( + "testing" + + "cosmossdk.io/math" + simapp "github.com/bitsongofficial/go-bitsong/app" + apptesting "github.com/bitsongofficial/go-bitsong/app/testing" + fantokentypes "github.com/bitsongofficial/go-bitsong/x/fantoken/types" + "github.com/bitsongofficial/go-bitsong/x/nft/keeper" + "github.com/bitsongofficial/go-bitsong/x/nft/types" + "github.com/cometbft/cometbft/crypto/tmhash" + sdk "github.com/cosmos/cosmos-sdk/types" + bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" + "github.com/stretchr/testify/suite" +)
39-44: Avoid local shadowing of app; use suite.App directly.Minor cleanup; reduces cognitive load.
Apply this diff:
- app := suite.App - suite.keeper = app.NftKeeper - suite.bk = app.BankKeeper - suite.App = app - suite.ctx = suite.Ctx + suite.keeper = suite.App.NftKeeper + suite.bk = suite.App.BankKeeper + suite.ctx = suite.Ctx
56-69: Strengthen CreateCollection test: assert initial supply.Capture the returned denom and assert supply starts at 0.
Apply this diff:
- _, err := suite.keeper.CreateCollection(suite.ctx, creator, testCollection) - suite.NoError(err) + collectionDenom, err := suite.keeper.CreateCollection(suite.ctx, creator, testCollection) + suite.NoError(err) + suite.Equal(math.NewInt(0), suite.keeper.GetSupply(suite.ctx, collectionDenom))
111-113: Second mint reuses identical NFT metadata; risk of ID collision depending on denom derivation.If denom derivation is deterministic from NFT fields, this could collide and make the second balance check flaky. Use distinct metadata (you already do this in the advanced test).
Apply this diff:
- nft2denom, err := suite.keeper.MintNFT(suite.ctx, collectionDenom, creator, owner, nft) + nft2 := types.Nft{ + Name: "My Second NFT", + Description: "This is my second NFT", + Uri: "ipfs://my-second-nft-metadata.json", + } + nft2denom, err := suite.keeper.MintNFT(suite.ctx, collectionDenom, creator, owner, nft2)
132-143: Nice parameterized subtests.Good coverage and readable expectations across scenarios.
Small note: subtests share state (supply grows across cases). That’s intentional here; just avoid t.Parallel() in this block.
Also applies to: 206-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
x/nft/types/nft.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
app/keepers/keepers.go(4 hunks)app/keepers/keys.go(2 hunks)proto/bitsong/nft/v1beta1/nft.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/collection_test.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/errors.go(1 hunks)x/nft/types/expected_keeper.go(1 hunks)x/nft/types/keys.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
x/nft/keeper/keeper.go (3)
x/nft/types/expected_keeper.go (2)
BankKeeper(17-27)AccountKeeper(11-15)x/nft/types/nft.pb.go (3)
Collection(25-32)Collection(36-36)Collection(37-39)x/nft/types/keys.go (3)
ModuleName(6-6)CollectionsPrefix(12-12)SupplyPrefix(13-13)
x/nft/keeper/collection_test.go (1)
x/nft/keeper/keeper.go (1)
Keeper(14-24)
app/keepers/keys.go (1)
x/nft/types/keys.go (1)
StoreKey(7-7)
app/keepers/keepers.go (3)
x/nft/types/keys.go (2)
ModuleName(6-6)StoreKey(7-7)x/nft/keeper/keeper.go (2)
Keeper(14-24)NewKeeper(26-45)x/nft/types/expected_keeper.go (2)
AccountKeeper(11-15)BankKeeper(17-27)
x/nft/types/errors.go (1)
x/nft/types/keys.go (1)
ModuleName(6-6)
x/nft/keeper/nft.go (4)
x/nft/keeper/keeper.go (1)
Keeper(14-24)x/nft/types/nft.pb.go (3)
Nft(109-113)Nft(117-117)Nft(118-120)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)x/nft/types/keys.go (1)
ModuleName(6-6)
x/nft/keeper/collection.go (3)
x/nft/keeper/keeper.go (1)
Keeper(14-24)x/nft/types/nft.pb.go (3)
Collection(25-32)Collection(36-36)Collection(37-39)x/nft/types/errors.go (2)
ErrCollectionAlreadyExists(6-6)ErrCollectionNotFound(7-7)
x/nft/keeper/keeper_test.go (6)
app/params/config.go (1)
DefaultBondDenom(16-16)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(14-24)app/app.go (1)
BitsongApp(173-194)x/nft/types/expected_keeper.go (1)
BankKeeper(17-27)x/nft/types/nft.pb.go (6)
Collection(25-32)Collection(36-36)Collection(37-39)Nft(109-113)Nft(117-117)Nft(118-120)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/nft.proto
6-16: Message "Collection" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
18-27: Message "Nft" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (10)
x/nft/types/expected_keeper.go (1)
17-27: BankKeeper interface matches Cosmos SDK v0.53.x; no changes required.The existing methods—GetSupply, HasSupply, GetDenomMetaData, HasDenomMetaData, and SetDenomMetaData—exactly align with the SDK v0.53.x signatures.
app/keepers/keys.go (1)
61-61: LGTM: NFT KV store key registered.This is correctly wired and will unblock keeper initialization.
app/keepers/keepers.go (3)
118-118: Module account perms for NFT look right.Minter/Burner is appropriate given coin ops during mint/burn flows.
155-155: Keeper field addition is fine.Public
NftKeeperonAppKeepersmatches existing keeper patterns.
413-419: Ensure NFT module account is initialized at genesis
The NFT keeper’s mint/send operations will panic if its module account isn’t created. Verify that x/nft is registered in the ModuleManager and that its InitGenesis sets up the module account.x/nft/types/errors.go (1)
5-8: LGTM: error registry.Clear, module-scoped errors with stable codes.
x/nft/keeper/keeper.go (1)
26-31: LGTM: module account check and logger scoping.Good defensive check and consistent module-scoped logger.
x/nft/keeper/nft.go (1)
30-44: Denom metadata fields look consistent.Base/Display/Symbol keyed to nftDenom, with name/description/URI from the NFT metadata, is reasonable for per-token denoms.
x/nft/keeper/collection.go (1)
94-105: LGTM: collection persistence helpers.Clear set/get helpers with typed errors.
x/nft/keeper/keeper_test.go (1)
45-49: Confirm minter module for funding test accounts.You mint from fantoken module. Ensure that module account has the Minter permission for the bond denom; otherwise MintCoins will error in some configurations. If available, prefer a test helper (e.g., FundAcc) or mint module.
Do you want me to switch this to the mint module or a helper if present?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/nft/keeper/collection.go (1)
82-89: Use a single, consistent existence check and module error.Rely on bank metadata (or your Collections map) to detect pre-existing collections and always return types.ErrCollectionAlreadyExists. Avoid fmt.Errorf and redundant checks.
Apply:
- if k.bk.HasSupply(ctx, denom) { - return "", fmt.Errorf("denom %s already exists", denom) - } - - _, exists := k.bk.GetDenomMetaData(ctx, denom) - if exists { - return "", types.ErrCollectionAlreadyExists - } + if k.bk.HasDenomMetaData(ctx, denom) { + return "", types.ErrCollectionAlreadyExists + }
🧹 Nitpick comments (5)
x/nft/keeper/collection.go (2)
34-38: Persist creator into stored collection.The creator address used to derive denom isn’t saved in state; set coll.Creator to preserve provenance.
k.bk.SetDenomMetaData(ctx, metadata) - if err := k.setCollection(ctx, denom, coll); err != nil { + // persist creator provenance + coll.Creator = creator.String() + if err := k.setCollection(ctx, denom, coll); err != nil { return "", err }
21-32: Optional: emit a creation event for indexing/UX.Emitting an sdk.Event (with denom, symbol, creator) eases downstream indexing and CLI UX.
x/nft/keeper/keeper_test.go (3)
68-70: Assert the typed error on duplicate creation.Strengthen the assertion to the module error for stability across message changes.
- suite.Error(err) + suite.ErrorIs(err, types.ErrCollectionAlreadyExists)
66-66: Remove noisy prints in tests; use test logs if needed.Prefer suite.T().Logf to keep CI output clean.
- fmt.Println("denom:", denom) + suite.T().Logf("denom: %s", denom) - fmt.Println("collectionDenom:", collectionDenom) + suite.T().Logf("collectionDenom: %s", collectionDenom) - fmt.Println("nft1denom:", nft1denom) + suite.T().Logf("nft1denom: %s", nft1denom) - fmt.Println("nft2denom:", nft2denom) + suite.T().Logf("nft2denom: %s", nft2denom) - fmt.Println("collectionDenom:", collectionDenom) + suite.T().Logf("collectionDenom: %s", collectionDenom) - fmt.Println("nftDenom for '"+tc.name+"':", nftDenom) + suite.T().Logf("nftDenom for %q: %s", tc.name, nftDenom)Also applies to: 83-83, 101-101, 114-114, 156-156, 215-215
64-67: Also assert bank metadata is set after creation.Quickly validates CreateCollection side effects.
meta, ok := suite.bk.GetDenomMetaData(suite.ctx, denom) suite.True(ok) suite.Equal(denom, meta.Base) suite.Equal("MYNFT", meta.Display)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
x/nft/keeper/collection.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
x/nft/keeper/keeper_test.go (5)
app/params/config.go (1)
DefaultBondDenom(16-16)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(14-24)x/nft/types/expected_keeper.go (1)
BankKeeper(17-27)x/nft/types/nft.pb.go (6)
Collection(25-32)Collection(36-36)Collection(37-39)Nft(109-113)Nft(117-117)Nft(118-120)
x/nft/keeper/collection.go (3)
x/nft/keeper/keeper.go (1)
Keeper(14-24)x/nft/types/nft.pb.go (3)
Collection(25-32)Collection(36-36)Collection(37-39)x/nft/types/errors.go (2)
ErrCollectionAlreadyExists(6-6)ErrCollectionNotFound(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (1)
x/nft/keeper/collection.go (1)
71-73: Denom hash formatting fixed — good.Lowercase hex with an explicit delimiter eliminates sdk.ValidateDenom violations and collision ambiguity.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/interchaintest-e2e.yml (1)
76-84: Order and version: checkout before setup-go; use a single setup-go major.Cache restore may miss because go.sum isn’t present before checkout; also you mix v4 and v5 across jobs.
- - name: Set up Go ${{ env.GO_VERSION }} - uses: actions/setup-go@v4 - with: - go-version: ${{ env.GO_VERSION }} - cache-dependency-path: interchaintest/go.sum - - - name: checkout chain - uses: actions/checkout@v4 + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go ${{ env.GO_VERSION }} + uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + cache-dependency-path: interchaintest/go.sumOptionally also bump build-docker’s setup-go to v5 for consistency.
🧹 Nitpick comments (2)
.github/workflows/interchaintest-e2e.yml (2)
20-23: Tighten permissions (principle of least privilege).No package registry pushes occur; drop packages: write to avoid over-scoping.
permissions: contents: read - packages: write + packages: readOr omit the packages key entirely.
49-55: Optional: enable Buildx cache to speed iterative PRs.Layer caching will cut CI time for NFT module churn.
- name: Build and export uses: docker/build-push-action@v5 with: context: . tags: bitsong:local + cache-from: type=gha + cache-to: type=gha,mode=max outputs: type=docker,dest=${{ env.TAR_PATH }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/interchaintest-e2e.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/interchaintest-e2e.yml
8-8: both "branches" and "branches-ignore" filters cannot be used for the same event "pull_request". note: use '!' to negate patterns
(events)
17-17: both "branches" and "branches-ignore" filters cannot be used for the same event "push". note: use '!' to negate patterns
(events)
🔇 Additional comments (1)
.github/workflows/interchaintest-e2e.yml (1)
25-25: Fix probe script reference & semver pin
- Update your probe command to target the real workflow file (
interchaintest-e2e.yml), notyour-workflow.yml.- Prefer
GO_VERSION: 1.23to automatically adopt the latest patch; if you must lock to 1.23.0, verify it exists in the Ubuntu runner’s toolcache (e.g.ls /opt/hostedtoolcache/go | grep 1.23.0).
| branches: | ||
| - main | ||
| - master | ||
| branches-ignore: | ||
| - "mvp/**" | ||
|
|
There was a problem hiding this comment.
Fix invalid pull_request filters (branches + branches-ignore are mutually exclusive).
actionlint flags this; GitHub Actions ignores one set unpredictably. Also note: pull_request filters apply to base branch, not head. To exclude PRs from mvp/** heads, drop branches-ignore here and gate jobs with an if condition.
Apply this diff to the trigger:
pull_request:
- branches:
- - main
- - master
- branches-ignore:
- - "mvp/**"
+ branches:
+ - main
+ - masterThen add a job-level guard to skip PRs whose head starts with mvp/ (outside this hunk):
jobs:
build-docker:
if: ${{ github.event_name != 'pull_request' || !startsWith(github.head_ref, 'mvp/') }}
...
e2e-tests:
if: ${{ github.event_name != 'pull_request' || !startsWith(github.head_ref, 'mvp/') }}
...🧰 Tools
🪛 actionlint (1.7.7)
8-8: both "branches" and "branches-ignore" filters cannot be used for the same event "pull_request". note: use '!' to negate patterns
(events)
🤖 Prompt for AI Agents
.github/workflows/interchaintest-e2e.yml lines 5-10: the pull_request trigger
currently uses both branches and branches-ignore which are mutually exclusive
and branches-ignore is inappropriate for filtering PR heads; remove the
branches-ignore entry and keep only the branches list under the pull_request
trigger, then add job-level guards on each job (e.g., build-docker and
e2e-tests) to skip PRs whose head ref starts with "mvp/" by using an if
condition that allows the job when the event is not pull_request or when
startsWith(github.head_ref, 'mvp/') is false (i.e., if: ${{ github.event_name !=
'pull_request' || !startsWith(github.head_ref, 'mvp/') }}).
| - main | ||
| - master | ||
| branches-ignore: | ||
| - "mvp/**" |
There was a problem hiding this comment.
Fix invalid push filters (branches + branches-ignore conflict).
Use negative patterns in branches instead of branches-ignore.
push:
tags:
- "**"
branches:
- - main
- - master
- branches-ignore:
- - "mvp/**"
+ - main
+ - master
+ - '!mvp/**'📝 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.
| - main | |
| - master | |
| branches-ignore: | |
| - "mvp/**" | |
| push: | |
| tags: | |
| - "**" | |
| branches: | |
| - main | |
| - master | |
| - '!mvp/**' |
🧰 Tools
🪛 actionlint (1.7.7)
17-17: both "branches" and "branches-ignore" filters cannot be used for the same event "push". note: use '!' to negate patterns
(events)
🤖 Prompt for AI Agents
.github/workflows/interchaintest-e2e.yml lines 15-18: the workflow currently
mixes branches and branches-ignore which conflicts for push filters; remove the
branches-ignore entry and replace it by using negative patterns in the branches
list (e.g., keep the allowed branches like main and master and add the negative
pattern for the mvp/** paths) so the push trigger uses only a valid branches
array with exclusion patterns.
- Implemented RESTful JSON APIs for NFT queries using gRPC gateway. - Added handlers for the following queries: - Collection - OwnerOf - NumTokens - NftInfo - Nfts - AllNftsByOwner - Generated code from protobuf definitions to facilitate communication between gRPC and HTTP. - Remove sdk.coin logic
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
x/nft/types/keys.go (1)
11-17: Document each prefix (future-proofing) and clarify intent.
Small readability win; helps avoid accidental reuse/collision when adding indexes (e.g., planned authority/minter).Apply this diff to add inline docs:
var ( - CollectionsPrefix = collections.NewPrefix(0) - SupplyPrefix = collections.NewPrefix(1) - NFTsPrefix = collections.NewPrefix(2) - NFTsByCollectionPrefix = collections.NewPrefix(3) - OwnersPrefix = collections.NewPrefix(4) + // CollectionsPrefix: collection metadata keyed by denom. + CollectionsPrefix = collections.NewPrefix(0) + // SupplyPrefix: per-collection total minted supply. + SupplyPrefix = collections.NewPrefix(1) + // NFTsPrefix: NFT records keyed by (collectionDenom, tokenId). + NFTsPrefix = collections.NewPrefix(2) + // NFTsByCollectionPrefix: index to list NFTs in a collection. + NFTsByCollectionPrefix = collections.NewPrefix(3) + // OwnersPrefix: index to list NFTs by owner. + OwnersPrefix = collections.NewPrefix(4) )third_party/proto/cosmos/query/v1/query.proto (2)
33-34: Tighten/proofread the trailing sentence in the comment.
Minor clarity nit.- // When set to true, the query can safely be called + // When set to true, the query can safely be called from within the state machine.
11-35: Document extension ID policy and rationale. Verified uniqueness ofmodule_query_safe(ID 11110001) across the repo; please add a comment or docs clarifying that customMethodOptionsIDs should be ≥ 50000 and why this specific number was chosen.x/nft/keeper/grpc_query_test.go (3)
151-158: Rename test: it asserts by collection, not by owner.
Avoid confusion with similarly named owner query.-func (suite *KeeperTestSuite) TestQueryNftsOfOwner() { +func (suite *KeeperTestSuite) TestQueryNftsByCollection() {
189-191: Don’t assume ordering from store iteration.
Iteration order may change; assert membership instead.- suite.Len(res.Nfts, 2) - suite.Equal(nft1.TokenId, res.Nfts[0].TokenId) - suite.Equal(nft2.TokenId, res.Nfts[1].TokenId) + suite.Len(res.Nfts, 2) + got := []string{res.Nfts[0].TokenId, res.Nfts[1].TokenId} + suite.ElementsMatch([]string{nft1.TokenId, nft2.TokenId}, got)
15-30: Deduplicate expectedDenom literals across tests.
Small cleanup: factor the expected denom into a const/helper to avoid drift if algo changes.x/nft/keeper/nft.go (3)
21-25: Surface a TODO for max_supply enforcement at the right place.
Matches PR objective; won’t compile until the field exists, so leave as a TODO now.coll, err := k.Collections.Get(ctx, collectionDenom) if err != nil { return types.ErrCollectionNotFound } + // TODO: enforce max_supply once added to Collection: if max_supply > 0 && supply >= max_supply { return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "max supply reached") }
47-50: Remove unused createNftDenom helper (leftover from sdk.Coin path).
Reduces confusion with “denom” terminology now that minting doesn’t mint bank coins.-func (k Keeper) createNftDenom(ctx sdk.Context, collectionDenom string) string { - supply := k.GetSupply(ctx, collectionDenom) - return fmt.Sprintf("%s-%d", collectionDenom, supply.Uint64()+1) -}
40-45: Optional: make the write + supply increment error messages typed.
Consistency with SDK errors improves UX for clients.if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil { - return fmt.Errorf("failed to set NFT: %w", err) + return sdkerrors.Wrap(err, "failed to set NFT") } - return k.incrementSupply(ctx, collectionDenom) + return k.incrementSupply(ctx, collectionDenom)x/nft/keeper/keeper_test.go (3)
43-48: Bank coin minting in SetupTest seems unnecessary for the current NFT flow.
The module no longer mints sdk.Coins for NFTs; consider removing to decouple tests from bank.If nothing in NFT tests consumes these funds, drop this block for faster/leaner setup.
95-101: Differentiate metadata for the second NFT for clarity.
Pure readability; avoids confusion when debugging.nft2 := types.Nft{ TokenId: "2", - Name: "My First NFT", - Description: "This is my first NFT", - Uri: "ipfs://my-first-nft-metadata.json", + Name: "My Second NFT", + Description: "This is my second NFT", + Uri: "ipfs://my-second-nft-metadata.json", }
115-223: Remove the large commented-out advanced test block; replace with focused negative cases.
Keeps the suite concise; add explicit tests for unauthorized minter and duplicate token_id instead.Apply this diff to drop the commented block:
-/* -... (entire block) -*/Then add these two targeted tests (outside the commented block, after TestMintNFT):
func (suite *KeeperTestSuite) TestMintNFT_UnauthorizedMinter() { testCollection := types.Collection{ Name: "My NFT Collection", Symbol: "MYNFT", Uri: "ipfs://meta.json", Minter: creator.String(), } denom, err := suite.keeper.CreateCollection(suite.ctx, creator, testCollection) suite.NoError(err) nft := types.Nft{TokenId: "1", Name: "N1", Uri: "ipfs://n1.json"} err = suite.keeper.MintNFT(suite.ctx, denom, /*minter=*/ owner, /*owner=*/ owner, nft) suite.Error(err) } func (suite *KeeperTestSuite) TestMintNFT_DuplicateTokenID() { testCollection := types.Collection{ Name: "My NFT Collection", Symbol: "MYNFT", Uri: "ipfs://meta.json", Minter: creator.String(), } denom, err := suite.keeper.CreateCollection(suite.ctx, creator, testCollection) suite.NoError(err) n := types.Nft{TokenId: "1", Name: "N1"} suite.NoError(suite.keeper.MintNFT(suite.ctx, denom, creator, owner, n)) err = suite.keeper.MintNFT(suite.ctx, denom, creator, owner, n) suite.Error(err) }x/nft/keeper/grpc_query.go (2)
110-113: Separate nil-request from empty-field validation for clearer errors.Apply:
- if req == nil || req.Collection == "" { - return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") - } + if req == nil { + return nil, status.Error(codes.InvalidArgument, "empty request") + } + if req.Collection == "" { + return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") + }
75-81: Return NotFound when the collection doesn’t exist (aligns with Collection()).Apply:
sdkCtx := sdk.UnwrapSDKContext(ctx) - supply := k.GetSupply(sdkCtx, req.Collection) + exists, err := k.Collections.Has(sdkCtx, req.Collection) + if err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } + if !exists { + return nil, status.Errorf(codes.NotFound, "collection %s not found", req.Collection) + } + supply := k.GetSupply(sdkCtx, req.Collection)x/nft/keeper/keeper.go (1)
80-81: Remove stale TODO.Queries are present; the comment is outdated.
Apply:
- // TODO: fix the store once we add queriesproto/bitsong/nft/v1beta1/nft.proto (3)
19-21: Fix typos in comments ("update_autority" → "update_authority") and punctuation.Apply:
- // bool is_mutable - // update_autority (who can update name, description and uri if is_mutable = true) + // bool is_mutable; + // update_authority (who can update name, description and uri if is_mutable = true); @@ - // update_autority (who can update name, description and uri if is_mutable = true) + // update_authority (who can update name, description and uri if is_mutable = true);Also applies to: 37-37
8-21: Model fields for planned semantics: max_supply and authority.The PR goals mention max_supply and authority; define them now to avoid wire-format churn later.
Apply:
message Collection { option (gogoproto.goproto_getters) = false; string symbol = 1; string name = 2; string description = 3; string uri = 4; string creator = 5; string minter = 6; uint64 num_tokens = 7; + uint64 max_supply = 8; // 0 = only master exists + string authority = 9; // who can edit metadata
33-33: Consider storing owner as bytes instead of string.Using bytes aligns with address.Codec and avoids HRP coupling in state; Bech32 can be derived at the edges.
If you choose to change it:
- string owner = 6; + bytes owner = 6 [(gogoproto.customname) = "Owner"];This would require adapting keeper/indexing code accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
x/nft/types/nft.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (13)
app/keepers/keepers.go(3 hunks)proto/bitsong/nft/v1beta1/nft.proto(1 hunks)proto/bitsong/nft/v1beta1/query.proto(1 hunks)third_party/proto/cosmos/query/v1/query.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/collection_test.go(1 hunks)x/nft/keeper/grpc_query.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/expected_keeper.go(1 hunks)x/nft/types/keys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/nft/types/expected_keeper.go
- x/nft/keeper/collection.go
- app/keepers/keepers.go
- x/nft/keeper/collection_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
x/nft/keeper/grpc_query.go (3)
x/nft/types/query.pb.go (37)
QueryServer(632-639)QueryCollectionRequest(33-35)QueryCollectionRequest(39-39)QueryCollectionRequest(40-42)QueryCollectionResponse(70-72)QueryCollectionResponse(76-76)QueryCollectionResponse(77-79)QueryOwnerOfRequest(107-110)QueryOwnerOfRequest(114-114)QueryOwnerOfRequest(115-117)QueryOwnerOfResponse(145-147)QueryOwnerOfResponse(151-151)QueryOwnerOfResponse(152-154)QueryNumTokensRequest(182-184)QueryNumTokensRequest(188-188)QueryNumTokensRequest(189-191)QueryNumTokensResponse(219-221)QueryNumTokensResponse(225-225)QueryNumTokensResponse(226-228)QueryNftInfoRequest(256-259)QueryNftInfoRequest(263-263)QueryNftInfoRequest(264-266)QueryNftInfoResponse(294-296)QueryNftInfoResponse(300-300)QueryNftInfoResponse(301-303)QueryNftsRequest(331-334)QueryNftsRequest(338-338)QueryNftsRequest(339-341)QueryNftsResponse(369-372)QueryNftsResponse(376-376)QueryNftsResponse(377-379)QueryAllNftsByOwnerRequest(407-409)QueryAllNftsByOwnerRequest(413-413)QueryAllNftsByOwnerRequest(414-416)QueryAllNftsByOwnerResponse(444-446)QueryAllNftsByOwnerResponse(450-450)QueryAllNftsByOwnerResponse(451-453)x/nft/keeper/keeper.go (1)
Keeper(52-63)x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)
x/nft/keeper/grpc_query_test.go (3)
x/nft/keeper/keeper_test.go (1)
KeeperTestSuite(25-32)x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/query.pb.go (18)
QueryCollectionRequest(33-35)QueryCollectionRequest(39-39)QueryCollectionRequest(40-42)QueryOwnerOfRequest(107-110)QueryOwnerOfRequest(114-114)QueryOwnerOfRequest(115-117)QueryNumTokensRequest(182-184)QueryNumTokensRequest(188-188)QueryNumTokensRequest(189-191)QueryNftInfoRequest(256-259)QueryNftInfoRequest(263-263)QueryNftInfoRequest(264-266)QueryNftsRequest(331-334)QueryNftsRequest(338-338)QueryNftsRequest(339-341)QueryAllNftsByOwnerRequest(407-409)QueryAllNftsByOwnerRequest(413-413)QueryAllNftsByOwnerRequest(414-416)
x/nft/keeper/nft.go (3)
x/nft/keeper/keeper.go (1)
Keeper(52-63)x/nft/types/nft.pb.go (6)
Nft(69-76)Nft(80-80)Nft(81-83)Collection(26-34)Collection(38-38)Collection(39-41)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)
x/nft/keeper/keeper_test.go (6)
app/params/config.go (1)
DefaultBondDenom(16-16)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(52-63)x/fantoken/types/expected_keepers.go (1)
BankKeeper(27-37)x/nft/types/keys.go (1)
ModuleName(6-6)x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)
x/nft/keeper/keeper.go (3)
x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/keys.go (6)
NFTsByCollectionPrefix(15-15)OwnersPrefix(16-16)ModuleName(6-6)CollectionsPrefix(12-12)SupplyPrefix(13-13)NFTsPrefix(14-14)x/nft/types/expected_keeper.go (1)
AccountKeeper(10-14)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/query.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (3)
x/nft/types/keys.go (1)
5-9: Module/store/router keys look correct and consistent.
No functional concerns; aligns with app wiring per summary.x/nft/keeper/grpc_query.go (1)
16-16: Compile-time interface assertion looks good.Ensures Keeper satisfies types.QueryServer.
proto/bitsong/nft/v1beta1/query.proto (1)
12-42: Service surface looks coherent and consistent with keeper queries.
| syntax = "proto3"; | ||
| package bitsong.nft.v1beta1; | ||
|
|
||
| import "gogoproto/gogo.proto"; |
There was a problem hiding this comment.
buf: missing dependency for gogoproto; build will fail.
Add gogoproto to buf deps or vendor third_party/proto/gogoproto/gogo.proto.
Add to buf.yaml:
version: v2
deps:
- buf.build/cosmos/gogo-protoIf you also rely on google/cosmos imports elsewhere, include:
- buf.build/googleapis/googleapis
- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/cosmos-sdk
🧰 Tools
🪛 Buf (1.55.1)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/nft.proto around line 4, the file imports
gogoproto/gogo.proto but buf.yaml has no dependency for gogoproto which will
cause builds to fail; update buf.yaml to add the gogoproto dependency
(buf.build/cosmos/gogo-proto) or vendor third_party/proto/gogoproto/gogo.proto
into the repo, and if your project also uses google/cosmos imports add the
recommended buf deps (buf.build/googleapis/googleapis,
buf.build/cosmos/cosmos-proto, buf.build/cosmos/cosmos-sdk) so the proto import
resolves during buf build.
| syntax = "proto3"; | ||
| package bitsong.nft.v1beta1; | ||
|
|
||
| import "gogoproto/gogo.proto"; |
There was a problem hiding this comment.
buf: missing dependency for gogoproto; build will fail.
Same as nft.proto: add gogoproto (and likely googleapis/cosmos) to buf deps or vendor the files.
Add to buf.yaml:
version: v2
deps:
- buf.build/cosmos/gogo-proto
- buf.build/googleapis/googleapis
- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/cosmos-sdk🧰 Tools
🪛 Buf (1.55.1)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/query.proto around line 4, the file imports
gogoproto/gogo.proto but buf build is missing that dependency so the build will
fail; update the repository's buf.yaml to include the required buf deps
(examples: buf.build/cosmos/gogo-proto, buf.build/googleapis/googleapis and any
cosmos proto/sdk repos referenced) or vendor the gogoproto and googleapis files
into the proto tree, then re-run buf mod update and buf generate to ensure
imports resolve; also verify import paths in the .proto files match the added
deps.
x/nft/keeper/nft.go
Outdated
| func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error { | ||
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | ||
| has, err := k.NFTs.Has(ctx, nftKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check NFT: %w", err) | ||
| } | ||
| if has { | ||
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate tokenId early.
Prevent empty/whitespace IDs; return a typed error.
func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error {
+ if strings.TrimSpace(metadata.TokenId) == "" {
+ return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is required")
+ }
nftKey := collections.Join(collectionDenom, metadata.TokenId)And add imports:
import (
"fmt"
"cosmossdk.io/collections"
"github.com/bitsongofficial/go-bitsong/x/nft/types"
sdk "github.com/cosmos/cosmos-sdk/types"
+ sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+ "strings"
)📝 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.
| func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error { | |
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | |
| has, err := k.NFTs.Has(ctx, nftKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to check NFT: %w", err) | |
| } | |
| if has { | |
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) | |
| } | |
| import ( | |
| "fmt" | |
| "cosmossdk.io/collections" | |
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |
| "strings" | |
| ) | |
| func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error { | |
| if strings.TrimSpace(metadata.TokenId) == "" { | |
| return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is required") | |
| } | |
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | |
| has, err := k.NFTs.Has(ctx, nftKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to check NFT: %w", err) | |
| } | |
| if has { | |
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) | |
| } | |
| // ... rest of function unchanged | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 11-19, validate metadata.TokenId
immediately: trim whitespace (strings.TrimSpace) and if the resulting tokenId is
empty return a typed error (use a defined NFT token-id error from the module,
e.g. types.ErrInvalidTokenId or wrap an sdkerrors.ErrInvalidRequest with a clear
message) instead of proceeding; add the required imports ("strings" and the
sdkerrors package "github.com/cosmos/cosmos-sdk/types/errors" or the module's
errors package) so the validation and typed error can be returned.
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid minter address: %w", err) | ||
| } | ||
|
|
||
| if !minter.Equals(collectionMinter) { | ||
| return fmt.Errorf("only the collection minter can mint NFTs") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle “no minter set” explicitly and use typed errors.
PR says minter may be null to disable minting. Current code treats empty minter as invalid bech32.
- collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter)
- if err != nil {
- return fmt.Errorf("invalid minter address: %w", err)
- }
-
- if !minter.Equals(collectionMinter) {
- return fmt.Errorf("only the collection minter can mint NFTs")
- }
+ if coll.Minter == "" {
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection")
+ }
+ collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter)
+ if err != nil {
+ return sdkerrors.Wrap(err, "invalid minter address")
+ }
+ if !minter.Equals(collectionMinter) {
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs")
+ }📝 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.
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | |
| if err != nil { | |
| return fmt.Errorf("invalid minter address: %w", err) | |
| } | |
| if !minter.Equals(collectionMinter) { | |
| return fmt.Errorf("only the collection minter can mint NFTs") | |
| } | |
| if coll.Minter == "" { | |
| return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") | |
| } | |
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | |
| if err != nil { | |
| return sdkerrors.Wrap(err, "invalid minter address") | |
| } | |
| if !minter.Equals(collectionMinter) { | |
| return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs") | |
| } |
- Added validation to ensure that the token ID is not empty when minting an NFT. - Implemented a check to disable minting for collections without a specified minter. - Introduced pagination support in the QueryAllNftsByOwnerRequest and QueryAllNftsByOwnerResponse messages. - Updated the keeper to handle the new pagination parameters and return paginated results. - Enhanced test cases to cover the new NFT minting and querying features, including scenarios for multiple owners and pagination.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
x/nft/keeper/collection.go (1)
58-63: Denom hashing now lowercase — good fix.
Matches sdk.ValidateDenom constraints and avoids collisions via delimiter.proto/bitsong/nft/v1beta1/query.proto (2)
122-140: Resolved: AllNftsByOwner now paginated.This addresses the earlier concern about unbounded responses.
4-7: Buf deps missing for gogoproto/googleapis/cosmos — proto build will fail.
gogoproto/gogo.proto(and google/cosmos imports) aren’t in buf deps. Add required deps to buf.yaml or vendor them.Apply:
version: v2 +deps: + - buf.build/cosmos/gogo-proto + - buf.build/googleapis/googleapis + - buf.build/cosmos/cosmos-proto + - buf.build/cosmos/cosmos-sdkRun to validate:
#!/bin/bash set -euo pipefail test -f buf.yaml && cat buf.yaml buf mod update buf build
🧹 Nitpick comments (10)
x/nft/keeper/grpc_query.go (2)
111-114: Return consistent error for nil request.When req is nil, this returns “collection cannot be empty”. Align with other handlers and return “empty request”; then separately validate collection.
- if req == nil || req.Collection == "" { - return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") - } + if req == nil { + return nil, status.Error(codes.InvalidArgument, "empty request") + } + if req.Collection == "" { + return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") + }
172-181: Prefer using the Owner index directly instead of raw KV iteration.You already have k.NFTs.Indexes.Owner. Using the index iterator avoids coupling to internal key encodings and simplifies pagination logic. If desired, I can wire a WithCollectionPaginationIndex option or do manual offset/limit over the iterator.
x/nft/keeper/nft.go (3)
56-59: Unused helper or unclear lifecycle.createNftDenom isn’t used in MintNFT or elsewhere. Remove it or add a call site/tests if it’s part of the public API.
12-54: Max supply and authority semantics not enforced.PR objectives mention max_supply and authority. MintNFT currently doesn’t enforce a cap nor check authority for metadata edits. Plan to extend Collection proto and add checks here.
Would you like a follow-up patch sketching max_supply/authority fields and enforcement points?
12-15: Validate collectionDenom too.Early-return if collectionDenom is empty or invalid to avoid malformed keys.
x/nft/keeper/keeper.go (1)
68-103: Constructor wiring LGTM; minor optional: prefer pointer receiver Keeper.Current pattern returns Keeper by value; consider returning *Keeper and using pointer receivers to avoid copying large structs on interface assertions and registrations.
x/nft/keeper/collection.go (1)
18-31: CreateCollection: consider initializing supply.After setCollection, explicitly set initial supply to zero to make HasSupply(ctx, denom) deterministic.
if err := k.setCollection(ctx, denom, coll); err != nil { return "", err } +// Initialize supply to zero for this denom. +if err := k.setSupply(ctx, denom, math.ZeroInt()); err != nil { + return "", err +} return denom, nilproto/bitsong/nft/v1beta1/query.proto (3)
38-41: Nit: Consider more RESTful path for owner listing.Suggest
GET /bitsong/nft/v1beta1/owners/{owner}/nftsfor consistency with hierarchical resources.- option (google.api.http).get = "/bitsong/nft/v1beta1/nfts_by_owner/{owner}"; + option (google.api.http).get = "/bitsong/nft/v1beta1/owners/{owner}/nfts";
51-56: Make embedded message non-nullable for better Go ergonomics.Avoids pointer fields in Go for
collection.- bitsong.nft.v1beta1.Collection collection = 1; + bitsong.nft.v1beta1.Collection collection = 1 [(gogoproto.nullable) = false];
95-100: Same here: makenftnon-nullable.Matches the pattern you already used for repeated fields; fewer nil checks.
- bitsong.nft.v1beta1.Nft nft = 1; + bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
x/nft/types/query.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (10)
app/keepers/keepers.go(3 hunks)proto/bitsong/nft/v1beta1/query.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/collection_test.go(1 hunks)x/nft/keeper/grpc_query.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/keys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- x/nft/types/keys.go
- app/keepers/keepers.go
- x/nft/keeper/grpc_query_test.go
- x/nft/keeper/keeper_test.go
- x/nft/keeper/collection_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
x/nft/keeper/grpc_query.go (5)
x/nft/types/query.pb.go (37)
QueryServer(634-641)QueryCollectionRequest(33-35)QueryCollectionRequest(39-39)QueryCollectionRequest(40-42)QueryCollectionResponse(70-72)QueryCollectionResponse(76-76)QueryCollectionResponse(77-79)QueryOwnerOfRequest(107-110)QueryOwnerOfRequest(114-114)QueryOwnerOfRequest(115-117)QueryOwnerOfResponse(145-147)QueryOwnerOfResponse(151-151)QueryOwnerOfResponse(152-154)QueryNumTokensRequest(182-184)QueryNumTokensRequest(188-188)QueryNumTokensRequest(189-191)QueryNumTokensResponse(219-221)QueryNumTokensResponse(225-225)QueryNumTokensResponse(226-228)QueryNftInfoRequest(256-259)QueryNftInfoRequest(263-263)QueryNftInfoRequest(264-266)QueryNftInfoResponse(294-296)QueryNftInfoResponse(300-300)QueryNftInfoResponse(301-303)QueryNftsRequest(331-334)QueryNftsRequest(338-338)QueryNftsRequest(339-341)QueryNftsResponse(369-372)QueryNftsResponse(376-376)QueryNftsResponse(377-379)QueryAllNftsByOwnerRequest(407-410)QueryAllNftsByOwnerRequest(414-414)QueryAllNftsByOwnerRequest(415-417)QueryAllNftsByOwnerResponse(445-448)QueryAllNftsByOwnerResponse(452-452)QueryAllNftsByOwnerResponse(453-455)x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/keys.go (1)
NFTsByOwnerPrefix(16-16)x/nft/keeper/collection.go (1)
MustSplitNftLengthPrefixedKey(141-148)
x/nft/keeper/keeper.go (3)
x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/keys.go (7)
NFTsByCollectionPrefix(15-15)NFTsByOwnerPrefix(16-16)StoreKey(7-7)ModuleName(6-6)CollectionsPrefix(12-12)SupplyPrefix(13-13)NFTsPrefix(14-14)x/nft/types/expected_keeper.go (1)
AccountKeeper(10-14)
x/nft/keeper/collection.go (3)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/nft.pb.go (3)
Collection(26-34)Collection(38-38)Collection(39-41)x/nft/types/errors.go (2)
ErrCollectionAlreadyExists(6-6)ErrCollectionNotFound(7-7)
x/nft/keeper/nft.go (3)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/nft.pb.go (6)
Nft(69-76)Nft(80-80)Nft(81-83)Collection(26-34)Collection(38-38)Collection(39-41)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/query.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (5)
x/nft/keeper/grpc_query.go (1)
17-17: Compile-time assertion looks good.
The QueryServer interface is implemented; receiver choice is consistent with the rest of the module.x/nft/keeper/keeper.go (1)
16-44: Indexes definition looks correct.Collection and Owner multi-indexes are aligned with key codecs; owner uses sdk.AccAddressKey with bech32 parsing.
proto/bitsong/nft/v1beta1/query.proto (3)
33-36: Nfts RPC + REST mapping look good.Pagination is supported via PageRequest; REST path is straightforward.
102-110: Good: collection paging is wired correctly.
QueryNftsRequestincludes PageRequest; aligns with keeper-side pagination.
111-120: Good: response includes PageResponse and uses non-nullable list items.
| service Query { | ||
| rpc Collection(QueryCollectionRequest) returns (QueryCollectionResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/collection/{collection}"; | ||
| } | ||
|
|
||
| rpc OwnerOf(QueryOwnerOfRequest) returns (QueryOwnerOfResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/owner/{collection}/{token_id}"; | ||
| } | ||
|
|
||
| rpc NumTokens(QueryNumTokensRequest) returns (QueryNumTokensResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/num_tokens/{collection}"; | ||
| } | ||
|
|
||
| rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/nft_info/{collection}/{token_id}"; | ||
| } | ||
|
|
||
| rpc Nfts(QueryNftsRequest) returns (QueryNftsResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts/{collection}"; | ||
| } | ||
|
|
||
| rpc AllNftsByOwner(QueryAllNftsByOwnerRequest) returns (QueryAllNftsByOwnerResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; | ||
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts_by_owner/{owner}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing planned RPC: AllNftInfo (nft + owner).
PR objectives include “allNftInfo”; it’s not defined here. Add an RPC and messages returning both Nft and Owner in one call.
Proto additions:
service Query {
@@
rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) {
option (cosmos.query.v1.module_query_safe) = true;
option (google.api.http).get = "/bitsong/nft/v1beta1/nft_info/{collection}/{token_id}";
}
+
+ rpc AllNftInfo(QueryAllNftInfoRequest) returns (QueryAllNftInfoResponse) {
+ option (cosmos.query.v1.module_query_safe) = true;
+ option (google.api.http).get = "/bitsong/nft/v1beta1/all_nft_info/{collection}/{token_id}";
+ }
@@
}
+
+message QueryAllNftInfoRequest {
+ option (gogoproto.equal) = false;
+ option (gogoproto.goproto_getters) = false;
+ string collection = 1;
+ string token_id = 2;
+}
+
+message QueryAllNftInfoResponse {
+ option (gogoproto.equal) = false;
+ option (gogoproto.goproto_getters) = false;
+ bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false];
+ string owner = 2;
+}📝 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.
| service Query { | |
| rpc Collection(QueryCollectionRequest) returns (QueryCollectionResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/collection/{collection}"; | |
| } | |
| rpc OwnerOf(QueryOwnerOfRequest) returns (QueryOwnerOfResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/owner/{collection}/{token_id}"; | |
| } | |
| rpc NumTokens(QueryNumTokensRequest) returns (QueryNumTokensResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/num_tokens/{collection}"; | |
| } | |
| rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nft_info/{collection}/{token_id}"; | |
| } | |
| rpc Nfts(QueryNftsRequest) returns (QueryNftsResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts/{collection}"; | |
| } | |
| rpc AllNftsByOwner(QueryAllNftsByOwnerRequest) returns (QueryAllNftsByOwnerResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts_by_owner/{owner}"; | |
| } | |
| } | |
| service Query { | |
| rpc Collection(QueryCollectionRequest) returns (QueryCollectionResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/collection/{collection}"; | |
| } | |
| rpc OwnerOf(QueryOwnerOfRequest) returns (QueryOwnerOfResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/owner/{collection}/{token_id}"; | |
| } | |
| rpc NumTokens(QueryNumTokensRequest) returns (QueryNumTokensResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/num_tokens/{collection}"; | |
| } | |
| rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nft_info/{collection}/{token_id}"; | |
| } | |
| rpc AllNftInfo(QueryAllNftInfoRequest) returns (QueryAllNftInfoResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/all_nft_info/{collection}/{token_id}"; | |
| } | |
| rpc Nfts(QueryNftsRequest) returns (QueryNftsResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts/{collection}"; | |
| } | |
| rpc AllNftsByOwner(QueryAllNftsByOwnerRequest) returns (QueryAllNftsByOwnerResponse) { | |
| option (cosmos.query.v1.module_query_safe) = true; | |
| option (google.api.http).get = "/bitsong/nft/v1beta1/nfts_by_owner/{owner}"; | |
| } | |
| } | |
| message QueryAllNftInfoRequest { | |
| option (gogoproto.equal) = false; | |
| option (gogoproto.goproto_getters) = false; | |
| string collection = 1; | |
| string token_id = 2; | |
| } | |
| message QueryAllNftInfoResponse { | |
| option (gogoproto.equal) = false; | |
| option (gogoproto.goproto_getters) = false; | |
| bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false]; | |
| string owner = 2; | |
| } |
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/query.proto around lines 12 to 42, the planned RPC
AllNftInfo is missing; add an RPC rpc AllNftInfo(QueryAllNftInfoRequest) returns
(QueryAllNftInfoResponse) with the same module_query_safe and google.api.http
GET pattern (e.g. "/bitsong/nft/v1beta1/all_nft_info/{collection}/{token_id}" or
appropriate path), and define the accompanying messages QueryAllNftInfoRequest
(collection, token_id) and QueryAllNftInfoResponse containing both the Nft info
message (e.g. Nft) and owner string (or reference to existing
QueryNftInfoResponse and Owner field) so a single call returns both NFT metadata
and owner together.
| func (k Keeper) AllNftsByOwner(ctx context.Context, req *types.QueryAllNftsByOwnerRequest) (*types.QueryAllNftsByOwnerResponse, error) { | ||
| if req == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "empty request") | ||
| } | ||
|
|
||
| owner, err := sdk.AccAddressFromBech32(req.Owner) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "invalid owner address: %s", err.Error()) | ||
| } | ||
|
|
||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
|
|
||
| store := prefix.NewStore( | ||
| sdkCtx.KVStore(k.storeKey), | ||
| append(types.NFTsByOwnerPrefix, address.MustLengthPrefix(owner)...), | ||
| ) | ||
|
|
||
| var nfts []types.Nft | ||
|
|
||
| pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error { | ||
| denom, tokenId := MustSplitNftLengthPrefixedKey(key) | ||
|
|
||
| nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nfts = append(nfts, nft) | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
|
|
||
| /*iter, err := k.NFTs.Indexes.Owner.MatchExact(ctx, owner) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| defer iter.Close() | ||
| nfts, err := indexes.CollectValues(ctx, k.NFTs, iter) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| }*/ | ||
| return &types.QueryAllNftsByOwnerResponse{ | ||
| Nfts: nfts, | ||
| Pagination: pageRes, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
AllNftsByOwner: wrong context type + incorrect key decoding (will return wrong NFTs or panic).
- Uses ctx (context.Context) instead of sdkCtx in k.NFTs.Get; types mismatch/compile error.
- Decodes the owner index key via MustSplitNftLengthPrefixedKey, which doesn’t match collections/indexes encoding. The owner index key is
[prefix][owner][0x00][primaryKey]. You must decode the primary key (Pair[string,string]) from key[1:] using PairKeyCodec, then Get via that Pair.
Apply this diff:
func (k Keeper) AllNftsByOwner(ctx context.Context, req *types.QueryAllNftsByOwnerRequest) (*types.QueryAllNftsByOwnerResponse, error) {
@@
store := prefix.NewStore(
sdkCtx.KVStore(k.storeKey),
append(types.NFTsByOwnerPrefix, address.MustLengthPrefix(owner)...),
)
var nfts []types.Nft
- pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error {
- denom, tokenId := MustSplitNftLengthPrefixedKey(key)
-
- nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId)))
- if err != nil {
- return err
- }
-
- nfts = append(nfts, nft)
- return nil
- })
+ pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, _ []byte) error {
+ // owner index key layout: <owner-encoded><0x00><primaryKey>
+ if len(key) == 0 {
+ return status.Error(codes.Internal, "corrupted owner index key")
+ }
+ // Decode (collection, tokenId) primary key from suffix after the delimiter
+ pk, err := collections.PairKeyCodec(collections.StringKey, collections.StringKey).Decode(key[1:])
+ if err != nil {
+ return err
+ }
+ nft, err := k.NFTs.Get(sdkCtx, pk)
+ if err != nil {
+ return err
+ }
+ nfts = append(nfts, nft)
+ return nil
+ })
@@
return &types.QueryAllNftsByOwnerResponse{
Nfts: nfts,
Pagination: pageRes,
}, nil
}📝 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.
| func (k Keeper) AllNftsByOwner(ctx context.Context, req *types.QueryAllNftsByOwnerRequest) (*types.QueryAllNftsByOwnerResponse, error) { | |
| if req == nil { | |
| return nil, status.Error(codes.InvalidArgument, "empty request") | |
| } | |
| owner, err := sdk.AccAddressFromBech32(req.Owner) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "invalid owner address: %s", err.Error()) | |
| } | |
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |
| store := prefix.NewStore( | |
| sdkCtx.KVStore(k.storeKey), | |
| append(types.NFTsByOwnerPrefix, address.MustLengthPrefix(owner)...), | |
| ) | |
| var nfts []types.Nft | |
| pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error { | |
| denom, tokenId := MustSplitNftLengthPrefixedKey(key) | |
| nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId))) | |
| if err != nil { | |
| return err | |
| } | |
| nfts = append(nfts, nft) | |
| return nil | |
| }) | |
| if err != nil { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| } | |
| /*iter, err := k.NFTs.Indexes.Owner.MatchExact(ctx, owner) | |
| if err != nil { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| } | |
| defer iter.Close() | |
| nfts, err := indexes.CollectValues(ctx, k.NFTs, iter) | |
| if err != nil { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| }*/ | |
| return &types.QueryAllNftsByOwnerResponse{ | |
| Nfts: nfts, | |
| Pagination: pageRes, | |
| }, nil | |
| } | |
| func (k Keeper) AllNftsByOwner(ctx context.Context, req *types.QueryAllNftsByOwnerRequest) (*types.QueryAllNftsByOwnerResponse, error) { | |
| if req == nil { | |
| return nil, status.Error(codes.InvalidArgument, "empty request") | |
| } | |
| owner, err := sdk.AccAddressFromBech32(req.Owner) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "invalid owner address: %s", err.Error()) | |
| } | |
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |
| store := prefix.NewStore( | |
| sdkCtx.KVStore(k.storeKey), | |
| append(types.NFTsByOwnerPrefix, address.MustLengthPrefix(owner)...), | |
| ) | |
| var nfts []types.Nft | |
| pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, _ []byte) error { | |
| // owner index key layout: <owner-encoded><0x00><primaryKey> | |
| if len(key) == 0 { | |
| return status.Error(codes.Internal, "corrupted owner index key") | |
| } | |
| // Decode (collection, tokenId) primary key from suffix after the delimiter | |
| pk, err := collections.PairKeyCodec(collections.StringKey, collections.StringKey).Decode(key[1:]) | |
| if err != nil { | |
| return err | |
| } | |
| nft, err := k.NFTs.Get(sdkCtx, pk) | |
| if err != nil { | |
| return err | |
| } | |
| nfts = append(nfts, nft) | |
| return nil | |
| }) | |
| if err != nil { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| } | |
| return &types.QueryAllNftsByOwnerResponse{ | |
| Nfts: nfts, | |
| Pagination: pageRes, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/grpc_query.go around lines 136-186, the handler uses the wrong
context and decodes owner-index keys incorrectly: replace k.NFTs.Get(ctx, ...)
with k.NFTs.Get(sdkCtx, ...) and instead of MustSplitNftLengthPrefixedKey decode
the owner index key layout ([prefix][owner][0x00][primaryKey]) by slicing off
the index prefix/owner portion and decoding the remaining primaryKey using
PairKeyCodec to obtain the Pair[string,string] (denom, tokenId), then call
k.NFTs.Get(sdkCtx, collections.Join(denom, tokenId)); propagate and return any
errors from decoding/Get as before and ensure pagination continues to work.
x/nft/keeper/nft.go
Outdated
| func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error { | ||
| if strings.TrimSpace(metadata.TokenId) == "" { | ||
| return fmt.Errorf("token ID cannot be empty") | ||
| } | ||
|
|
||
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | ||
| has, err := k.NFTs.Has(ctx, nftKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check NFT: %w", err) | ||
| } | ||
| if has { | ||
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) | ||
| } | ||
|
|
||
| coll, err := k.Collections.Get(ctx, collectionDenom) | ||
| if err != nil { | ||
| return types.ErrCollectionNotFound | ||
| } | ||
|
|
||
| if coll.Minter == "" { | ||
| return fmt.Errorf("minting disabled for this collection") | ||
| } | ||
|
|
||
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid minter address: %w", err) | ||
| } | ||
|
|
||
| if !minter.Equals(collectionMinter) { | ||
| return fmt.Errorf("only the collection minter can mint NFTs") | ||
| } | ||
|
|
||
| // TODO: Charge fee if necessary | ||
|
|
||
| metadata.Collection = collectionDenom | ||
| metadata.Owner = owner.String() | ||
|
|
||
| if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil { | ||
| return fmt.Errorf("failed to set NFT: %w", err) | ||
| } | ||
|
|
||
| return k.incrementSupply(ctx, collectionDenom) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed Cosmos SDK errors and consistent wrapping.
Replace fmt.Errorf with sdkerrors/errorsmod for proper ABCI codes and better client UX. Also align “minting disabled” and authorization checks to ErrUnauthorized.
import (
"fmt"
"strings"
"cosmossdk.io/collections"
+ errorsmod "cosmossdk.io/errors"
"github.com/bitsongofficial/go-bitsong/x/nft/types"
sdk "github.com/cosmos/cosmos-sdk/types"
+ sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
@@
if strings.TrimSpace(metadata.TokenId) == "" {
- return fmt.Errorf("token ID cannot be empty")
+ return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty")
}
@@
has, err := k.NFTs.Has(ctx, nftKey)
if err != nil {
- return fmt.Errorf("failed to check NFT: %w", err)
+ return errorsmod.Wrap(err, "failed to check NFT")
}
if has {
- return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "nft %s already exists in collection %s", metadata.TokenId, collectionDenom)
}
@@
if coll.Minter == "" {
- return fmt.Errorf("minting disabled for this collection")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection")
}
collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter)
if err != nil {
- return fmt.Errorf("invalid minter address: %w", err)
+ return errorsmod.Wrap(err, "invalid minter address")
}
if !minter.Equals(collectionMinter) {
- return fmt.Errorf("only the collection minter can mint NFTs")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs")
}
@@
if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil {
- return fmt.Errorf("failed to set NFT: %w", err)
+ return errorsmod.Wrap(err, "failed to set NFT")
}📝 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.
| func (k Keeper) MintNFT(ctx sdk.Context, collectionDenom string, minter sdk.AccAddress, owner sdk.AccAddress, metadata types.Nft) error { | |
| if strings.TrimSpace(metadata.TokenId) == "" { | |
| return fmt.Errorf("token ID cannot be empty") | |
| } | |
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | |
| has, err := k.NFTs.Has(ctx, nftKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to check NFT: %w", err) | |
| } | |
| if has { | |
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) | |
| } | |
| coll, err := k.Collections.Get(ctx, collectionDenom) | |
| if err != nil { | |
| return types.ErrCollectionNotFound | |
| } | |
| if coll.Minter == "" { | |
| return fmt.Errorf("minting disabled for this collection") | |
| } | |
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | |
| if err != nil { | |
| return fmt.Errorf("invalid minter address: %w", err) | |
| } | |
| if !minter.Equals(collectionMinter) { | |
| return fmt.Errorf("only the collection minter can mint NFTs") | |
| } | |
| // TODO: Charge fee if necessary | |
| metadata.Collection = collectionDenom | |
| metadata.Owner = owner.String() | |
| if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil { | |
| return fmt.Errorf("failed to set NFT: %w", err) | |
| } | |
| return k.incrementSupply(ctx, collectionDenom) | |
| } | |
| // At the top of x/nft/keeper/nft.go, update imports: | |
| import ( | |
| "fmt" | |
| "strings" | |
| "cosmossdk.io/collections" | |
| errorsmod "cosmossdk.io/errors" | |
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |
| ) | |
| func (k Keeper) MintNFT( | |
| ctx sdk.Context, | |
| collectionDenom string, | |
| minter sdk.AccAddress, | |
| owner sdk.AccAddress, | |
| metadata types.Nft, | |
| ) error { | |
| if strings.TrimSpace(metadata.TokenId) == "" { | |
| return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty") | |
| } | |
| nftKey := collections.Join(collectionDenom, metadata.TokenId) | |
| has, err := k.NFTs.Has(ctx, nftKey) | |
| if err != nil { | |
| return errorsmod.Wrap(err, "failed to check NFT") | |
| } | |
| if has { | |
| return sdkerrors.Wrapf( | |
| sdkerrors.ErrInvalidRequest, | |
| "nft %s already exists in collection %s", | |
| metadata.TokenId, | |
| collectionDenom, | |
| ) | |
| } | |
| coll, err := k.Collections.Get(ctx, collectionDenom) | |
| if err != nil { | |
| return types.ErrCollectionNotFound | |
| } | |
| if coll.Minter == "" { | |
| return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") | |
| } | |
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | |
| if err != nil { | |
| return errorsmod.Wrap(err, "invalid minter address") | |
| } | |
| if !minter.Equals(collectionMinter) { | |
| return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs") | |
| } | |
| // TODO: Charge fee if necessary | |
| metadata.Collection = collectionDenom | |
| metadata.Owner = owner.String() | |
| if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil { | |
| return errorsmod.Wrap(err, "failed to set NFT") | |
| } | |
| return k.incrementSupply(ctx, collectionDenom) | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 12 to 54, replace usages of fmt.Errorf with
Cosmos SDK typed errors and proper wrapping: use errorsmod.Wrap/Wrapf (or
sdkerrors.Wrap/Wrapf depending on SDK import) for returned errors that wrap
underlying errors (e.g., after NFTs.Has, sdk.AccAddressFromBech32, setNft), and
return the module/system typed errors for auth issues (use
types.ErrUnauthorized.Wrapf or types.ErrUnauthorized for the "minting disabled"
and "only the collection minter" cases) and types.ErrCollectionNotFound where
already used; ensure all wrapped errors include context (via Wrap/Warpf) and
keep the existing behavior of returning types.ErrCollectionNotFound for missing
collection and wrapping underlying errors for failure cases.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
x/nft/keeper/nft.go (1)
3-12: Switch to Cosmos SDK typed errors and consistent wrapping.Use sdkerrors/errorsmod for ABCI codes and consistent client UX. Also handle “minting disabled”/authorization with ErrUnauthorized. This consolidates prior feedback.
import ( "context" - "fmt" + "fmt" "strings" "cosmossdk.io/collections" "github.com/bitsongofficial/go-bitsong/x/nft/types" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + errorsmod "cosmossdk.io/errors" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) @@ if strings.TrimSpace(metadata.TokenId) == "" { - return fmt.Errorf("token ID cannot be empty") + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty") } @@ has, err := k.NFTs.Has(ctx, nftKey) if err != nil { - return fmt.Errorf("failed to check NFT: %w", err) + return errorsmod.Wrap(err, "failed to check NFT") } if has { - return fmt.Errorf("NFT with token ID %s already exists in collection %s", metadata.TokenId, collectionDenom) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "nft %s already exists in collection %s", metadata.TokenId, collectionDenom) } @@ if coll.Minter == "" { - return fmt.Errorf("minting disabled for this collection") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") } collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) if err != nil { - return fmt.Errorf("invalid minter address: %w", err) + return errorsmod.Wrap(err, "invalid minter address") } if !minter.Equals(collectionMinter) { - return fmt.Errorf("only the collection minter can mint NFTs") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs") } @@ if err := k.setNft(ctx, collectionDenom, metadata.TokenId, metadata); err != nil { - return fmt.Errorf("failed to set NFT: %w", err) + return errorsmod.Wrap(err, "failed to set NFT") } @@ nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) if err != nil { - return fmt.Errorf("failed to get NFT: %w", err) + return errorsmod.Wrap(err, "failed to get NFT") } @@ if nft.Owner != oldOwner.String() { - return fmt.Errorf("only the owner can transfer the NFT") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the owner can transfer the NFT") } @@ err = k.setNft(ctx, collectionDenom, tokenId, nft) if err != nil { - return fmt.Errorf("failed to set NFT Owner: %w", err) + return errorsmod.Wrap(err, "failed to set NFT owner") }Also applies to: 15-17, 21-26, 33-44, 51-53, 109-112, 114-116, 119-122
🧹 Nitpick comments (9)
x/nft/types/events.go (2)
5-14: Tidy up event names and make transfer event explicit
EventTypeTransferNftis defined but unused.- Consider aligning attribute naming with bank module conventions (
recipientvsreceiver) or keepreceiverconsistently across the module.
16-23: Emit a dedicated transfer event (include both sender and receiver)
NewNftReceivedEventonly includes the receiver; indexers typically need both ends of a transfer. Keep the received event if you want post-state semantics, but also emit a transfer event.Add this helper:
func NewNftTransferEvent(sender, receiver sdk.AccAddress, collection, tokenId string) sdk.Event { return sdk.NewEvent( EventTypeTransferNft, sdk.NewAttribute(AttributeKeySender, sender.String()), sdk.NewAttribute(AttributeKeyReceiver, receiver.String()), sdk.NewAttribute(AttributeKeyCollection, collection), sdk.NewAttribute(AttributeKeyTokenId, tokenId), ) }x/nft/types/keys_test.go (1)
10-19: Add negative tests (missing 0x00, empty parts, overlong denom)Current test only covers the happy path. Add coverage for failure modes:
func TestSplitNftKey_Errors(t *testing.T) { // no separator key := []byte("denom-without-sep") require.Panics(t, func() { types.MustSplitNftLengthPrefixedKey(key) }) // empty denom key = append([]byte{0}, []byte("1")...) require.Panics(t, func() { types.MustSplitNftLengthPrefixedKey(key) }) // empty tokenId key = append([]byte("denom"), 0) require.Panics(t, func() { types.MustSplitNftLengthPrefixedKey(key) }) // overlong denom long := strings.Repeat("a", types.MaxDenomLength+1) key = append(append([]byte(long), 0), []byte("1")...) require.Panics(t, func() { types.MustSplitNftLengthPrefixedKey(key) }) }x/nft/keeper/keeper_test.go (2)
67-69: Assert the specific error type for duplicate collectionStrengthen the assertion to ensure you catch regressions in error semantics:
-_, err = suite.keeper.CreateCollection(suite.ctx, creator, testCollection) -suite.Error(err) +_, err = suite.keeper.CreateCollection(suite.ctx, creator, testCollection) +suite.ErrorIs(err, types.ErrCollectionAlreadyExists)
139-151: Optional: add pagination assertions for AllNftsByOwnerAdd a case with a
PageRequest(limit=1) to verify pagination wiring, since you’ll expose this over gRPC.x/nft/keeper/nft.go (3)
55-55: Emit events on mint for parity with transfers.Consider emitting a “received” (or a dedicated “minted”) event so indexers can track mints uniformly.
// TODO: add events + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvent( + types.NewNftReceivedEvent(owner, collectionDenom, metadata.TokenId), + )
114-116: No-op when transferring to the same owner.Avoid an unnecessary write and event when oldOwner == newOwner.
if nft.Owner != oldOwner.String() { return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the owner can transfer the NFT") } + if nft.Owner == newOwner.String() { + return nil + }
98-101: Remove unused helpercreateNftDenom. It isn’t referenced anywhere; delete it or wire it where needed.x/nft/keeper/collection.go (1)
13-26: Set creator deterministically; optionally validate symbol.Prevent caller-controlled Creator and ensure minimal metadata hygiene.
func (k Keeper) CreateCollection(ctx context.Context, creator sdk.AccAddress, coll types.Collection) (denom string, err error) { + // enforce creator from signer + coll.Creator = creator.String() + denom, err = k.validateCollectionDenom(ctx, creator, coll.Symbol) if err != nil { return "", err } // TODO: charge fee @@ if err := k.setCollection(ctx, denom, coll); err != nil { return "", err } return denom, nil }Optionally reject empty/whitespace symbols:
-import ( +import ( "context" "fmt" + "strings" @@ func (k Keeper) CreateCollection(ctx context.Context, creator sdk.AccAddress, coll types.Collection) (denom string, err error) { + if strings.TrimSpace(coll.Symbol) == "" { + return "", sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "symbol cannot be empty") + }(Note: add
sdkerrorsimport if you adopt this.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
x/nft/keeper/collection.go(1 hunks)x/nft/keeper/collection_test.go(1 hunks)x/nft/keeper/grpc_query.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/events.go(1 hunks)x/nft/types/expected_keeper.go(1 hunks)x/nft/types/keys.go(1 hunks)x/nft/types/keys_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/nft/types/expected_keeper.go
- x/nft/keeper/collection_test.go
- x/nft/keeper/grpc_query.go
🧰 Additional context used
🧬 Code graph analysis (5)
x/nft/types/keys_test.go (1)
x/nft/types/keys.go (1)
MustSplitNftLengthPrefixedKey(50-57)
x/nft/keeper/keeper.go (3)
x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/keys.go (7)
NFTsByCollectionPrefix(24-24)NFTsByOwnerPrefix(25-25)StoreKey(14-14)ModuleName(13-13)CollectionsPrefix(21-21)SupplyPrefix(22-22)NFTsPrefix(23-23)x/nft/types/expected_keeper.go (1)
AccountKeeper(10-17)
x/nft/keeper/keeper_test.go (4)
app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (6)
Collection(26-34)Collection(38-38)Collection(39-41)Nft(69-76)Nft(80-80)Nft(81-83)x/nft/types/query.pb.go (3)
QueryAllNftsByOwnerRequest(407-410)QueryAllNftsByOwnerRequest(414-414)QueryAllNftsByOwnerRequest(415-417)
x/nft/keeper/collection.go (4)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (3)
Collection(26-34)Collection(38-38)Collection(39-41)x/fantoken/types/validation.go (1)
ValidateDenom(34-40)x/nft/types/errors.go (2)
ErrCollectionAlreadyExists(6-6)ErrCollectionNotFound(7-7)
x/nft/keeper/nft.go (4)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (6)
Nft(69-76)Nft(80-80)Nft(81-83)Collection(26-34)Collection(38-38)Collection(39-41)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)x/nft/types/events.go (6)
EventTypeTransferNft(6-6)AttributeKeyReceiver(10-10)AttributeKeySender(9-9)AttributeKeyCollection(12-12)AttributeKeyTokenId(13-13)NewNftReceivedEvent(16-23)
🔇 Additional comments (7)
x/nft/types/keys.go (3)
12-18: Clarify/justify MaxDenomLength (43)Hard-coding 43 looks tailored to the current denom scheme (e.g., "nft" + 40-hex). If denom derivation changes (new prefix/format), this becomes brittle. Add a short doc comment explaining why 43, or reference the derivation utility; alternatively, expose it via a single source of truth used by both derivation and validation.
20-26: Store prefixes look goodPrefix layout is clear and future-proof for indexing.
50-57: Panic helper is fine; ensure only used with trusted keys
MustSplitNftLengthPrefixedKeyis okay for internal, trusted keys. Avoid using it on user input or untrusted storage keys.x/nft/keeper/keeper.go (1)
59-64: Keeper schema and maps look solidSchema, maps, and indexes are set up idiomatically; good use of
IndexedMapfor collection and owner queries.x/nft/keeper/nft.go (2)
51-53: Persist-then-increment ordering looks good.Writing the NFT before bumping supply prevents inflated supply on failed writes.
Also applies to: 57-57
66-75: Account auto-creation + event emission mirrors bank module.Good alignment with SDK patterns; costs are controlled and telemetry is captured.
Also applies to: 81-93
x/nft/keeper/collection.go (1)
61-63: Denom hashing fixed to lowercase with delimiter — LGTM.This satisfies sdk.ValidateDenom and removes collision ambiguity between creator and symbol.
| func newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes { | ||
| return NFTIndexes{ | ||
| Collection: indexes.NewMulti( | ||
| sb, | ||
| types.NFTsByCollectionPrefix, | ||
| "nfts_by_collection", | ||
| collections.StringKey, | ||
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | ||
| func(pk collections.Pair[string, string], v types.Nft) (string, error) { | ||
| return v.Collection, nil | ||
| }, | ||
| ), | ||
| Owner: indexes.NewMulti( | ||
| sb, | ||
| types.NFTsByOwnerPrefix, | ||
| "nfts_by_owner", | ||
| sdk.AccAddressKey, | ||
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | ||
| func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { | ||
| return sdk.AccAddressFromBech32(v.Owner) | ||
| }, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use AddressCodec for owner index instead of hardcoding Bech32
Decouple from Bech32 assumptions by injecting address.Codec. This supports custom HRPs and future-proofing.
-func newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes {
+func newNFTIndexes(sb *collections.SchemaBuilder, addrCodec address.Codec) NFTIndexes {
return NFTIndexes{
Collection: indexes.NewMulti(
sb,
types.NFTsByCollectionPrefix,
"nfts_by_collection",
collections.StringKey,
collections.PairKeyCodec(collections.StringKey, collections.StringKey),
func(pk collections.Pair[string, string], v types.Nft) (string, error) {
return v.Collection, nil
},
),
Owner: indexes.NewMulti(
sb,
types.NFTsByOwnerPrefix,
"nfts_by_owner",
sdk.AccAddressKey,
collections.PairKeyCodec(collections.StringKey, collections.StringKey),
- func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) {
- return sdk.AccAddressFromBech32(v.Owner)
- },
+ func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) {
+ bz, err := addrCodec.StringToBytes(v.Owner)
+ return sdk.AccAddress(bz), err
+ },
),
}
}📝 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.
| func newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes { | |
| return NFTIndexes{ | |
| Collection: indexes.NewMulti( | |
| sb, | |
| types.NFTsByCollectionPrefix, | |
| "nfts_by_collection", | |
| collections.StringKey, | |
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | |
| func(pk collections.Pair[string, string], v types.Nft) (string, error) { | |
| return v.Collection, nil | |
| }, | |
| ), | |
| Owner: indexes.NewMulti( | |
| sb, | |
| types.NFTsByOwnerPrefix, | |
| "nfts_by_owner", | |
| sdk.AccAddressKey, | |
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | |
| func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { | |
| return sdk.AccAddressFromBech32(v.Owner) | |
| }, | |
| ), | |
| } | |
| } | |
| func newNFTIndexes(sb *collections.SchemaBuilder, addrCodec address.Codec) NFTIndexes { | |
| return NFTIndexes{ | |
| Collection: indexes.NewMulti( | |
| sb, | |
| types.NFTsByCollectionPrefix, | |
| "nfts_by_collection", | |
| collections.StringKey, | |
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | |
| func(pk collections.Pair[string, string], v types.Nft) (string, error) { | |
| return v.Collection, nil | |
| }, | |
| ), | |
| Owner: indexes.NewMulti( | |
| sb, | |
| types.NFTsByOwnerPrefix, | |
| "nfts_by_owner", | |
| sdk.AccAddressKey, | |
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | |
| func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { | |
| bz, err := addrCodec.StringToBytes(v.Owner) | |
| return sdk.AccAddress(bz), err | |
| }, | |
| ), | |
| } | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/keeper.go around lines 20 to 43, the owner index currently
decodes addresses using sdk.AccAddressFromBech32 which hardcodes Bech32; change
newNFTIndexes to accept an address.Codec (e.g. add an addrCodec parameter),
replace the direct sdk.AccAddressFromBech32 call with decoding via the injected
addrCodec (use the codec's string->bytes/string->address API and convert the
resulting bytes to sdk.AccAddress), and update all call sites to pass the
module's address.Codec so the index no longer assumes Bech32 and supports custom
HRPs.
| func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, storeService store.KVStoreService, ak types.AccountKeeper, logger log.Logger) Keeper { | ||
| /*if addr := ak.GetModuleAddress(types.ModuleName); addr == nil { | ||
| panic("the " + types.ModuleName + " module account has not been set") | ||
| }*/ | ||
|
|
||
| // TODO: validate all metadata length | ||
|
|
||
| logger = logger.With(log.ModuleKey, "x/"+types.ModuleName) | ||
|
|
||
| sb := collections.NewSchemaBuilder(storeService) | ||
|
|
||
| k := Keeper{ | ||
| cdc: cdc, | ||
| storeKey: key, | ||
| storeService: storeService, | ||
| ak: ak, | ||
| logger: logger, | ||
| // TODO: fix the store once we add queries | ||
| Collections: collections.NewMap(sb, types.CollectionsPrefix, "collections", collections.StringKey, codec.CollValue[types.Collection](cdc)), | ||
| Supply: collections.NewMap(sb, types.SupplyPrefix, "supply", collections.StringKey, sdk.IntValue), | ||
| NFTs: collections.NewIndexedMap( | ||
| sb, | ||
| types.NFTsPrefix, | ||
| "nfts", | ||
| collections.PairKeyCodec(collections.StringKey, collections.StringKey), | ||
| codec.CollValue[types.Nft](cdc), | ||
| newNFTIndexes(sb), | ||
| ), | ||
| } | ||
|
|
||
| schema, err := sb.Build() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| k.Schema = schema | ||
| return k | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pass AddressCodec from AccountKeeper into indexes
Wire the codec from ak:
-func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, storeService store.KVStoreService, ak types.AccountKeeper, logger log.Logger) Keeper {
+func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, storeService store.KVStoreService, ak types.AccountKeeper, logger log.Logger) Keeper {
@@
- k := Keeper{
+ k := Keeper{
@@
- NFTs: collections.NewIndexedMap(
+ NFTs: collections.NewIndexedMap(
sb,
types.NFTsPrefix,
"nfts",
collections.PairKeyCodec(collections.StringKey, collections.StringKey),
codec.CollValue[types.Nft](cdc),
- newNFTIndexes(sb),
+ newNFTIndexes(sb, ak.AddressCodec()),
),
}🏁 Script executed:
#!/bin/bash
rg -nP 'type\s+AccountKeeper\b' -C3 --type=go
rg -nP 'AddressCodec' -C3 --type=go
rg -nP 'newNFTIndexes' -C3 --type=goLength of output: 7785
🏁 Script executed:
#!/bin/bash
rg -nP 'type\s+NFTIndexes\b' -C3 --type=goLength of output: 466
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'func newNFTIndexes' --type=go x/nft/keeper/keeper.goLength of output: 475
🏁 Script executed:
#!/bin/bash
sed -n '15,80p' x/nft/keeper/keeper.goLength of output: 2141
Add AddressCodec parameter to newNFTIndexes and wire ak.AddressCodec()
- In
x/nft/keeper/keeper.go, changetofunc newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes
and inside thefunc newNFTIndexes(sb *collections.SchemaBuilder, ac address.Codec) NFTIndexes
Ownerindex extractor replacewithreturn sdk.AccAddressFromBech32(v.Owner)
return ac.StringToBytes(v.Owner)
- In
NewKeeper, update the call fromtonewNFTIndexes(sb),
newNFTIndexes(sb, ak.AddressCodec()),
🤖 Prompt for AI Agents
In x/nft/keeper/keeper.go around lines 66 to 102, the call to newNFTIndexes(sb)
must be changed to newNFTIndexes(sb, ak.AddressCodec()); update the function
call here to pass ak.AddressCodec() as the second argument. Also update the
newNFTIndexes function signature (where it's defined) from func newNFTIndexes(sb
*collections.SchemaBuilder) NFTIndexes to func newNFTIndexes(sb
*collections.SchemaBuilder, ac address.Codec) NFTIndexes and inside its Owner
index extractor replace sdk.AccAddressFromBech32(v.Owner) with
ac.StringToBytes(v.Owner) so the address codec from the account keeper is used
for encoding/decoding owner addresses.
| func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) { | ||
| parts := bytes.SplitN(key, []byte{0}, 2) | ||
| if len(parts) != 2 { | ||
| return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts, got %d", len(parts)) | ||
| } | ||
|
|
||
| denomLen := len(parts[0]) | ||
|
|
||
| if denomLen > MaxDenomLength { | ||
| return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "decoded denom key length %d exceeds max allowed length %d", denomLen, MaxDenomLength) | ||
| } | ||
|
|
||
| if len(key)-1 < denomLen { | ||
| return nil, nil, fmt.Errorf("key is malformed: length prefix %d is greater than tokenId bytes %d", denomLen, len(key)-1) | ||
| } | ||
|
|
||
| denom = parts[0] | ||
| tokenId = parts[1] | ||
|
|
||
| return denom, tokenId, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix ineffective validation and misleading function name
- The function name says “length-prefixed” but the implementation is zero-delimited. Either rename or implement true length-prefixing.
- The check
if len(key)-1 < denomLenis always false for well-formeddenom\x00tokenIdand doesn’t validate anything. - Missing validation for empty
denomor emptytokenId.
Minimal, backward-compatible fix while keeping the name:
func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) {
- parts := bytes.SplitN(key, []byte{0}, 2)
- if len(parts) != 2 {
- return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts, got %d", len(parts))
- }
-
- denomLen := len(parts[0])
-
- if denomLen > MaxDenomLength {
- return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "decoded denom key length %d exceeds max allowed length %d", denomLen, MaxDenomLength)
- }
-
- if len(key)-1 < denomLen {
- return nil, nil, fmt.Errorf("key is malformed: length prefix %d is greater than tokenId bytes %d", denomLen, len(key)-1)
- }
-
- denom = parts[0]
- tokenId = parts[1]
-
- return denom, tokenId, nil
+ parts := bytes.SplitN(key, []byte{0}, 2)
+ if len(parts) != 2 {
+ return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts separated by 0x00")
+ }
+ denom, tokenId = parts[0], parts[1]
+ if len(denom) == 0 {
+ return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "denom is empty")
+ }
+ if len(denom) > MaxDenomLength {
+ return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "denom length %d exceeds max allowed %d", len(denom), MaxDenomLength)
+ }
+ if len(tokenId) == 0 {
+ return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is empty")
+ }
+ return denom, tokenId, nil
}Optionally, rename to SplitNftZeroDelimitedKey to reflect reality.
📝 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.
| func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) { | |
| parts := bytes.SplitN(key, []byte{0}, 2) | |
| if len(parts) != 2 { | |
| return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts, got %d", len(parts)) | |
| } | |
| denomLen := len(parts[0]) | |
| if denomLen > MaxDenomLength { | |
| return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "decoded denom key length %d exceeds max allowed length %d", denomLen, MaxDenomLength) | |
| } | |
| if len(key)-1 < denomLen { | |
| return nil, nil, fmt.Errorf("key is malformed: length prefix %d is greater than tokenId bytes %d", denomLen, len(key)-1) | |
| } | |
| denom = parts[0] | |
| tokenId = parts[1] | |
| return denom, tokenId, nil | |
| } | |
| func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) { | |
| parts := bytes.SplitN(key, []byte{0}, 2) | |
| if len(parts) != 2 { | |
| return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts separated by 0x00") | |
| } | |
| denom, tokenId = parts[0], parts[1] | |
| if len(denom) == 0 { | |
| return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "denom is empty") | |
| } | |
| if len(denom) > MaxDenomLength { | |
| return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "denom length %d exceeds max allowed %d", len(denom), MaxDenomLength) | |
| } | |
| if len(tokenId) == 0 { | |
| return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is empty") | |
| } | |
| return denom, tokenId, nil | |
| } |
- Refactored MintNFT function to accept individual metadata fields (tokenId, name, description, uri) instead of a single metadata object. - Added validateNftMetadata function to enforce constraints on NFT metadata, including length checks for tokenId, name, description, and uri. - Introduced Editions field in the Nft struct to track the number of editions for each NFT. - Implemented GetNft function to retrieve NFT details by collection and token ID. - Updated setNft function to accept a complete Nft object. - Added incrementEdition function to increase the edition count for a specific NFT. - Updated protobuf definitions to include the new Editions field and added a new Edition type for tracking individual NFT editions. - Adjusted key definitions to accommodate new fields and prefixes.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
x/nft/keeper/keeper.go (1)
20-43: Use AddressCodec for owner index (don’t hardcode Bech32)Inject address.Codec and decode via it to support custom HRPs; update call sites accordingly.
-import ( +import ( "cosmossdk.io/collections" "cosmossdk.io/collections/indexes" "cosmossdk.io/core/store" "cosmossdk.io/log" "cosmossdk.io/math" storetypes "cosmossdk.io/store/types" "github.com/bitsongofficial/go-bitsong/x/nft/types" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" + "cosmossdk.io/core/address" ) @@ -type NFTIndexes struct { +type NFTIndexes struct { Collection *indexes.Multi[string, collections.Pair[string, string], types.Nft] Owner *indexes.Multi[sdk.AccAddress, collections.Pair[string, string], types.Nft] } -func newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes { +func newNFTIndexes(sb *collections.SchemaBuilder, addrCodec address.Codec) NFTIndexes { return NFTIndexes{ @@ - Owner: indexes.NewMulti( + Owner: indexes.NewMulti( sb, types.NFTsByOwnerPrefix, "nfts_by_owner", sdk.AccAddressKey, collections.PairKeyCodec(collections.StringKey, collections.StringKey), - func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { - return sdk.AccAddressFromBech32(v.Owner) - }, + func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { + bz, err := addrCodec.StringToBytes(v.Owner) + return sdk.AccAddress(bz), err + }, ), } }@@ - NFTs: collections.NewIndexedMap( + NFTs: collections.NewIndexedMap( sb, types.NFTsPrefix, "nfts", collections.PairKeyCodec(collections.StringKey, collections.StringKey), codec.CollValue[types.Nft](cdc), - newNFTIndexes(sb), + newNFTIndexes(sb, ak.AddressCodec()), ),proto/bitsong/nft/v1beta1/nft.proto (1)
4-4: buf build will fail: missing gogoproto dependencyAdd gogoproto to buf deps (or vendor it).
# buf.yaml +version: v2 +deps: + - buf.build/cosmos/gogo-proto +# If using address/cosmos annotations elsewhere, also consider: +# - buf.build/googleapis/googleapis +# - buf.build/cosmos/cosmos-proto +# - buf.build/cosmos/cosmos-sdkx/nft/types/keys.go (1)
35-55: Fix misnamed/misimplemented key splitter; validate empties and drop bogus length checkName says “length-prefixed” but it’s zero-delimited; also the length check is meaningless. Tighten validation.
func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) { - parts := bytes.SplitN(key, []byte{0}, 2) - if len(parts) != 2 { - return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts, got %d", len(parts)) - } - - denomLen := len(parts[0]) - - if denomLen > MaxDenomLength { - return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "decoded denom key length %d exceeds max allowed length %d", denomLen, MaxDenomLength) - } - - if len(key)-1 < denomLen { - return nil, nil, fmt.Errorf("key is malformed: length prefix %d is greater than tokenId bytes %d", denomLen, len(key)-1) - } - - denom = parts[0] - tokenId = parts[1] - - return denom, tokenId, nil + parts := bytes.SplitN(key, []byte{0x00}, 2) + if len(parts) != 2 { + return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts separated by 0x00") + } + denom, tokenId = parts[0], parts[1] + if len(denom) == 0 { + return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "denom is empty") + } + if len(denom) > MaxDenomLength { + return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "denom length %d exceeds max allowed %d", len(denom), MaxDenomLength) + } + if len(tokenId) == 0 { + return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is empty") + } + return denom, tokenId, nil }proto/bitsong/nft/v1beta1/query.proto (1)
12-44: Add planned AllNftInfo RPC and messages (nft + owner)Aligns with PR objectives and reduces client round-trips.
service Query { @@ rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) { option (cosmos.query.v1.module_query_safe) = true; option (google.api.http).get = "/bitsong/nft/v1beta1/collections/{collection}/{token_id}"; } + + rpc AllNftInfo(QueryAllNftInfoRequest) returns (QueryAllNftInfoResponse) { + option (cosmos.query.v1.module_query_safe) = true; + option (google.api.http).get = "/bitsong/nft/v1beta1/all_nft_info/{collection}/{token_id}"; + } @@ } @@ message QueryNftInfoResponse { option (gogoproto.equal) = false; option (gogoproto.goproto_getters) = false; bitsong.nft.v1beta1.Nft nft = 1; } + +message QueryAllNftInfoRequest { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; + string collection = 1; + string token_id = 2; +} + +message QueryAllNftInfoResponse { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; + bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false]; + string owner = 2; +}Follow up: implement in keeper grpc_query.go.
Also applies to: 89-103
🧹 Nitpick comments (13)
x/nft/keeper/edition_test.go (2)
47-56: Also assert persisted counter (nft.Editions) after printing 3 editionsVerifies state, not just return value.
suite.NoError(err) suite.Equal(uint64(3), edition) + + // editions counter should be persisted on the NFT + nft, err := suite.keeper.GetNft(suite.ctx, collectionDenom, "1") + suite.NoError(err) + suite.Equal(uint64(3), nft.Editions) }
3-56: Add negative tests (non-minter caller, unknown NFT, zero/empty tokenId)Covers authorization and input validation paths for PrintEdition. I can draft them if useful.
x/nft/keeper/edition.go (3)
34-36: Clarify “minter disabled” vs. “unauthorized minter”When collectionMinter is unset (minting disabled per PR spec), return a dedicated error (e.g., ErrUnauthorized/ErrMintingDisabled) instead of the generic “only the collection minter…” message.
21-27: Use module error types, not fmt.Errorf, for gRPC-friendly codesPrefer x/nft/types errors (e.g., ErrNotFound, ErrUnauthorized) to ensure consistent ABCI codes.
12-18: Add basic input validation (tokenId, owner)
- tokenId must be non-empty
- owner must be non-empty, valid address (use AccountKeeper.AddressCodec()).
Also applies to: 40-45
x/nft/keeper/keeper_test.go (2)
34-35: Avoid brittle hash-bound denom assertionHard-coding expectedDenom1 ties the test to a specific hashing scheme. Prefer asserting non-empty and round-tripping via keeper queries, or computing the denom via the same helper used in CreateCollection if exposed.
Also applies to: 90-91, 114-116
120-149: Add negative mint testsAdd cases for: non-minter mint attempt, duplicate token_id, and empty metadata fields to lock the contract.
x/nft/keeper/keeper.go (1)
67-73: Decide on the module account checkEither enforce the ModuleAccount presence (panic early) or remove the dead code comment to avoid drift.
proto/bitsong/nft/v1beta1/nft.proto (2)
21-23: Typo: “update_authority”Fix comment spelling.
- // update_autority (who can update name, description and uri if is_mutable = true) + // update_authority (who can update name, description and uri if is_mutable = true)Also applies to: 40-44
18-20: Annotate address fields for clarity and toolingOptionally mark creator/minter/owner as cosmos address strings to aid validation and codegen.
+import "cosmos_proto/cosmos.proto"; @@ - string creator = 6; - string minter = 7; + string creator = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + string minter = 7 [(cosmos_proto.scalar) = "cosmos.AddressString"]; @@ - string owner = 6; + string owner = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"];Note: add buf deps for cosmos-proto if not already present.
Also applies to: 35-36, 54-54
x/nft/types/keys.go (1)
57-64: Optional: Rename NFT key helper to reflect zero-delimited encoding or remove itRename
SplitNftLengthPrefixedKey/MustSplitNftLengthPrefixedKeytoSplitNftZeroDelimitedKey/MustSplitNftZeroDelimitedKey(or delete both and usecollections.Paircodec); update call sites inx/nft/keeper/grpc_query.go(L156) andx/nft/types/keys_test.go(L16).x/nft/keeper/collection.go (1)
139-153: Return typed validation errors for metadata length limitsAlign with sdkerrors.ErrInvalidRequest for client-friendly codes.
func (k Keeper) validateCollectionMetadata(name, description, uri string) error { if len(name) > types.MaxNameLength { - return fmt.Errorf("name cannot be longer than %d characters", types.MaxNameLength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "name cannot be longer than %d characters", types.MaxNameLength) } if len(description) > types.MaxDescriptionLength { - return fmt.Errorf("description cannot be longer than %d characters", types.MaxDescriptionLength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "description cannot be longer than %d characters", types.MaxDescriptionLength) } if len(uri) > types.MaxURILength { - return fmt.Errorf("uri cannot be longer than %d characters", types.MaxURILength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "uri cannot be longer than %d characters", types.MaxURILength) } return nil }Note: add imports if you accept.
x/nft/keeper/nft.go (1)
156-159: Remove unused createNftDenom or document its intended use
The methodcreateNftDenomin x/nft/keeper/nft.go (lines 156–159) isn’t referenced anywhere; either delete it to avoid confusion or add a comment explaining its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
x/nft/types/nft.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (11)
proto/bitsong/nft/v1beta1/nft.proto(1 hunks)proto/bitsong/nft/v1beta1/query.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/collection_test.go(1 hunks)x/nft/keeper/edition.go(1 hunks)x/nft/keeper/edition_test.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/keys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/nft/keeper/collection_test.go
- x/nft/keeper/grpc_query_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
x/nft/keeper/edition_test.go (1)
x/nft/keeper/keeper_test.go (1)
KeeperTestSuite(58-64)
x/nft/keeper/edition.go (2)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (6)
Edition(114-119)Edition(123-123)Edition(124-126)Collection(26-35)Collection(39-39)Collection(40-42)
x/nft/keeper/keeper.go (3)
x/nft/types/nft.pb.go (9)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)Edition(114-119)Edition(123-123)Edition(124-126)x/nft/types/keys.go (8)
NFTsByCollectionPrefix(30-30)NFTsByOwnerPrefix(31-31)StoreKey(14-14)ModuleName(13-13)CollectionsPrefix(27-27)SupplyPrefix(28-28)NFTsPrefix(29-29)EditionsPrefix(32-32)x/nft/types/expected_keeper.go (1)
AccountKeeper(10-17)
x/nft/keeper/collection.go (4)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (3)
Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/errors.go (2)
ErrCollectionNotFound(7-7)ErrCollectionAlreadyExists(6-6)x/nft/types/keys.go (4)
MaxSymbolLength(20-20)MaxNameLength(21-21)MaxDescriptionLength(22-22)MaxURILength(23-23)
x/nft/keeper/keeper_test.go (4)
x/nft/types/nft.pb.go (6)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/query.pb.go (3)
QueryAllNftsByOwnerRequest(407-410)QueryAllNftsByOwnerRequest(414-414)QueryAllNftsByOwnerRequest(415-417)
x/nft/keeper/nft.go (5)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)x/nft/types/nft.pb.go (6)
Nft(70-79)Nft(83-83)Nft(84-86)Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/events.go (6)
EventTypeTransferNft(6-6)AttributeKeyReceiver(10-10)AttributeKeySender(9-9)AttributeKeyCollection(12-12)AttributeKeyTokenId(13-13)NewNftReceivedEvent(16-23)x/nft/types/keys.go (4)
MaxTokenIdLength(19-19)MaxNameLength(21-21)MaxDescriptionLength(22-22)MaxURILength(23-23)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/query.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
| import "gogoproto/gogo.proto"; | ||
| import "google/api/annotations.proto"; | ||
| import "cosmos/query/v1/query.proto"; | ||
| import "cosmos/base/query/v1beta1/pagination.proto"; | ||
| import "bitsong/nft/v1beta1/nft.proto"; |
There was a problem hiding this comment.
buf build will fail: missing gogoproto (and likely cosmos/google) deps
Add deps to buf.yaml (or vendor).
Example buf.yaml:
version: v2
deps:
- buf.build/cosmos/gogo-proto
- buf.build/googleapis/googleapis
- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/cosmos-sdkThen:
- buf mod update
- buf generate
🧰 Tools
🪛 Buf (1.55.1)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/query.proto around lines 4 to 8, the file imports
external proto packages (gogoproto, google api, cosmos query) but the
repository's buf.yaml lacks those deps causing buf build/generate to fail;
update buf.yaml to add the missing module dependencies (for example add entries
for gogo-proto, googleapis, and the relevant cosmos proto modules) or vendor
those protos, then run buf mod update and buf generate to fetch modules and
regenerate artifacts.
x/nft/keeper/collection.go
Outdated
| func (k Keeper) GetMinter(ctx context.Context, denom string) (sdk.AccAddress, error) { | ||
| coll, err := k.Collections.Get(ctx, denom) | ||
| if err != nil { | ||
| return nil, types.ErrCollectionNotFound | ||
| } | ||
|
|
||
| if coll.Minter == "" { | ||
| return nil, fmt.Errorf("minting disabled for this collection") | ||
| } | ||
|
|
||
| return sdk.AccAddressFromBech32(coll.Minter) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed Cosmos SDK errors for minter checks
Return standard ABCI codes and wrap parsing errors.
import (
"context"
"fmt"
"strings"
"cosmossdk.io/math"
+ errorsmod "cosmossdk.io/errors"
"github.com/bitsongofficial/go-bitsong/x/nft/types"
tmcrypto "github.com/cometbft/cometbft/crypto"
sdk "github.com/cosmos/cosmos-sdk/types"
+ sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
@@
func (k Keeper) GetMinter(ctx context.Context, denom string) (sdk.AccAddress, error) {
coll, err := k.Collections.Get(ctx, denom)
if err != nil {
return nil, types.ErrCollectionNotFound
}
if coll.Minter == "" {
- return nil, fmt.Errorf("minting disabled for this collection")
+ return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection")
}
collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter)
if err != nil {
- return nil, fmt.Errorf("invalid minter address: %w", err)
+ return nil, errorsmod.Wrap(err, "invalid minter address")
}
return collectionMinter, nil
}📝 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.
| func (k Keeper) GetMinter(ctx context.Context, denom string) (sdk.AccAddress, error) { | |
| coll, err := k.Collections.Get(ctx, denom) | |
| if err != nil { | |
| return nil, types.ErrCollectionNotFound | |
| } | |
| if coll.Minter == "" { | |
| return nil, fmt.Errorf("minting disabled for this collection") | |
| } | |
| return sdk.AccAddressFromBech32(coll.Minter) | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "strings" | |
| "cosmossdk.io/math" | |
| errorsmod "cosmossdk.io/errors" | |
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | |
| tmcrypto "github.com/cometbft/cometbft/crypto" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |
| ) | |
| func (k Keeper) GetMinter(ctx context.Context, denom string) (sdk.AccAddress, error) { | |
| coll, err := k.Collections.Get(ctx, denom) | |
| if err != nil { | |
| return nil, types.ErrCollectionNotFound | |
| } | |
| if coll.Minter == "" { | |
| return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") | |
| } | |
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | |
| if err != nil { | |
| return nil, errorsmod.Wrap(err, "invalid minter address") | |
| } | |
| return collectionMinter, nil | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/collection.go around lines 70 to 81, replace the plain
string/error returns with typed Cosmos SDK errors: when coll.Minter == "" return
a standard SDK error (e.g. sdkerrors.ErrInvalidRequest wrapped with a clear
message like "minting disabled for this collection") instead of fmt.Errorf, and
when parsing the bech32 address wrap the parsing error with a typed SDK error
(e.g. sdkerrors.ErrInvalidAddress) using sdkerrors.Wrapf to include the original
parse error; ensure imports include the SDK errors package.
| if err := k.setEdition(ctx, edition); err != nil { | ||
| return 0, fmt.Errorf("failed to set edition: %w", err) | ||
| } | ||
|
|
||
| if err := k.incrementEdition(ctx, collectionDenom, tokenId); err != nil { | ||
| return 0, fmt.Errorf("failed to increment edition: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent store inconsistency: rollback Edition if increment fails
If incrementEdition errors after setEdition, you’ll persist an Edition with a stale nft.Editions counter. Remove the written edition on failure.
if err := k.setEdition(ctx, edition); err != nil {
return 0, fmt.Errorf("failed to set edition: %w", err)
}
- if err := k.incrementEdition(ctx, collectionDenom, tokenId); err != nil {
- return 0, fmt.Errorf("failed to increment edition: %w", err)
- }
+ if err := k.incrementEdition(ctx, collectionDenom, tokenId); err != nil {
+ // rollback the written edition to keep state consistent
+ _ = k.Editions.Remove(ctx, collections.Join3(collectionDenom, tokenId, edition.Seq))
+ return 0, fmt.Errorf("failed to increment edition: %w", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "cosmossdk.io/collections" | ||
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | ||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch to typed Cosmos SDK errors; enforce planned “minter nullable disables minting” semantics
Consistent codes improve client UX; wrap underlying errors.
import (
"context"
"fmt"
"strings"
"cosmossdk.io/collections"
+ errorsmod "cosmossdk.io/errors"
"github.com/bitsongofficial/go-bitsong/x/nft/types"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
+ sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
@@
has, err := k.NFTs.Has(ctx, nftKey)
if err != nil {
- return fmt.Errorf("failed to check NFT: %w", err)
+ return errorsmod.Wrap(err, "failed to check NFT")
}
if has {
- return fmt.Errorf("NFT with token ID %s already exists in collection %s", tokenId, collectionDenom)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "nft %s already exists in collection %s", tokenId, collectionDenom)
}
@@
coll, err := k.Collections.Get(ctx, collectionDenom)
if err != nil {
return types.ErrCollectionNotFound
}
if coll.Minter == "" {
- return fmt.Errorf("minting disabled for this collection")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection")
}
collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter)
if err != nil {
- return fmt.Errorf("invalid minter address: %w", err)
+ return errorsmod.Wrap(err, "invalid minter address")
}
if !minter.Equals(collectionMinter) {
- return fmt.Errorf("only the collection minter can mint NFTs")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs")
}
@@
if err := k.setNft(ctx, nft); err != nil {
- return fmt.Errorf("failed to set NFT: %w", err)
+ return errorsmod.Wrap(err, "failed to set NFT")
}
@@
has, err := k.NFTs.Has(ctx, nftKey)
if err != nil {
- return nil, fmt.Errorf("failed to check NFT: %w", err)
+ return nil, errorsmod.Wrap(err, "failed to check NFT")
}
if !has {
- return nil, fmt.Errorf("NFT with token ID %s does not exist in collection %s", tokenId, collectionDenom)
+ return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "nft %s not found in collection %s", tokenId, collectionDenom)
}
nft, err := k.NFTs.Get(ctx, nftKey)
if err != nil {
- return nil, fmt.Errorf("failed to get NFT: %w", err)
+ return nil, errorsmod.Wrap(err, "failed to get NFT")
}
@@
if strings.TrimSpace(tokenId) == "" {
- return fmt.Errorf("token ID cannot be empty")
+ return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty")
}
if len(tokenId) > types.MaxTokenIdLength {
- return fmt.Errorf("token ID length exceeds maximum of %d", types.MaxTokenIdLength)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token_id length exceeds maximum of %d", types.MaxTokenIdLength)
}
if len(name) > types.MaxNameLength {
- return fmt.Errorf("name length exceeds maximum of %d", types.MaxNameLength)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "name length exceeds maximum of %d", types.MaxNameLength)
}
if len(description) > types.MaxDescriptionLength {
- return fmt.Errorf("description length exceeds maximum of %d", types.MaxDescriptionLength)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "description length exceeds maximum of %d", types.MaxDescriptionLength)
}
if len(uri) > types.MaxURILength {
- return fmt.Errorf("URI length exceeds maximum of %d", types.MaxURILength)
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "uri length exceeds maximum of %d", types.MaxURILength)
}
@@
if nft.Owner != oldOwner.String() {
- return fmt.Errorf("only the owner can transfer the NFT")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the owner can transfer the NFT")
}
@@
err = k.setNft(ctx, nft)
if err != nil {
- return fmt.Errorf("failed to set NFT Owner: %w", err)
+ return errorsmod.Wrap(err, "failed to set NFT owner")
}
@@
nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId))
if err != nil {
- return fmt.Errorf("failed to get NFT: %w", err)
+ return errorsmod.Wrap(err, "failed to get NFT")
}
nft.Editions += 1
return k.setNft(ctx, nft)Also applies to: 28-35, 37-53, 67-69, 114-127, 132-151, 166-180, 191-199
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 3-12 (and similarly update the other ranges
28-35, 37-53, 67-69, 114-127, 132-151, 166-180, 191-199), replace plain
fmt.Errorf/errors.New or untyped errors with Cosmos SDK typed errors (use
github.com/cosmos/cosmos-sdk/types/errors as sdkerrors and the module-specific
errors registered in x/nft/types); ensure you Wrap or Wrapf underlying errors so
context is preserved (sdkerrors.Wrap/Wrapf or
sdkerrors.Wrapf(types.ErrInvalidRequest, "...: %v", err)). Also enforce the
planned “minter nullable disables minting” semantics by checking for a nil/empty
minter where minting is allowed and returning the appropriate typed error (e.g.,
types.ErrUnauthorized or a new types.ErrMintingDisabled registered via
sdkerrors.Register) instead of proceeding or returning a plain error; update all
affected branches in the listed line ranges accordingly.
…rvice - Deleted request and local_request functions for NumTokens. - Removed HTTP handler registration for NumTokens in both server and client. - Cleaned up related patterns and forwarding functions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
proto/bitsong/nft/v1beta1/nft.proto (1)
4-4: buf build will fail: missing gogoproto dep.Add gogoproto to buf deps (or vendor); otherwise imports won’t resolve.
Example buf.yaml change:
version: v2 +deps: + - buf.build/cosmos/gogo-proto +# If used elsewhere in the repo, also: +# - buf.build/googleapis/googleapis +# - buf.build/cosmos/cosmos-proto +# - buf.build/cosmos/cosmos-sdkThen run: buf mod update && buf build.
proto/bitsong/nft/v1beta1/query.proto (2)
12-44: Missing planned RPC: AllNftInfo (nft + owner).PR objectives mention allNftInfo; not defined here.
service Query { @@ rpc NftInfo(QueryNftInfoRequest) returns (QueryNftInfoResponse) { option (cosmos.query.v1.module_query_safe) = true; option (google.api.http).get = "/bitsong/nft/v1beta1/collections/{collection}/{token_id}"; } + + rpc AllNftInfo(QueryAllNftInfoRequest) returns (QueryAllNftInfoResponse) { + option (cosmos.query.v1.module_query_safe) = true; + option (google.api.http).get = "/bitsong/nft/v1beta1/all_nft_info/{collection}/{token_id}"; + } @@ } + +message QueryAllNftInfoRequest { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; + string collection = 1; + string token_id = 2; +} + +message QueryAllNftInfoResponse { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; + bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false]; + string owner = 2; +}
4-8: buf deps missing for gogoproto (and likely google/cosmos).Add deps to buf.yaml (or vendor) so these imports resolve.
version: v2 +deps: + - buf.build/cosmos/gogo-proto + - buf.build/googleapis/googleapis + - buf.build/cosmos/cosmos-proto + - buf.build/cosmos/cosmos-sdkThen: buf mod update && buf build.
🧹 Nitpick comments (6)
x/nft/keeper/grpc_query.go (4)
149-189: Reuse the helper to avoid drift (AllNftsByOwner → GetNftsByOwner).Both paths duplicate pagination/decoding. Call the helper to centralize logic.
func (k Keeper) AllNftsByOwner(ctx context.Context, req *types.QueryAllNftsByOwnerRequest) (*types.QueryAllNftsByOwnerResponse, error) { @@ - sdkCtx := sdk.UnwrapSDKContext(ctx) - - store := prefix.NewStore( - sdkCtx.KVStore(k.storeKey), - append(types.NFTsByOwnerPrefix, address.MustLengthPrefix(owner)...), - ) - - var nfts []types.Nft - - pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error { - denom, tokenId := types.MustSplitNftLengthPrefixedKey(key) - - nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId))) - if err != nil { - return err - } - - nfts = append(nfts, nft) - return nil - }) + nfts, pageRes, err := k.GetNftsByOwner(ctx, owner, req.Pagination) if err != nil { return nil, status.Error(codes.Internal, err.Error()) }
95-97: Nit: align nil-request error with other handlers.Other RPCs return “empty request” for nil req. Mirror that for consistency.
- if req == nil || req.Collection == "" { - return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") - } + if req == nil { + return nil, status.Error(codes.InvalidArgument, "empty request") + } + if req.Collection == "" { + return nil, status.Error(codes.InvalidArgument, "collection cannot be empty") + }
119-147: Add unit tests for owner-index pagination path.Tests would have caught the key-decoding bug. Recommend cases: empty index key guard, multi-collection ownership, pageKey round-trips.
I can add table-driven tests for GetNftsByOwner covering these edge cases.
122-127: Minor: prefix build is fine, but consider a helper for owner-scope store.To reduce duplication and future mistakes, wrap prefix.NewStore creation in a small helper.
Also applies to: 161-164
proto/bitsong/nft/v1beta1/nft.proto (1)
25-44: Reserve field numbers/names for planned NFT fields.You’ve planned seller_fee and payment_address/creators; reserve now to prevent wire-compat breaks later.
message Nft { option (gogoproto.goproto_getters) = false; @@ uint64 editions = 8; // number of printed editions - - // seller_fee_bps - // payment_address + // Reserve identifiers for upcoming fields to avoid reuse. + reserved 9, 10; + reserved "seller_fee_bps", "payment_address", "creators"; }proto/bitsong/nft/v1beta1/query.proto (1)
53-58: Optional: mark embedded messages non-nullable for consistency.You already do this for repeated fields; consider the same for single embedded messages.
- bitsong.nft.v1beta1.Collection collection = 1; + bitsong.nft.v1beta1.Collection collection = 1 [(gogoproto.nullable) = false]; @@ - bitsong.nft.v1beta1.Nft nft = 1; + bitsong.nft.v1beta1.Nft nft = 1 [(gogoproto.nullable) = false];Also applies to: 83-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
x/nft/types/query.pb.gois excluded by!**/*.pb.gox/nft/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (4)
proto/bitsong/nft/v1beta1/nft.proto(1 hunks)proto/bitsong/nft/v1beta1/query.proto(1 hunks)x/nft/keeper/grpc_query.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/nft/keeper/grpc_query_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
x/nft/keeper/grpc_query.go (3)
x/nft/keeper/keeper.go (1)
Keeper(52-64)x/nft/types/nft.pb.go (6)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)x/nft/types/keys.go (2)
NFTsByOwnerPrefix(31-31)MustSplitNftLengthPrefixedKey(57-64)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/nft/v1beta1/query.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (2)
x/nft/keeper/grpc_query.go (1)
19-40: RPCs look solid (validation, errors, pagination).Input checks, NotFound mapping, and use of CollectionPaginate with prefix all look correct.
Also applies to: 42-66, 68-92, 94-117
proto/bitsong/nft/v1beta1/query.proto (1)
35-38: Good: pagination present on large-list RPCs.Owner and collection listing RPCs expose PageRequest/PageResponse; avoids unbounded responses.
Also applies to: 110-128
| pageRes, err := query.Paginate(store, pagination, func(key []byte, value []byte) error { | ||
| denom, tokenId := types.MustSplitNftLengthPrefixedKey(key) | ||
|
|
||
| nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nfts = append(nfts, nft) |
There was a problem hiding this comment.
Fix owner-index decoding and use sdkCtx in k.NFTs.Get (panic/incorrect results today).
- Owner index key is not a length-prefixed Pair; it’s
[prefix][owner][0x00][primaryKey]. Decoding with MustSplitNftLengthPrefixedKey is wrong. - Use PairKeyCodec on key[1:] (skip delimiter) and pass sdkCtx to k.NFTs.Get.
Apply:
@@
- pageRes, err := query.Paginate(store, pagination, func(key []byte, value []byte) error {
- denom, tokenId := types.MustSplitNftLengthPrefixedKey(key)
-
- nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId)))
- if err != nil {
- return err
- }
-
- nfts = append(nfts, nft)
- return nil
- })
+ pageRes, err := query.Paginate(store, pagination, func(key []byte, _ []byte) error {
+ if len(key) == 0 {
+ return status.Error(codes.Internal, "corrupted owner index key")
+ }
+ pk, err := collections.PairKeyCodec(collections.StringKey, collections.StringKey).Decode(key[1:])
+ if err != nil {
+ return err
+ }
+ nft, err := k.NFTs.Get(sdkCtx, pk)
+ if err != nil {
+ return err
+ }
+ nfts = append(nfts, nft)
+ return nil
+ })
@@
- pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error {
- denom, tokenId := types.MustSplitNftLengthPrefixedKey(key)
-
- nft, err := k.NFTs.Get(ctx, collections.Join(string(denom), string(tokenId)))
- if err != nil {
- return err
- }
-
- nfts = append(nfts, nft)
- return nil
- })
+ pageRes, err := query.Paginate(store, req.Pagination, func(key []byte, _ []byte) error {
+ if len(key) == 0 {
+ return status.Error(ccodes.Internal, "corrupted owner index key")
+ }
+ pk, err := collections.PairKeyCodec(collections.StringKey, collections.StringKey).Decode(key[1:])
+ if err != nil {
+ return err
+ }
+ nft, err := k.NFTs.Get(sdkCtx, pk)
+ if err != nil {
+ return err
+ }
+ nfts = append(nfts, nft)
+ return nil
+ })Note: import/use of status/codes already present. Replace ctx with sdkCtx for Gets.
Also applies to: 168-176
💡 Verification agent
🧩 Analysis chain
Verify repo-wide: no remaining MustSplitNftLengthPrefixedKey on owner index.
Also applies to: 168-176
🏁 Script executed:
#!/bin/bash
# Find incorrect decoders and wrong context usage in owner index pagination paths.
rg -nP -C2 '(MustSplitNftLengthPrefixedKey\(|k\.NFTs\.Get\s*\(\s*ctx\s*,)' --type=go x/nftLength of output: 2684
Replace owner-index key decoding in owner-pagination callback
In QueryNFTsByOwner (grpc_query.go:168–176), replace types.MustSplitNftLengthPrefixedKey(key) and collections.Join(...) with the dedicated owner-index key splitter and NFT key constructor.
🤖 Prompt for AI Agents
In x/nft/keeper/grpc_query.go around lines 129 to 137, the owner-pagination
callback is decoding the owner-index key and building the NFT lookup key using
the generic length-prefixed splitter and collections.Join; replace
types.MustSplitNftLengthPrefixedKey(key) with the dedicated owner-index key
splitter (e.g. types.MustSplitNftOwnerIndexKey or the repo's owner-index
splitter) to extract denom and tokenId correctly, and use the NFT key
constructor (e.g. types.MakeNFTKey or the repo's NFT key builder) instead of
collections.Join(string(denom), string(tokenId)) when calling k.NFTs.Get so the
lookup uses the canonical NFT key format.
- Removed the 'description' field from the Collection and Nft messages in nft.proto. - Updated CreateCollection and MintNFT functions to eliminate the description parameter. - Adjusted validation functions to exclude description checks. - Modified test cases to reflect the removal of description from collection and NFT creation. - Updated the protobuf generated files to align with the new structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proto/bitsong/nft/v1beta1/nft.proto (1)
4-4: buf: missing dependency for gogoproto; build will fail.Add gogoproto to buf deps (or vendor it) so
import "gogoproto/gogo.proto";resolves.Add to buf.yaml:
version: v2 +deps: + - buf.build/cosmos/gogo-proto +# If you use cosmos/google APIs elsewhere, also include: +# - buf.build/googleapis/googleapis +# - buf.build/cosmos/cosmos-proto +# - buf.build/cosmos/cosmos-sdk
🧹 Nitpick comments (8)
x/nft/keeper/grpc_query_test.go (6)
18-26: Broaden assertions: verify denom/creator/initial supply.After querying the collection, also assert denom, creator, and initial num_tokens to catch wiring regressions.
suite.NoError(err) suite.Equal(testCollection1.Name, res.Collection.Name) suite.Equal(testCollection1.Symbol, res.Collection.Symbol) suite.Equal(testCollection1.Uri, res.Collection.Uri) suite.Equal(testCollection1.Minter, res.Collection.Minter) + suite.Equal(collectionDenom, res.Collection.Denom) + suite.Equal(creator1.String(), res.Collection.Creator) + suite.EqualValues(0, res.Collection.NumTokens) }
50-56: Use the minted tokenId variable, not a string literal.Avoid brittle tests if testNft1.TokenId changes.
res, err := suite.keeper.OwnerOf(suite.ctx, &types.QueryOwnerOfRequest{ Collection: collectionDenom, - TokenId: "1", + TokenId: testNft1.TokenId, })
92-92: Rename test to reflect behavior (query by collection, not owner).The test calls Nfts(collection) rather than “by owner”.
-func (suite *KeeperTestSuite) TestQueryNftsOfOwner() { +func (suite *KeeperTestSuite) TestQueryNftsByCollection() {
129-132: Don’t rely on ordering; assert membership.Iteration order may change with key layout. Prefer ElementsMatch.
- suite.Equal(testNft1.TokenId, res.Nfts[0].TokenId) - suite.Equal(testNft2.TokenId, res.Nfts[1].TokenId) + suite.ElementsMatch( + []string{testNft1.TokenId, testNft2.TokenId}, + []string{res.Nfts[0].TokenId, res.Nfts[1].TokenId}, + )
171-176: Avoid order-dependence and check editions per item.Assert set equality for tokenIds and validate editions in a loop.
- suite.Len(res.Nfts, 2) - suite.Equal(testNft1.TokenId, res.Nfts[0].TokenId) - suite.Equal(uint64(0), res.Nfts[0].Editions) - suite.Equal(testNft2.TokenId, res.Nfts[1].TokenId) - suite.Equal(uint64(0), res.Nfts[1].Editions) + suite.Len(res.Nfts, 2) + suite.ElementsMatch( + []string{testNft1.TokenId, testNft2.TokenId}, + []string{res.Nfts[0].TokenId, res.Nfts[1].TokenId}, + ) + for _, n := range res.Nfts { + suite.Equal(uint64(0), n.Editions) + }
125-127: Add pagination coverage for Nfts and AllNftsByOwner.Add tests with PageRequest (e.g., Limit=1, CountTotal=true) to verify pagination wiring and stable ordering across pages.
Example new test (add to this file):
func (suite *KeeperTestSuite) TestQueryPagination() { // ... setup: create collection, mint 2+ NFTs to owner1 ... // Nfts by collection res1, err := suite.keeper.Nfts(suite.ctx, &types.QueryNftsRequest{ Collection: collectionDenom, Pagination: &query.PageRequest{Limit: 1, CountTotal: true}, }) suite.NoError(err) suite.Len(res1.Nfts, 1) suite.NotNil(res1.Pagination) suite.Greater(res1.Pagination.Total, uint64(1)) suite.NotEmpty(res1.Pagination.NextKey) // AllNftsByOwner res2, err := suite.keeper.AllNftsByOwner(suite.ctx, &types.QueryAllNftsByOwnerRequest{ Owner: owner1.String(), Pagination: &query.PageRequest{Limit: 1, CountTotal: true}, }) suite.NoError(err) suite.Len(res2.Nfts, 1) suite.NotNil(res2.Pagination) }Imports needed:
import ( "github.com/cosmos/cosmos-sdk/types/query" "github.com/bitsongofficial/go-bitsong/x/nft/types" )Also applies to: 167-169
proto/bitsong/nft/v1beta1/nft.proto (2)
36-38: Clarify max_editions TODO; avoid tag conflict with editions=8.The comment suggests
max_editions = 8, but field 8 is already used by editions. Update the note to prevent accidental wire incompatibility later.- // TODO: add max_editions - // uint64 max_editions = 8; // max number of printed editions, 0 means no limit + // TODO: add max_editions (use a new tag, e.g., 9) + // uint64 max_editions = 9; // max number of printed editions, 0 means no limit uint64 editions = 8; // number of printed editions
17-21: Optional: annotate bech32 addresses for stronger typing.Annotate address fields with cosmos_proto scalar for validation/tooling. This requires adding cosmos-proto to buf deps.
Example:
import "cosmos_proto/cosmos.proto"; string creator = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; string minter = 7 [(cosmos_proto.scalar) = "cosmos.AddressString"]; string authority = 8 [(cosmos_proto.scalar) = "cosmos.AddressString"]; string owner = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // in Nft string owner = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // in EditionYou’ll need this buf dep:
deps: - buf.build/cosmos/cosmos-protoAlso applies to: 27-35, 47-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
x/nft/types/nft.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
proto/bitsong/nft/v1beta1/nft.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/edition_test.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/keys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- x/nft/types/keys.go
- x/nft/keeper/nft.go
- x/nft/keeper/edition_test.go
- x/nft/keeper/collection.go
- x/nft/keeper/keeper_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
x/nft/keeper/grpc_query_test.go (3)
x/nft/keeper/keeper_test.go (1)
KeeperTestSuite(54-60)x/nft/types/nft.pb.go (6)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-80)Nft(84-84)Nft(85-87)x/nft/types/query.pb.go (15)
QueryCollectionRequest(33-35)QueryCollectionRequest(39-39)QueryCollectionRequest(40-42)QueryOwnerOfRequest(107-110)QueryOwnerOfRequest(114-114)QueryOwnerOfRequest(115-117)QueryNftInfoRequest(182-185)QueryNftInfoRequest(189-189)QueryNftInfoRequest(190-192)QueryNftsRequest(257-260)QueryNftsRequest(264-264)QueryNftsRequest(265-267)QueryAllNftsByOwnerRequest(333-336)QueryAllNftsByOwnerRequest(340-340)QueryAllNftsByOwnerRequest(341-343)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (13)
proto/bitsong/nft/v1beta1/nft.proto (1)
4-4: buf build will fail: missing gogoproto dependencyThe import "gogoproto/gogo.proto" isn’t available per static analysis. Add the buf dep or vendor it.
Apply (outside this file) to buf.yaml:
version: v2 +deps: + - buf.build/cosmos/gogo-proto +# If used elsewhere, also consider: +# - buf.build/googleapis/googleapis +# - buf.build/cosmos/cosmos-proto +# - buf.build/cosmos/cosmos-sdkOr vendor third_party/proto/gogoproto/gogo.proto.
x/nft/types/keys.go (1)
38-58: Fix misleading “length-prefixed” name and incomplete validationThe key is zero-delimited, not length-prefixed; current checks don’t validate empty parts or tokenId length. Tighten parsing to prevent malformed keys entering indexes.
func SplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte, err error) { - parts := bytes.SplitN(key, []byte{0}, 2) - if len(parts) != 2 { - return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts, got %d", len(parts)) - } - - denomLen := len(parts[0]) - - if denomLen > MaxDenomLength { - return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "decoded denom key length %d exceeds max allowed length %d", denomLen, MaxDenomLength) - } - - if len(key)-1 < denomLen { - return nil, nil, fmt.Errorf("key is malformed: length prefix %d is greater than tokenId bytes %d", denomLen, len(key)-1) - } - - denom = parts[0] - tokenId = parts[1] - - return denom, tokenId, nil + parts := bytes.SplitN(key, []byte{0}, 2) + if len(parts) != 2 { + return nil, nil, fmt.Errorf("invalid composite key format: expected 2 parts separated by 0x00") + } + denom, tokenId = parts[0], parts[1] + if len(denom) == 0 { + return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "denom is empty") + } + if len(denom) > MaxDenomLength { + return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "denom length %d exceeds max allowed %d", len(denom), MaxDenomLength) + } + if len(tokenId) == 0 { + return nil, nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "token_id is empty") + } + if len(tokenId) > MaxTokenIdLength { + return nil, nil, errors.Wrapf(sdkerrors.ErrInvalidType, "token_id length %d exceeds max allowed %d", len(tokenId), MaxTokenIdLength) + } + return denom, tokenId, nil }Optionally rename to SplitNftZeroDelimitedKey for accuracy, updating call sites.
x/nft/keeper/nft.go (8)
3-13: Add Cosmos SDK typed errors; keep fmt for Sprintf only.
Enables proper ABCI codes and client UX.import ( "context" "fmt" "strings" "cosmossdk.io/collections" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/math" "github.com/bitsongofficial/go-bitsong/x/nft/types" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" )
28-35: Wrap store check errors; typed error for duplicates.has, err := k.NFTs.Has(ctx, nftKey) if err != nil { - return fmt.Errorf("failed to check NFT: %w", err) + return errorsmod.Wrap(err, "failed to check NFT") } if has { - return fmt.Errorf("NFT with token ID %s already exists in collection %s", tokenId, collectionDenom) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "nft %s already exists in collection %s", tokenId, collectionDenom) }
42-53: Enforce “minter nullable disables minting” with typed errors.
Also wrap invalid bech32.- if coll.Minter == "" { - return fmt.Errorf("minting disabled for this collection") - } + if coll.Minter == "" { + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") + } collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) if err != nil { - return fmt.Errorf("invalid minter address: %w", err) + return errorsmod.Wrap(err, "invalid minter address") } if !minter.Equals(collectionMinter) { - return fmt.Errorf("only the collection minter can mint NFTs") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs") }
118-134: GetNft: typed not-found and wrapping.- has, err := k.NFTs.Has(ctx, nftKey) - if err != nil { - return nil, fmt.Errorf("failed to check NFT: %w", err) - } - if !has { - return nil, fmt.Errorf("NFT with token ID %s does not exist in collection %s", tokenId, collectionDenom) - } + has, err := k.NFTs.Has(ctx, nftKey) + if err != nil { + return nil, errorsmod.Wrap(err, "failed to check NFT") + } + if !has { + return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "nft %s not found in collection %s", tokenId, collectionDenom) + } nft, err := k.NFTs.Get(ctx, nftKey) if err != nil { - return nil, fmt.Errorf("failed to get NFT: %w", err) + return nil, errorsmod.Wrap(err, "failed to get NFT") }
136-155: Metadata validation: switch to typed errors.- if strings.TrimSpace(tokenId) == "" { - return fmt.Errorf("token ID cannot be empty") - } + if strings.TrimSpace(tokenId) == "" { + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty") + } @@ - if len(tokenId) > types.MaxTokenIdLength { - return fmt.Errorf("token ID length exceeds maximum of %d", types.MaxTokenIdLength) - } + if len(tokenId) > types.MaxTokenIdLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token_id length exceeds maximum of %d", types.MaxTokenIdLength) + } @@ - if len(name) > types.MaxNameLength { - return fmt.Errorf("name length exceeds maximum of %d", types.MaxNameLength) - - } + if len(name) > types.MaxNameLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "name length exceeds maximum of %d", types.MaxNameLength) + } @@ - if len(uri) > types.MaxURILength { - return fmt.Errorf("URI length exceeds maximum of %d", types.MaxURILength) - } + if len(uri) > types.MaxURILength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "uri length exceeds maximum of %d", types.MaxURILength) + }
167-185: Transfer: typed errors and wrapping.
Preserve message substrings used in tests.- if oldOwner.Equals(newOwner) { - return fmt.Errorf("cannot transfer NFT to the same owner") - } + if oldOwner.Equals(newOwner) { + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "cannot transfer NFT to the same owner") + } @@ - nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) - if err != nil { - return fmt.Errorf("collection or token_id does not exist") - } + nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) + if err != nil { + return sdkerrors.Wrap(sdkerrors.ErrNotFound, "collection or token_id does not exist") + } @@ - if nft.Owner != oldOwner.String() { - return fmt.Errorf("only the owner can transfer the NFT") - } + if nft.Owner != oldOwner.String() { + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the owner can transfer the NFT") + } @@ - err = k.setNft(ctx, nft) - if err != nil { - return fmt.Errorf("failed to set NFT Owner: %w", err) - } + err = k.setNft(ctx, nft) + if err != nil { + return errorsmod.Wrap(err, "failed to set NFT owner") + }
196-204: Edition increment: wrap error.- nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) - if err != nil { - return fmt.Errorf("failed to get NFT: %w", err) - } + nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) + if err != nil { + return errorsmod.Wrap(err, "failed to get NFT") + }
71-73: Wrap persistence error.- if err := k.setNft(ctx, nft); err != nil { - return fmt.Errorf("failed to set NFT: %w", err) - } + if err := k.setNft(ctx, nft); err != nil { + return errorsmod.Wrap(err, "failed to set NFT") + }x/nft/keeper/keeper.go (2)
21-44: Decouple owner index from Bech32; use AddressCodec.
Future-proofs HRP changes and aligns with injected codec.-func newNFTIndexes(sb *collections.SchemaBuilder) NFTIndexes { +func newNFTIndexes(sb *collections.SchemaBuilder, ac address.Codec) NFTIndexes { @@ - Owner: indexes.NewMulti( + Owner: indexes.NewMulti( sb, types.NFTsByOwnerPrefix, "nfts_by_owner", sdk.AccAddressKey, collections.PairKeyCodec(collections.StringKey, collections.StringKey), - func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { - return sdk.AccAddressFromBech32(v.Owner) - }, + func(pk collections.Pair[string, string], v types.Nft) (sdk.AccAddress, error) { + bz, err := ac.StringToBytes(v.Owner) + return sdk.AccAddress(bz), err + }, ),
100-107: Wire AddressCodec into indexes.codec.CollValue[types.Nft](cdc), - newNFTIndexes(sb), + newNFTIndexes(sb, ak.AddressCodec()),x/nft/keeper/collection.go (1)
68-79: Return typed SDK errors from GetMinter and wrap address parsing errorUse sdkerrors for ABCI codes and errorsmod.Wrap to retain cause. This was flagged earlier; still applies.
func (k Keeper) GetMinter(ctx context.Context, denom string) (sdk.AccAddress, error) { coll, err := k.Collections.Get(ctx, denom) if err != nil { return nil, types.ErrCollectionNotFound } if coll.Minter == "" { - return nil, fmt.Errorf("minting disabled for this collection") + return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") } - return sdk.AccAddressFromBech32(coll.Minter) + addr, err := sdk.AccAddressFromBech32(coll.Minter) + if err != nil { + return nil, errorsmod.Wrap(err, "invalid minter address") + } + return addr, nil }Also add the imports:
import ( "context" "fmt" "strings" "cosmossdk.io/math" + errorsmod "cosmossdk.io/errors" "github.com/bitsongofficial/go-bitsong/x/nft/types" tmcrypto "github.com/cometbft/cometbft/crypto" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" )
🧹 Nitpick comments (13)
third_party/proto/amino/amino.proto (1)
11-31: Add upstream license header for third_party provenanceSince this file mirrors cosmos-sdk extensions, include the original license header to preserve attribution and reduce compliance risk.
Also applies to: 33-84
proto/bitsong/nft/v1beta1/nft.proto (2)
8-22: Annotate address fields with cosmos_proto.scalar and reserve skipped field numbersAdd scalar hints for better codegen and mark intentionally skipped tags to avoid future reuse.
syntax = "proto3"; package bitsong.nft.v1beta1; -import "gogoproto/gogo.proto"; +import "gogoproto/gogo.proto"; +import "cosmos_proto/cosmos.proto"; option go_package = "github.com/bitsongofficial/go-bitsong/x/nft/types"; message Collection { option (gogoproto.goproto_getters) = false; string denom = 1; string symbol = 2; string name = 3; string uri = 5; + reserved 4; // keep gap intentional - string creator = 6; - string minter = 7; // who can mint new nfts, if not set no one can mint - string authority = 8; // who can update name, description and uri, if not set no one can update + string creator = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + string minter = 7 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // who can mint new nfts, if not set no one can mint + string authority = 8 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // who can update name, description and uri, if not set no one can update uint64 num_tokens = 9; }
24-42: Reserve the unused field number and annotate owner as address scalarAlso adds scalar hints for owner.
message Nft { option (gogoproto.goproto_getters) = false; string collection = 1; string token_id = 2; string name = 3; string uri = 5; + reserved 4; - string owner = 6; + string owner = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // string authority = 7; // who can update name, description and uri, if not set no one can update // TODO: add max_editions // uint64 max_editions = 8; // max number of printed editions, 0 means no limit uint64 editions = 7; // number of printed editions // seller_fee_bps // payment_address }third_party/proto/cosmos_proto/cosmos.proto (1)
108-112: Satisfy buf lint: add enum and value comments for ScalarTypeAdds minimal doc comments to silence COMMENT_ENUM/COMMENT_ENUM_VALUE.
-enum ScalarType { - SCALAR_TYPE_UNSPECIFIED = 0; - SCALAR_TYPE_STRING = 1; - SCALAR_TYPE_BYTES = 2; -} +enum ScalarType { + // SCALAR_TYPE_UNSPECIFIED indicates an unknown or unspecified scalar type. + SCALAR_TYPE_UNSPECIFIED = 0; + // SCALAR_TYPE_STRING indicates the scalar is encoded in a string field. + SCALAR_TYPE_STRING = 1; + // SCALAR_TYPE_BYTES indicates the scalar is encoded in a bytes field. + SCALAR_TYPE_BYTES = 2; +}third_party/proto/cosmos/msg/v1/msg.proto (1)
1-10: Add upstream license header for third_party provenanceSame rationale as other vendored protos.
x/nft/keeper/msg_server_test.go (2)
210-352: Subtest state coupling.
“valid message” mutates state and affects later cases (e.g., “sender is not the owner”). Either reset state per case or re-mint before each subtest.
487-528: Nit: rename to reflect PrintEdition.
Improves test discoverability.-func (suite *KeeperTestSuite) TestMsgPrintNFT_MaxLimit() { +func (suite *KeeperTestSuite) TestMsgPrintEdition_MaxLimit() {x/nft/keeper/nft.go (1)
157-160: Remove unused helpercreateNftDenom
Delete this function fromx/nft/keeper/nft.go—it’s never referenced in the codebase.x/nft/keeper/keeper.go (1)
86-93: Nit: remove stale TODO (“fix the store once we add queries”).
Queries are already present; the comment is misleading.x/nft/keeper/collection.go (3)
137-147: Prefer typed errors for metadata validationUnify with Cosmos error model; return ErrInvalidRequest.
func (k Keeper) validateCollectionMetadata(name, uri string) error { if len(name) > types.MaxNameLength { - return fmt.Errorf("name cannot be longer than %d characters", types.MaxNameLength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "name cannot be longer than %d characters", types.MaxNameLength) } if len(uri) > types.MaxURILength { - return fmt.Errorf("uri cannot be longer than %d characters", types.MaxURILength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "uri cannot be longer than %d characters", types.MaxURILength) } return nil }(Imports from the GetMinter comment cover sdkerrors.)
27-31: TODO: fee handlingIf fees are part of the UX, consider wiring a param/constant and emit an event. I can draft this with a keeper param and ante fee check.
14-21: Future alignment: authority supportProto exposes authority on collection create, but Keeper.CreateCollection doesn’t accept it. Confirm MsgServer behavior; either plumb authority through or remove from proto until implemented.
proto/bitsong/nft/v1beta1/tx.proto (1)
20-34: Authority field present in proto; ensure Keeper path supports or ignore explicitlyIf authority is intentionally non-functional for this PR, document it in MsgServer validation to avoid misleading clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
x/nft/types/nft.pb.gois excluded by!**/*.pb.gox/nft/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
proto/bitsong/nft/v1beta1/nft.proto(1 hunks)proto/bitsong/nft/v1beta1/tx.proto(1 hunks)third_party/proto/amino/amino.proto(1 hunks)third_party/proto/cosmos/msg/v1/msg.proto(1 hunks)third_party/proto/cosmos_proto/cosmos.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/edition.go(1 hunks)x/nft/keeper/keeper.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/msg_server.go(1 hunks)x/nft/keeper/msg_server_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)x/nft/types/keys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/nft/keeper/edition.go
🧰 Additional context used
🧬 Code graph analysis (6)
x/nft/keeper/msg_server.go (2)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/tx.pb.go (25)
MsgServer(511-516)MsgCreateCollection(34-41)MsgCreateCollection(45-45)MsgCreateCollection(46-48)MsgCreateCollectionResponse(76-78)MsgCreateCollectionResponse(82-82)MsgCreateCollectionResponse(83-85)MsgMintNFT(120-127)MsgMintNFT(131-131)MsgMintNFT(132-134)MsgMintNFTResponse(162-165)MsgMintNFTResponse(169-169)MsgMintNFTResponse(170-172)MsgSendNFT(214-219)MsgSendNFT(223-223)MsgSendNFT(224-226)MsgSendNFTResponse(254-255)MsgSendNFTResponse(259-259)MsgSendNFTResponse(260-262)MsgPrintEdition(290-295)MsgPrintEdition(299-299)MsgPrintEdition(300-302)MsgPrintEditionResponse(330-334)MsgPrintEditionResponse(338-338)MsgPrintEditionResponse(339-341)
x/nft/keeper/msg_server_test.go (4)
x/nft/keeper/keeper_test.go (1)
KeeperTestSuite(54-63)x/nft/types/tx.pb.go (12)
MsgCreateCollection(34-41)MsgCreateCollection(45-45)MsgCreateCollection(46-48)MsgMintNFT(120-127)MsgMintNFT(131-131)MsgMintNFT(132-134)MsgSendNFT(214-219)MsgSendNFT(223-223)MsgSendNFT(224-226)MsgPrintEdition(290-295)MsgPrintEdition(299-299)MsgPrintEdition(300-302)x/nft/types/nft.pb.go (3)
Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/keys.go (2)
MaxNftsInCollection(25-25)MaxEditions(26-26)
x/nft/keeper/keeper_test.go (5)
x/nft/types/nft.pb.go (6)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/keeper/msg_server.go (1)
NewMsgServerImpl(18-20)x/nft/types/query.pb.go (3)
QueryAllNftsByOwnerRequest(333-336)QueryAllNftsByOwnerRequest(340-340)QueryAllNftsByOwnerRequest(341-343)
x/nft/keeper/collection.go (4)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/nft.pb.go (3)
Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/errors.go (2)
ErrCollectionNotFound(7-7)ErrCollectionAlreadyExists(6-6)x/nft/types/keys.go (3)
MaxSymbolLength(20-20)MaxNameLength(21-21)MaxURILength(22-22)
x/nft/keeper/nft.go (5)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)x/nft/types/keys.go (4)
MaxNftsInCollection(25-25)MaxTokenIdLength(19-19)MaxNameLength(21-21)MaxURILength(22-22)x/nft/types/nft.pb.go (6)
Nft(70-79)Nft(83-83)Nft(84-86)Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/events.go (6)
EventTypeTransferNft(6-6)AttributeKeyReceiver(10-10)AttributeKeySender(9-9)AttributeKeyCollection(12-12)AttributeKeyTokenId(13-13)NewNftReceivedEvent(16-23)
x/nft/keeper/keeper.go (3)
x/nft/types/nft.pb.go (9)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)Edition(114-119)Edition(123-123)Edition(124-126)x/nft/types/keys.go (8)
NFTsByCollectionPrefix(33-33)NFTsByOwnerPrefix(34-34)StoreKey(14-14)ModuleName(13-13)CollectionsPrefix(30-30)SupplyPrefix(31-31)NFTsPrefix(32-32)EditionsPrefix(35-35)x/nft/types/expected_keeper.go (1)
AccountKeeper(10-17)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/tx.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
third_party/proto/cosmos_proto/cosmos.proto
108-112: Enum "ScalarType" should have a non-empty comment for documentation.
(COMMENT_ENUM)
109-109: Enum value "SCALAR_TYPE_UNSPECIFIED" should have a non-empty comment for documentation.
(COMMENT_ENUM_VALUE)
110-110: Enum value "SCALAR_TYPE_STRING" should have a non-empty comment for documentation.
(COMMENT_ENUM_VALUE)
111-111: Enum value "SCALAR_TYPE_BYTES" should have a non-empty comment for documentation.
(COMMENT_ENUM_VALUE)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (8)
proto/bitsong/nft/v1beta1/nft.proto (1)
8-22: PR description vs schema mismatch: authority marked “pending” but present hereCollection.authority is already defined. Confirm this is intentional and update the PR checklist/status to avoid confusion for reviewers/users.
third_party/proto/cosmos/msg/v1/msg.proto (1)
11-30: Signer annotations confirmed in proto/bitsong/nft/v1beta1/tx.proto
AllMsgCreateCollection,MsgMintNFT,MsgSendNFT, andMsgPrintEditiontypes include the required(cosmos.msg.v1.signer)options.x/nft/keeper/msg_server.go (1)
51-73: MsgServer handlers look solid.
Consistent address decoding, context unwrap, and keeper delegation; responses match protobufs.Also applies to: 75-95, 96-119
x/nft/keeper/keeper_test.go (3)
80-101: CreateCollection test LGTM.
Covers happy path and duplicate-collection error.
103-145: Mint flow coverage LGTM.
Validates initial supply and increments.
147-201: Transfer flow coverage LGTM.
Ownership index and owner change assertions are appropriate.x/nft/keeper/collection.go (2)
92-105: Denom derivation LGTM (delimiter + lowercase hex)Delimiter removes collision ambiguity; lowercase hex satisfies sdk.ValidateDenom.
113-121: Denom validation and uniqueness LGTMUsing sdk.ValidateDenom and module error on conflict is correct.
| @@ -0,0 +1,95 @@ | |||
| syntax = "proto3";; | |||
There was a problem hiding this comment.
Fix proto syntax typo
Double semicolon breaks compilation.
-syntax = "proto3";;
+syntax = "proto3";📝 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.
| syntax = "proto3";; | |
| syntax = "proto3"; |
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/tx.proto around line 1, the proto syntax
declaration has a double semicolon; replace the line `syntax = "proto3";;` with
a single semicolon `syntax = "proto3";` and then re-run your protobuf
generation/compilation step to verify the fix.
| import "gogoproto/gogo.proto"; | ||
| import "cosmos/msg/v1/msg.proto"; | ||
| import "amino/amino.proto"; | ||
| import "cosmos_proto/cosmos.proto"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Buf import resolution: add module deps (preferred) or vendor third_party protos
Buf error indicates missing deps. Add these to buf.yaml to resolve imports.
Example buf.yaml snippet:
version: v1
deps:
- buf.build/cosmos/gogo-proto
- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/amino
- buf.build/cosmos/cosmos-sdk
Alternatively, vendor files under third_party/proto and adjust buf.yaml roots.
🧰 Tools
🪛 Buf (1.55.1)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/tx.proto around lines 4 to 7, the file imports
third-party protos (gogoproto, cosmos msg/amino/cosmos_proto) but Buf cannot
resolve them; update buf.yaml to add the recommended module dependencies
(buf.build entries for gogo-proto, cosmos-proto, amino, cosmos-sdk) or vendor
those third-party protos under third_party/proto and adjust buf.yaml roots
accordingly so the imports resolve during buf build.
| option (amino.name) = "cosmos-sdk/nft/MsgCreateCollection"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use module-specific amino names to avoid collisions
Prefix amino names with “bitsong/nft/…” rather than “cosmos-sdk/nft/…”.
- option (amino.name) = "cosmos-sdk/nft/MsgCreateCollection";
+ option (amino.name) = "bitsong/nft/MsgCreateCollection";
@@
- option (amino.name) = "cosmos-sdk/nft/MsgMintNFT";
+ option (amino.name) = "bitsong/nft/MsgMintNFT";
@@
- option (amino.name) = "cosmos-sdk/nft/MsgSendNFT";
+ option (amino.name) = "bitsong/nft/MsgSendNFT";
@@
- option (amino.name) = "cosmos-sdk/nft/MsgPrintEdition";
+ option (amino.name) = "bitsong/nft/MsgPrintEdition";Also applies to: 42-43, 63-64, 79-80
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/tx.proto around lines 22-23 (and similarly at
42-43, 63-64, 79-80), the amino.name options use the generic prefix
"cosmos-sdk/nft/…", which can cause name collisions; update each amino.name to
use the module-specific prefix "bitsong/nft/…" (e.g., replace
"cosmos-sdk/nft/MsgCreateCollection" with "bitsong/nft/MsgCreateCollection") for
all occurrences mentioned so the amino names are unique to the Bitsong NFT
module.
| // TODO(fdymylja): once we fully migrate to protov2 the go_package needs to be updated. | ||
| // We need this right now because gogoproto codegen needs to import the extension. | ||
| option go_package = "github.com/cosmos/cosmos-sdk/types/tx/amino"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify vendoring vs upstream to avoid extension collisions and import confusion
The custom go_package and vendored extension look fine, but please confirm we’re not shadowing the same extensions pulled in via upstream deps (cosmos-sdk). Duplicate option numbers or packages will break buf/protoc.
Run:
🏁 Script executed:
#!/bin/bash
# Find duplicate amino extensions or conflicting go_package files
rg -nP 'extend\s+google\.protobuf\.(MessageOptions|FieldOptions)\b' third_party proto -C1
rg -n '1111000[1-6]' third_party proto -C1
# Check if upstream amino proto also exists
fd -a --strip-cwd-prefix -t f 'amino.proto' third_party protoLength of output: 2854
🏁 Script executed:
#!/bin/bash
# Show upstream cosmos_proto extension definitions with their tag numbers
rg -nP 'extend\s+google\.protobuf\.(MessageOptions|FieldOptions)' third_party/proto/cosmos_proto/cosmos.proto -C5Length of output: 1054
Avoid extension tag collisions with upstream: the Amino extension name = 11110001 in third_party/proto/amino/amino.proto conflicts with module_query_safe = 11110001 in third_party/proto/cosmos/query/v1/query.proto. Rename or reassign Amino’s extension tags to an unused range.
| import "google/protobuf/descriptor.proto"; | ||
|
|
||
| option go_package = "github.com/regen-network/cosmos-proto"; | ||
| option go_package = "github.com/cosmos/cosmos-proto;cosmos_proto"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm vendoring strategy to avoid duplicate cosmos-proto
The local go_package matches upstream. Ensure we don’t also pull cosmos-proto via buf deps, which would create duplicate symbols.
Run:
🏁 Script executed:
#!/bin/bash
# Check for multiple cosmos_proto definitions
fd -a --strip-cwd-prefix -t f 'cosmos.proto' third_party proto
rg -nP 'package\s+cosmos_proto;' third_party proto -C2Length of output: 580
🏁 Script executed:
#!/bin/bash
# List all cosmos.proto files in the repository
find . -type f -name 'cosmos.proto'
# Search for any package cosmos_proto definitions
rg -nP 'package\s+cosmos_proto;' -C2 .
# Locate Buf config files
find . -type f \( -name 'buf.yaml' -o -name 'buf.work.yaml' \)
# If Buf configs exist, check for upstream cosmos-proto dependencies
for cfg in buf.yaml buf.work.yaml; do
if [ -f "$cfg" ]; then
echo "Checking $cfg for cosmos-proto references"
rg -n 'cosmos_proto' -n "$cfg"
fi
doneLength of output: 765
🏁 Script executed:
#!/bin/bash
# Show beginning of Buf configuration files to inspect deps
echo "==== buf.work.yaml ===="
sed -n '1,200p' buf.work.yaml
echo "==== proto/buf.yaml ===="
sed -n '1,200p' proto/buf.yaml
# Search for any references to upstream cosmos-proto in Buf configs
rg -n 'cosmos-proto' -n buf.work.yaml proto/buf.yamlLength of output: 851
Drop upstream cosmos-proto Buf dependency
Remove the buf.build/cosmos/cosmos-proto entry from proto/buf.yaml to avoid importing cosmos_proto both via your vendored third_party/proto/cosmos_proto and the upstream dependency, which would create duplicate symbols.
🤖 Prompt for AI Agents
In third_party/proto/cosmos_proto/cosmos.proto around line 6, the project
vendorizes cosmos_proto but the repository still references the upstream
buf.build/cosmos/cosmos-proto in proto/buf.yaml causing duplicate symbols; open
proto/buf.yaml and remove (or comment out) the buf.build/cosmos/cosmos-proto
entry so the build uses the vendored third_party/proto/cosmos_proto only, then
re-run buf build/commit changes to ensure no duplicate imports remain.
| for i := 0; i < types.MaxNftsInCollection; i++ { | ||
| mintMsg := &types.MsgMintNFT{ | ||
| Minter: minter1.String(), | ||
| Recipient: owner1.String(), | ||
| Collection: res.Denom, | ||
| TokenId: "token" + string(rune(i)), | ||
| Name: "Token " + string(rune(i)), | ||
| Uri: "http://example.com/token" + string(rune(i)), | ||
| } |
There was a problem hiding this comment.
Bug: token/name/uri use string(rune(i)); use strconv.Itoa.
Current code generates non-printable, often 1-char token IDs.
+import "strconv"
@@
- TokenId: "token" + string(rune(i)),
- Name: "Token " + string(rune(i)),
- Uri: "http://example.com/token" + string(rune(i)),
+ TokenId: "token" + strconv.Itoa(i),
+ Name: "Token " + strconv.Itoa(i),
+ Uri: "http://example.com/token" + strconv.Itoa(i),🤖 Prompt for AI Agents
In x/nft/keeper/msg_server_test.go around lines 183 to 191, the test builds
token/name/uri using string(rune(i)) which yields non-printable or single-char
runes; replace each string(rune(i)) with strconv.Itoa(i) to produce proper
decimal strings (e.g., "token"+strconv.Itoa(i)), and add the strconv import (or
group into existing imports) at the top of the file.
| import ( | ||
| "context" | ||
|
|
||
| errorsmod "cosmossdk.io/errors" | ||
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import strings to allow empty-minter handling.
Required for trimming/empty checks in CreateCollection.
import (
"context"
+ "strings"
errorsmod "cosmossdk.io/errors"
"github.com/bitsongofficial/go-bitsong/x/nft/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)📝 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.
| import ( | |
| "context" | |
| errorsmod "cosmossdk.io/errors" | |
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |
| ) | |
| import ( | |
| "context" | |
| "strings" | |
| errorsmod "cosmossdk.io/errors" | |
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |
| ) |
🤖 Prompt for AI Agents
In x/nft/keeper/msg_server.go around lines 3 to 10, the file needs to import the
standard "strings" package so CreateCollection can trim and check for empty
minter values; add the import "strings" to the import block and update
CreateCollection to use strings.TrimSpace when validating the minter (or to
allow an empty minter by trimming and checking for zero-length) so empty/minter
handling works correctly.
x/nft/keeper/msg_server.go
Outdated
| minter, err := k.ac.StringToBytes(msg.Minter) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid minter address: %s", err) | ||
| } |
There was a problem hiding this comment.
Honor “minter may be empty” per PR contract.
Empty minter should disable minting; current code rejects empty string as invalid bech32.
- minter, err := k.ac.StringToBytes(msg.Minter)
- if err != nil {
- return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid minter address: %s", err)
- }
+ var minter sdk.AccAddress
+ var err error
+ if strings.TrimSpace(msg.Minter) != "" {
+ minter, err = k.ac.StringToBytes(msg.Minter)
+ if err != nil {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid minter address: %s", err)
+ }
+ }Also update the CreateCollection tests to accept empty minter (see comment in msg_server_test.go). Do you want me to push test changes as well?
🤖 Prompt for AI Agents
In x/nft/keeper/msg_server.go around lines 30-33, the current code rejects an
empty msg.Minter as an invalid bech32 address; change the logic to treat an
empty minter string as "minting disabled" by not attempting to decode it and
leaving the minter bytes nil/empty (i.e., only call k.ac.StringToBytes when
msg.Minter is non-empty and return the existing error only for non-empty invalid
addresses), and update the CreateCollection tests to accept and assert that an
empty minter is allowed (adjust expectations accordingly).
| supply := k.GetSupply(ctx, collectionDenom) | ||
| if supply.Equal(math.NewInt(types.MaxNftsInCollection)) { | ||
| return fmt.Errorf("max supply reached for collection %s", collectionDenom) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use GTE for supply cap and typed error.
Avoids accidental overflow acceptance.
- if supply.Equal(math.NewInt(types.MaxNftsInCollection)) {
- return fmt.Errorf("max supply reached for collection %s", collectionDenom)
- }
+ if supply.GTE(math.NewInt(types.MaxNftsInCollection)) {
+ return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "max supply reached for collection %s", collectionDenom)
+ }📝 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.
| supply := k.GetSupply(ctx, collectionDenom) | |
| if supply.Equal(math.NewInt(types.MaxNftsInCollection)) { | |
| return fmt.Errorf("max supply reached for collection %s", collectionDenom) | |
| } | |
| supply := k.GetSupply(ctx, collectionDenom) | |
| if supply.GTE(math.NewInt(types.MaxNftsInCollection)) { | |
| return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "max supply reached for collection %s", collectionDenom) | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 55 to 58, the code currently checks equality
against the max supply and returns a generic error; change the check to use
greater-than-or-equal (supply.GTE(math.NewInt(types.MaxNftsInCollection))) to
prevent accepting values that overflow past the cap, and replace the fmt.Errorf
with the canonical typed error from the types package (e.g., return
types.ErrMaxSupplyReached.Wrapf or types.ErrMaxSupplyReached.WithFormat/Wrap
depending on project convention) including the collectionDenom in the message so
callers can handle the error programmatically.
| func MustSplitNftLengthPrefixedKey(key []byte) (denom, tokenId []byte) { | ||
| denom, tokenId, err := SplitNftLengthPrefixedKey(key) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return denom, tokenId | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add tests for edge cases (empty parts, overlong fields)
Cover empty denom/tokenId and overlong tokenId to ensure the parser rejects malformed keys.
Run to find existing tests and add cases if missing:
🏁 Script executed:
#!/bin/bash
fd -a --strip-cwd-prefix -t f 'keys_test.go'
rg -nP 'SplitNftLengthPrefixedKey|MustSplitNftLengthPrefixedKey' -C3Length of output: 2718
Add edge-case tests for SplitNftLengthPrefixedKey and MustSplitNftLengthPrefixedKey
x/nft/types/keys_test.go: cover malformed keys by adding cases that verify
- empty denom (e.g. key = []byte{0, 'a'})
- empty tokenId (e.g. key = []byte{'a', 0})
- missing separator (e.g. key = []byte("foo"))
- excessively long fields (e.g. a tokenId far exceeding expected length)
Assert thatSplitNftLengthPrefixedKeyreturns an error and thatMustSplitNftLengthPrefixedKeypanics for each.
🤖 Prompt for AI Agents
In x/nft/types/keys.go around lines 60 to 67, add unit tests in
x/nft/types/keys_test.go that exercise edge cases for SplitNftLengthPrefixedKey
and MustSplitNftLengthPrefixedKey: create table-driven subtests for (1) empty
denom using key = []byte{0, 'a'}, (2) empty tokenId using key = []byte{'a', 0},
(3) missing separator using key = []byte("foo"), and (4) excessively long field
using a key with a very large length prefix followed by many bytes; for each
case assert that SplitNftLengthPrefixedKey returns a non-nil error and that
MustSplitNftLengthPrefixedKey panics (use t.Run with require.Error or
assert.Panics/require.Panics or the testing package’s recover pattern), keeping
tests concise and isolated.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (11)
proto/bitsong/nft/v1beta1/nft.proto (1)
4-4: buf build will fail: missing gogoproto dependency.Add gogoproto to buf deps (or vendor it) so
import "gogoproto/gogo.proto";resolves.Suggested buf.yaml snippet:
version: v2 +deps: + - buf.build/cosmos/gogo-proto +# If used elsewhere, also: +# - buf.build/googleapis/googleapis +# - buf.build/cosmos/cosmos-proto +# - buf.build/cosmos/cosmos-sdkx/nft/keeper/nft.go (9)
3-13: Adopt typed SDK errors and errorsmod; add imports.Switch away from plain fmt errors for proper ABCI codes and consistent client UX.
import ( "context" "fmt" "strings" "cosmossdk.io/collections" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/math" "github.com/bitsongofficial/go-bitsong/x/nft/types" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" )
28-35: Use typed errors for existence checks.Wrap internal errors and return InvalidRequest on duplicate.
has, err := k.NFTs.Has(ctx, nftKey) if err != nil { - return fmt.Errorf("failed to check NFT: %w", err) + return errorsmod.Wrap(err, "failed to check NFT") } if has { - return fmt.Errorf("NFT with token ID %s already exists in collection %s", tokenId, collectionDenom) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "nft %s already exists in collection %s", tokenId, collectionDenom) }
37-54: Enforce “minter may be empty disables minting” with typed errors; wrap bech32 parse.Aligns with PR design and improves codes.
coll, err := k.Collections.Get(ctx, collectionDenom) if err != nil { return types.ErrCollectionNotFound } if coll.Minter == "" { - return fmt.Errorf("minting disabled for this collection") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") } collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) if err != nil { - return fmt.Errorf("invalid minter address: %w", err) + return errorsmod.Wrap(err, "invalid minter address") } if !minter.Equals(collectionMinter) { - return fmt.Errorf("only the collection minter can mint NFTs") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection minter can mint NFTs") }
55-58: Use GTE for supply cap and return typed error.Prevents overflow past cap and improves UX.
- if supply.Equal(math.NewInt(types.MaxNftsInCollection)) { - return fmt.Errorf("max supply reached for collection %s", collectionDenom) - } + if supply.GTE(math.NewInt(types.MaxNftsInCollection)) { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "max supply reached for collection %s", collectionDenom) + }
186-197: Return typed errors in GetNft.has, err := k.NFTs.Has(ctx, nftKey) if err != nil { - return nil, fmt.Errorf("failed to check NFT: %w", err) + return nil, errorsmod.Wrap(err, "failed to check NFT") } if !has { - return nil, fmt.Errorf("NFT with token ID %s does not exist in collection %s", tokenId, collectionDenom) + return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "nft %s not found in collection %s", tokenId, collectionDenom) } nft, err := k.NFTs.Get(ctx, nftKey) if err != nil { - return nil, fmt.Errorf("failed to get NFT: %w", err) + return nil, errorsmod.Wrap(err, "failed to get NFT") }
202-221: Use typed validation errors; align messages.if strings.TrimSpace(tokenId) == "" { - return fmt.Errorf("token ID cannot be empty") + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "token_id cannot be empty") } if len(tokenId) > types.MaxTokenIdLength { - return fmt.Errorf("token ID length exceeds maximum of %d", types.MaxTokenIdLength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "token_id length exceeds maximum of %d", types.MaxTokenIdLength) } if len(name) > types.MaxNameLength { - return fmt.Errorf("name length exceeds maximum of %d", types.MaxNameLength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "name length exceeds maximum of %d", types.MaxNameLength) } if len(uri) > types.MaxURILength { - return fmt.Errorf("URI length exceeds maximum of %d", types.MaxURILength) + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "uri length exceeds maximum of %d", types.MaxURILength) }
124-135: Use typed errors for authority checks in SetNFTName.if coll.Authority == "" { - return fmt.Errorf("no authority, cannot set NFT name") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "no authority, cannot set NFT name") } collectionAuthority, err := sdk.AccAddressFromBech32(coll.Authority) if err != nil { - return fmt.Errorf("invalid authority address: %w", err) + return errorsmod.Wrap(err, "invalid authority address") } if !authority.Equals(collectionAuthority) { - return fmt.Errorf("only the collection authority can set NFT name") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can set NFT name") }
151-168: Use typed errors for authority checks in SetNFTUri.if coll.Authority == "" { - return fmt.Errorf("no authority, cannot set NFT uri") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "no authority, cannot set NFT uri") } collectionAuthority, err := sdk.AccAddressFromBech32(coll.Authority) if err != nil { - return fmt.Errorf("invalid authority address: %w", err) + return errorsmod.Wrap(err, "invalid authority address") } if !authority.Equals(collectionAuthority) { - return fmt.Errorf("only the collection authority can set NFT uri") + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can set NFT uri") }
71-73: Wrap persistence error with errorsmod.- if err := k.setNft(ctx, nft); err != nil { - return fmt.Errorf("failed to set NFT: %w", err) - } + if err := k.setNft(ctx, nft); err != nil { + return errorsmod.Wrap(err, "failed to set NFT") + }x/nft/keeper/collection.go (1)
154-165: Use typed error when minting is disabled in GetMinter.- if coll.Minter == "" { - return nil, fmt.Errorf("minting disabled for this collection") - } + if coll.Minter == "" { + return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "minting disabled for this collection") + }
🧹 Nitpick comments (10)
proto/bitsong/nft/v1beta1/nft.proto (3)
17-21: Consider annotating address fields with cosmos address scalar.If you already depend on cosmos-proto, use
[(cosmos_proto.scalar) = "cosmos.AddressString"]on creator/minter/authority/owner to improve SDK/clients UX. Otherwise, keep as string.- string creator = 6; - string minter = 7; - string authority = 8; + string creator = 6 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + string minter = 7 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + string authority = 8 [(cosmos_proto.scalar) = "cosmos.AddressString"];Note: requires adding
buf.build/cosmos/cosmos-prototo buf deps.Also applies to: 33-36, 52-53
40-42: Allocate or reserve planned fields now to avoid wire-breaking changes.You plan
seller_fee_bpsandpayment_address. Either add them with safe defaults or reserve their field numbers now.// seller_fee_bps // payment_address + // reserved 10, 11; // reserve now if not adding fields yet
24-24: Message casing nit.If you prefer consistency with common SDK modules, consider
NFTinstead ofNft. Not required.x/nft/keeper/edition_test.go (1)
54-55: Assert Editions counter after prints.Validate NFT state reflects three editions.
suite.NoError(err) suite.Equal(uint64(3), edition) + + // verify on-chain state + nft, err := suite.keeper.GetNft(suite.ctx, collectionDenom, testNft1.TokenId) + suite.NoError(err) + suite.Equal(uint64(3), nft.Editions)x/nft/keeper/nft.go (1)
75-77: Emit Mint events.Add Transfer-like events for Mint to aid indexing and UX.
- // TODO: add events + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvents(sdk.Events{ + sdk.NewEvent( + types.EventTypeMintNft, + sdk.NewAttribute(types.AttributeKeyReceiver, owner.String()), + sdk.NewAttribute(types.AttributeKeyCollection, collectionDenom), + sdk.NewAttribute(types.AttributeKeyTokenId, tokenId), + ), + sdk.NewEvent( + sdk.EventTypeMessage, + sdk.NewAttribute(types.AttributeKeySender, minter.String()), + ), + })Note: ensure constants exist or add them in types/events.go.
x/nft/keeper/keeper_test.go (1)
129-171: Reduce duplication in test setup with small helpers.Collection/NFT creation and common assertions repeat a lot. Introduce helpers to create a collection, mint, and fetch owners to simplify and speed up maintenance.
Also applies to: 185-227, 229-297, 298-331, 332-365, 367-422, 424-523, 524-589
x/nft/keeper/collection.go (4)
219-232: Return typed validation errors from createCollectionDenom.- if strings.TrimSpace(symbol) == "" { - return "", fmt.Errorf("symbol cannot be empty") - } + if strings.TrimSpace(symbol) == "" { + return "", errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "symbol cannot be empty") + } @@ - if len(symbol) > types.MaxSymbolLength { - return "", fmt.Errorf("symbol cannot be longer than %d characters", types.MaxSymbolLength) - } + if len(symbol) > types.MaxSymbolLength { + return "", errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "symbol cannot be longer than %d characters", types.MaxSymbolLength) + }
255-265: Return typed validation errors from validateCollectionMetadata.- if len(name) > types.MaxNameLength { - return fmt.Errorf("name cannot be longer than %d characters", types.MaxNameLength) - } + if len(name) > types.MaxNameLength { + return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "name cannot be longer than %d characters", types.MaxNameLength) + } @@ - if len(uri) > types.MaxURILength { - return fmt.Errorf("uri cannot be longer than %d characters", types.MaxURILength) - } + if len(uri) > types.MaxURILength { + return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "uri cannot be longer than %d characters", types.MaxURILength) + }
74-92: Emit events for metadata/authority/minter updates (observability).Consider emitting events (module, denom, actor, fields changed) for SetCollectionName/Uri, SetMinter, SetAuthority to aid indexers and wallets.
Also applies to: 93-111, 112-134, 135-153
16-24: UnifyCreateCollection’s context parameter (optional consistency)
All otherKeepermethods in x/nft/keeper (e.g.SendNFT,SetCollectionName,SetAuthority, etc.) accept acontext.ContextbutCreateCollectioncurrently takes ansdk.Context. For consistency, change its signature tofunc (k Keeper) CreateCollection(ctx context.Context, …)and callsdk.UnwrapSDKContext(ctx)inside.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
proto/bitsong/nft/v1beta1/nft.proto(1 hunks)x/nft/keeper/collection.go(1 hunks)x/nft/keeper/edition_test.go(1 hunks)x/nft/keeper/grpc_query_test.go(1 hunks)x/nft/keeper/keeper_test.go(1 hunks)x/nft/keeper/msg_server.go(1 hunks)x/nft/keeper/msg_server_test.go(1 hunks)x/nft/keeper/nft.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/nft/keeper/msg_server_test.go
- x/nft/keeper/msg_server.go
- x/nft/keeper/grpc_query_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
x/nft/keeper/edition_test.go (1)
x/nft/keeper/keeper_test.go (1)
KeeperTestSuite(66-75)
x/nft/keeper/keeper_test.go (5)
x/nft/types/nft.pb.go (6)
Collection(26-35)Collection(39-39)Collection(40-42)Nft(70-79)Nft(83-83)Nft(84-86)app/testing/test_suite.go (1)
KeeperTestHelper(31-45)x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/keeper/msg_server.go (1)
NewMsgServerImpl(18-20)x/nft/types/query.pb.go (3)
QueryAllNftsByOwnerRequest(333-336)QueryAllNftsByOwnerRequest(340-340)QueryAllNftsByOwnerRequest(341-343)
x/nft/keeper/nft.go (5)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/errors.go (1)
ErrCollectionNotFound(7-7)x/nft/types/keys.go (4)
MaxNftsInCollection(25-25)MaxTokenIdLength(19-19)MaxNameLength(21-21)MaxURILength(22-22)x/nft/types/nft.pb.go (6)
Nft(70-79)Nft(83-83)Nft(84-86)Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/events.go (6)
EventTypeTransferNft(6-6)AttributeKeyReceiver(10-10)AttributeKeySender(9-9)AttributeKeyCollection(12-12)AttributeKeyTokenId(13-13)NewNftReceivedEvent(16-23)
x/nft/keeper/collection.go (4)
x/nft/keeper/keeper.go (1)
Keeper(53-66)x/nft/types/nft.pb.go (3)
Collection(26-35)Collection(39-39)Collection(40-42)x/nft/types/errors.go (2)
ErrCollectionNotFound(7-7)ErrCollectionAlreadyExists(6-6)x/nft/types/keys.go (3)
MaxSymbolLength(20-20)MaxNameLength(21-21)MaxURILength(22-22)
🪛 Buf (1.55.1)
proto/bitsong/nft/v1beta1/nft.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (1)
proto/bitsong/nft/v1beta1/nft.proto (1)
17-21: PR description vs code: “authority” is implemented.The proto defines
Collection.authorityand keeper enforces it, while the PR checklist marks authority as “pending”. Update the PR description or remove the field to avoid confusion.
| string denom = 1; | ||
|
|
||
| string symbol = 2; | ||
| string name = 3; | ||
| string uri = 5; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reserve skipped field number 4 in Collection.
You’re using 1,2,3,5. Reserve 4 to prevent accidental reuse and wire incompatibility later.
message Collection {
option (gogoproto.goproto_getters) = false;
+ reserved 4;📝 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.
| string denom = 1; | |
| string symbol = 2; | |
| string name = 3; | |
| string uri = 5; | |
| message Collection { | |
| option (gogoproto.goproto_getters) = false; | |
| reserved 4; | |
| string denom = 1; | |
| string symbol = 2; | |
| string name = 3; | |
| string uri = 5; | |
| } |
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/nft.proto around lines 11 to 16, the Collection
message currently uses field numbers 1, 2, 3 and 5 but skips 4; reserve field
number 4 to prevent accidental reuse and wire incompatibility by adding a
reserved declaration for field number 4 inside the Collection message (e.g.,
reserve the numeric tag 4) so future changes cannot reuse that tag.
| string collection = 1; | ||
| string token_id = 2; | ||
|
|
||
| string name = 3; | ||
| string uri = 5; | ||
|
|
||
| string owner = 6; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reserve skipped field number 4 in Nft.
Same rationale as Collection: protect future evolution.
message Nft {
option (gogoproto.goproto_getters) = false;
+ reserved 4;📝 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.
| string collection = 1; | |
| string token_id = 2; | |
| string name = 3; | |
| string uri = 5; | |
| string owner = 6; | |
| message Nft { | |
| option (gogoproto.goproto_getters) = false; | |
| reserved 4; | |
| string collection = 1; | |
| string token_id = 2; | |
| string name = 3; | |
| string uri = 5; | |
| string owner = 6; | |
| } |
🤖 Prompt for AI Agents
In proto/bitsong/nft/v1beta1/nft.proto around lines 27 to 33, the Nft message is
missing a reservation for field number 4; add a reserved 4; declaration inside
the Nft message (e.g., after token_id = 2;) to explicitly reserve field number 4
so it cannot be reused in future evolutions; ensure the reserved statement uses
protobuf syntax "reserved 4;" and keep it near the other field declarations for
clarity.
| package keeper | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| errorsmod "cosmossdk.io/errors" | ||
| "cosmossdk.io/math" | ||
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | ||
| tmcrypto "github.com/cometbft/cometbft/crypto" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Imports already include errorsmod/sdkerrors; proposed diffs won’t add new deps.
After applying the typed errors changes, ensure there are no leftover fmt.Errorf usages.
🏁 Script executed:
#!/bin/bash
rg -n "fmt\\.Errorf\\(" x/nft/keeper/collection.goLength of output: 883
Replace fmt.Errorf calls with typed errors
There are leftover fmt.Errorf usages in x/nft/keeper/collection.go (lines 81, 100, 119, 123, 142, 161, 174, 223, 227, 257, 261). Update them to use errorsmod or sdkerrors helpers for consistent, typed error handling.
🤖 Prompt for AI Agents
In x/nft/keeper/collection.go around lines 81, 100, 119, 123, 142, 161, 174,
223, 227, 257 and 261 replace the plain fmt.Errorf calls with typed Cosmos SDK
errors: pick the appropriate sdkerrors sentinel (e.g.
sdkerrors.ErrInvalidRequest, sdkerrors.ErrUnauthorized, sdkerrors.ErrNotFound,
etc.) or use errorsmod.Wrapf/errorsmod.Wrap to preserve context and formatting
(e.g. errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "describe: %s", val) or
sdkerrors.ErrUnauthorized.Wrapf("describe: %s", val)); ensure the error
semantics match the original message, adjust the return signatures if needed,
and remove the unused fmt import if no other fmt usage remains.
| if coll.Authority != authority.String() { | ||
| return fmt.Errorf("only the collection authority can change the name") | ||
| } | ||
|
|
||
| if err := k.validateCollectionMetadata(name, coll.Uri); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| coll.Name = name | ||
|
|
||
| return k.setCollection(ctx, coll) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed SDK errors for authorization failures in SetCollectionName.
Return proper ABCI codes instead of plain fmt errors.
- if coll.Authority != authority.String() {
- return fmt.Errorf("only the collection authority can change the name")
- }
+ if coll.Authority != authority.String() {
+ return errorsmod.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can change the name")
+ }📝 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.
| if coll.Authority != authority.String() { | |
| return fmt.Errorf("only the collection authority can change the name") | |
| } | |
| if err := k.validateCollectionMetadata(name, coll.Uri); err != nil { | |
| return err | |
| } | |
| coll.Name = name | |
| return k.setCollection(ctx, coll) | |
| } | |
| if coll.Authority != authority.String() { | |
| return errorsmod.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can change the name") | |
| } | |
| if err := k.validateCollectionMetadata(name, coll.Uri); err != nil { | |
| return err | |
| } | |
| coll.Name = name | |
| return k.setCollection(ctx, coll) | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/collection.go around lines 80 to 91, replace the plain
fmt.Errorf used for the authorization check with a typed Cosmos SDK error (e.g.
return sdkerrors.ErrUnauthorized.Wrapf("only the collection authority (%s) can
change the name", coll.Authority) or sdkerrors.ErrUnauthorized.WithMessage("only
the collection authority can change the name")), import
"github.com/cosmos/cosmos-sdk/types/errors" (alias sdkerrors) at the top, and
ensure the function returns that SDK error so callers get an ABCI code rather
than a plain fmt error.
| if coll.Authority != authority.String() { | ||
| return fmt.Errorf("only the collection authority can change the uri") | ||
| } | ||
|
|
||
| if err := k.validateCollectionMetadata(coll.Name, uri); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| coll.Uri = uri | ||
|
|
||
| return k.setCollection(ctx, coll) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed SDK errors for authorization failures in SetCollectionUri.
- if coll.Authority != authority.String() {
- return fmt.Errorf("only the collection authority can change the uri")
- }
+ if coll.Authority != authority.String() {
+ return errorsmod.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can change the uri")
+ }📝 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.
| if coll.Authority != authority.String() { | |
| return fmt.Errorf("only the collection authority can change the uri") | |
| } | |
| if err := k.validateCollectionMetadata(coll.Name, uri); err != nil { | |
| return err | |
| } | |
| coll.Uri = uri | |
| return k.setCollection(ctx, coll) | |
| } | |
| if coll.Authority != authority.String() { | |
| return errorsmod.Wrap(sdkerrors.ErrUnauthorized, "only the collection authority can change the uri") | |
| } | |
| if err := k.validateCollectionMetadata(coll.Name, uri); err != nil { | |
| return err | |
| } | |
| coll.Uri = uri | |
| return k.setCollection(ctx, coll) | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/collection.go around lines 99 to 110, replace the plain
fmt.Errorf authorization error with a typed Cosmos SDK error (e.g., return
sdkerrors.ErrUnauthorized.Wrap("only the collection authority can change the
uri") or Wrapf if you want formatting) and add the import for the SDK errors
package ("github.com/cosmos/cosmos-sdk/types/errors") if missing; keep the rest
of the function unchanged and ensure the function returns the new sdk error type
so callers can handle it consistently.
| expectedDenom1 = "nft9436DDD23FB751AEA7BC6C767F20F943DD735E06" | ||
|
|
||
| testCollection2 = types.Collection{ | ||
| Name: "My Second NFT Collection", | ||
| Symbol: "MYNFT2", | ||
| Uri: "ipfs://my-nft-collection-metadata.json", | ||
| Minter: minter1.String(), | ||
| } | ||
| expectedDenom2 = "nft6C5B4EC9EA22932F217B0A0CDCA3A987B8271CD0" | ||
|
|
There was a problem hiding this comment.
Fix denom expectations: tests use uppercase but keeper now emits lowercase.
createCollectionDenom uses %x (lowercase hex). Update the expected strings to lowercase to match runtime behavior.
-expectedDenom1 = "nft9436DDD23FB751AEA7BC6C767F20F943DD735E06"
+expectedDenom1 = "nft9436ddd23fb751aea7bc6c767f20f943dd735e06"
@@
-expectedDenom2 = "nft6C5B4EC9EA22932F217B0A0CDCA3A987B8271CD0"
+expectedDenom2 = "nft6c5b4ec9ea22932f217b0a0cdca3a987b8271cd0"Prefer deriving the expected denom instead of hardcoding to avoid brittleness across address prefix changes:
// helper (place near the top of the test file)
func expectedDenomFor(t *testing.T, creator sdk.AccAddress, symbol string) string {
t.Helper()
bz := []byte(fmt.Sprintf("%s/%s", creator.String(), symbol))
return fmt.Sprintf("nft%x", tmcrypto.AddressHash(bz))
}Then replace the equality assertions accordingly.
🤖 Prompt for AI Agents
In x/nft/keeper/keeper_test.go around lines 37 to 46, the test hardcodes
expectedDenom strings in uppercase but the keeper now emits lowercase hex
(createCollectionDenom uses %x), so update the expected values to lowercase or,
better, add and use a helper that derives the expected denom from the creator
address and symbol (compute "nft" + tmcrypto.AddressHash(fmt.Sprintf("%s/%s",
creator.String(), symbol)) formatted with %x) and replace the hardcoded
assertions to call that helper to avoid brittleness across address prefix
changes.
| err = suite.keeper.SetMinter(suite.ctx, minter1, &minter2, collectionDenom) | ||
| suite.NoError(err) | ||
|
|
||
| minter, err := suite.keeper.GetMinter(suite.ctx, collectionDenom) | ||
| suite.NoError(err) | ||
| suite.Equal(minter2.String(), minter.String()) | ||
|
|
||
| err = suite.keeper.SetMinter(suite.ctx, minter1, &minter1, collectionDenom) | ||
| suite.Error(err) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test expects error when setting the same minter; keeper doesn’t enforce this.
This test assumes no-op minter updates are invalid. Keeper.SetMinter currently allows it. Either relax the test or add a guard in the keeper (recommended).
Proposed keeper change (see x/nft/keeper/collection.go SetMinter diff in my other comment) to reject identical new minter with ErrInvalidRequest.
🏁 Script executed:
#!/bin/bash
# Search for SetMinter implementation in the NFT keeper directory
rg -nP "func \\(.*Keeper\\) SetMinter" -n x/nft/keeperLength of output: 207
🏁 Script executed:
#!/bin/bash
# Display SetMinter implementation to verify whether it guards against identical newMinter
sed -n '80,200p' x/nft/keeper/collection.goLength of output: 3025
🏁 Script executed:
#!/bin/bash
# Locate the keeper_test.go file and display lines around the test snippet
file=$(fd --full-path keeper_test.go)
echo "Using test file: $file"
sed -n '290,330p' "$file"Length of output: 802
🏁 Script executed:
#!/bin/bash
# Display lines 300-340 of the NFT keeper test to verify test logic
sed -n '300,340p' x/nft/keeper/keeper_test.goLength of output: 1198
Enforce no-op minter updates as invalid
SetMinter currently allows passing the same address as newMinter, but the test assumes this is an error. Add a check in SetMinter to return ErrInvalidRequest when newMinter.String() == oldMinter.String() before updating the collection.
🤖 Prompt for AI Agents
In x/nft/keeper/keeper_test.go around lines 310 to 319, the test expects
SetMinter to treat a no-op update (newMinter equal to oldMinter) as an error;
update the SetMinter implementation to check if newMinter.String() ==
oldMinter.String() and immediately return sdkerrors.ErrInvalidRequest (or the
module's ErrInvalidRequest) without modifying state. Ensure you perform the
comparison before any state changes, return the proper error type, and add no
further side effects so the test's suite.Error(err) assertion passes.
| package keeper | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "cosmossdk.io/collections" | ||
| "cosmossdk.io/math" | ||
| "github.com/bitsongofficial/go-bitsong/x/nft/types" | ||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| ) | ||
|
|
||
| func (k Keeper) MintNFT( | ||
| ctx context.Context, | ||
| minter sdk.AccAddress, | ||
| owner sdk.AccAddress, | ||
| collectionDenom, | ||
| tokenId, | ||
| name, | ||
| uri string, | ||
| ) error { | ||
| if err := k.validateNftMetadata(tokenId, name, uri); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nftKey := collections.Join(collectionDenom, tokenId) | ||
| has, err := k.NFTs.Has(ctx, nftKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check NFT: %w", err) | ||
| } | ||
| if has { | ||
| return fmt.Errorf("NFT with token ID %s already exists in collection %s", tokenId, collectionDenom) | ||
| } | ||
|
|
||
| coll, err := k.Collections.Get(ctx, collectionDenom) | ||
| if err != nil { | ||
| return types.ErrCollectionNotFound | ||
| } | ||
|
|
||
| if coll.Minter == "" { | ||
| return fmt.Errorf("minting disabled for this collection") | ||
| } | ||
|
|
||
| collectionMinter, err := sdk.AccAddressFromBech32(coll.Minter) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid minter address: %w", err) | ||
| } | ||
|
|
||
| if !minter.Equals(collectionMinter) { | ||
| return fmt.Errorf("only the collection minter can mint NFTs") | ||
| } | ||
|
|
||
| supply := k.GetSupply(ctx, collectionDenom) | ||
| if supply.Equal(math.NewInt(types.MaxNftsInCollection)) { | ||
| return fmt.Errorf("max supply reached for collection %s", collectionDenom) | ||
| } | ||
|
|
||
| // TODO: Charge fee if necessary | ||
|
|
||
| nft := types.Nft{ | ||
| Collection: collectionDenom, | ||
| TokenId: tokenId, | ||
| Name: name, | ||
| Uri: uri, | ||
| Owner: owner.String(), | ||
| Editions: 0, | ||
| } | ||
|
|
||
| if err := k.setNft(ctx, nft); err != nil { | ||
| return fmt.Errorf("failed to set NFT: %w", err) | ||
| } | ||
|
|
||
| // TODO: add events | ||
|
|
||
| return k.incrementSupply(ctx, collectionDenom) | ||
| } | ||
|
|
||
| func (k Keeper) SendNFT(ctx context.Context, fromAddr, toAddr sdk.AccAddress, collectionDenom, tokenId string) error { | ||
| err := k.changeNftOwner(ctx, fromAddr, toAddr, collectionDenom, tokenId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Same as https://github.com/cosmos/cosmos-sdk/blob/v0.53.4/x/bank/keeper/send.go | ||
| // Create account if recipient does not exist. | ||
| // | ||
| // NOTE: This should ultimately be removed in favor a more flexible approach | ||
| // such as delegated fee messages. | ||
| accExists := k.ak.HasAccount(ctx, toAddr) | ||
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr)) | ||
| } | ||
|
|
||
| // Same as https://github.com/cosmos/cosmos-sdk/blob/v0.53.4/x/bank/keeper/send.go | ||
| // bech32 encoding is expensive! Only do it once for fromAddr | ||
| fromAddrString := fromAddr.String() | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| sdkCtx.EventManager().EmitEvents(sdk.Events{ | ||
| sdk.NewEvent( | ||
| types.EventTypeTransferNft, | ||
| sdk.NewAttribute(types.AttributeKeyReceiver, toAddr.String()), | ||
| sdk.NewAttribute(types.AttributeKeySender, fromAddrString), | ||
| sdk.NewAttribute(types.AttributeKeyCollection, collectionDenom), | ||
| sdk.NewAttribute(types.AttributeKeyTokenId, tokenId), | ||
| ), | ||
| sdk.NewEvent( | ||
| sdk.EventTypeMessage, | ||
| sdk.NewAttribute(types.AttributeKeySender, fromAddrString), | ||
| ), | ||
| }) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (k Keeper) SetNFTName(ctx context.Context, authority sdk.AccAddress, collectionDenom, tokenId, name string) error { | ||
| coll, err := k.Collections.Get(ctx, collectionDenom) | ||
| if err != nil { | ||
| return types.ErrCollectionNotFound | ||
| } | ||
|
|
||
| if coll.Authority == "" { | ||
| return fmt.Errorf("no authority, cannot set NFT name") | ||
| } | ||
|
|
||
| collectionAuthority, err := sdk.AccAddressFromBech32(coll.Authority) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid authority address: %w", err) | ||
| } | ||
|
|
||
| if !authority.Equals(collectionAuthority) { | ||
| return fmt.Errorf("only the collection authority can set NFT name") | ||
| } | ||
|
|
||
| nft, err := k.GetNft(ctx, collectionDenom, tokenId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = k.validateNftMetadata(tokenId, name, nft.Uri) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nft.Name = name | ||
| return k.setNft(ctx, *nft) | ||
| } | ||
|
|
||
| func (k Keeper) SetNFTUri(ctx context.Context, authority sdk.AccAddress, collectionDenom, tokenId, uri string) error { | ||
| coll, err := k.Collections.Get(ctx, collectionDenom) | ||
| if err != nil { | ||
| return types.ErrCollectionNotFound | ||
| } | ||
|
|
||
| if coll.Authority == "" { | ||
| return fmt.Errorf("no authority, cannot set NFT uri") | ||
| } | ||
|
|
||
| collectionAuthority, err := sdk.AccAddressFromBech32(coll.Authority) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid authority address: %w", err) | ||
| } | ||
|
|
||
| if !authority.Equals(collectionAuthority) { | ||
| return fmt.Errorf("only the collection authority can set NFT uri") | ||
| } | ||
|
|
||
| nft, err := k.GetNft(ctx, collectionDenom, tokenId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = k.validateNftMetadata(tokenId, nft.Name, uri) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| nft.Uri = uri | ||
| return k.setNft(ctx, *nft) | ||
| } | ||
|
|
||
| func (k Keeper) GetNft(ctx context.Context, collectionDenom, tokenId string) (*types.Nft, error) { | ||
| nftKey := collections.Join(collectionDenom, tokenId) | ||
| has, err := k.NFTs.Has(ctx, nftKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check NFT: %w", err) | ||
| } | ||
| if !has { | ||
| return nil, fmt.Errorf("NFT with token ID %s does not exist in collection %s", tokenId, collectionDenom) | ||
| } | ||
|
|
||
| nft, err := k.NFTs.Get(ctx, nftKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get NFT: %w", err) | ||
| } | ||
|
|
||
| return &nft, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sweep for remaining fmt.Errorf in keeper to standardize errors.
Automate detection to avoid regressions.
🏁 Script executed:
#!/bin/bash
# Find plain fmt.Errorf in keeper (should be replaced by errorsmod/sdkerrors).
rg -nP --type=go -C2 '\bfmt\.Errorf\(' x/nft/keeperLength of output: 10227
Standardize keeper errors: replace fmt.Errorf with sdkerrors.Wrap
Replace all plain fmt.Errorf usages in x/nft/keeper (e.g., in nft.go, edition.go, collection.go) with sdkerrors.Wrap or errorsmod.Wrap using registered error codes, and add a lint rule to forbid plain fmt.Errorf in the keeper package.
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go lines 1-200, replace all uses of fmt.Errorf with the
Cosmos SDK error wrapping (sdkerrors.Wrap or errorsmod.Wrap) using the module's
registered errors from the x/nft/types package (e.g.,
types.ErrCollectionNotFound, types.ErrUnauthorized, types.ErrMaxSupplyReached,
types.ErrInvalidAddress, types.ErrNotFound, types.ErrAlreadyExists, etc.) so
each returned error uses the appropriate typed error and wraps the original
error when available (e.g., wrap underlying DB/collection errors), update error
messages to be short context strings passed to Wrap, and add/update linting to
forbid plain fmt.Errorf in the keeper package.
| func (k Keeper) changeNftOwner(ctx context.Context, oldOwner, newOwner sdk.AccAddress, collectionDenom string, tokenId string) error { | ||
| if oldOwner.Equals(newOwner) { | ||
| return fmt.Errorf("cannot transfer NFT to the same owner") | ||
| } | ||
|
|
||
| nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) | ||
| if err != nil { | ||
| return fmt.Errorf("collection or token_id does not exist") | ||
| } | ||
|
|
||
| if nft.Owner != oldOwner.String() { | ||
| return fmt.Errorf("only the owner can transfer the NFT") | ||
| } | ||
|
|
||
| nft.Owner = newOwner.String() | ||
| err = k.setNft(ctx, nft) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to set NFT Owner: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return typed errors during transfer; preserve underlying error.
if oldOwner.Equals(newOwner) {
- return fmt.Errorf("cannot transfer NFT to the same owner")
+ return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "cannot transfer NFT to the same owner")
}
nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId))
if err != nil {
- return fmt.Errorf("collection or token_id does not exist")
+ return errorsmod.Wrap(err, "failed to get NFT")
}
if nft.Owner != oldOwner.String() {
- return fmt.Errorf("only the owner can transfer the NFT")
+ return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "only the owner can transfer the NFT")
}
nft.Owner = newOwner.String()
err = k.setNft(ctx, nft)
if err != nil {
- return fmt.Errorf("failed to set NFT Owner: %w", err)
+ return errorsmod.Wrap(err, "failed to set NFT owner")
}📝 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.
| func (k Keeper) changeNftOwner(ctx context.Context, oldOwner, newOwner sdk.AccAddress, collectionDenom string, tokenId string) error { | |
| if oldOwner.Equals(newOwner) { | |
| return fmt.Errorf("cannot transfer NFT to the same owner") | |
| } | |
| nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) | |
| if err != nil { | |
| return fmt.Errorf("collection or token_id does not exist") | |
| } | |
| if nft.Owner != oldOwner.String() { | |
| return fmt.Errorf("only the owner can transfer the NFT") | |
| } | |
| nft.Owner = newOwner.String() | |
| err = k.setNft(ctx, nft) | |
| if err != nil { | |
| return fmt.Errorf("failed to set NFT Owner: %w", err) | |
| } | |
| func (k Keeper) changeNftOwner( | |
| ctx context.Context, | |
| oldOwner, newOwner sdk.AccAddress, | |
| collectionDenom, tokenId string, | |
| ) error { | |
| if oldOwner.Equals(newOwner) { | |
| return sdkerrors.Wrap( | |
| sdkerrors.ErrInvalidRequest, | |
| "cannot transfer NFT to the same owner", | |
| ) | |
| } | |
| nft, err := k.NFTs.Get( | |
| ctx, | |
| collections.Join(collectionDenom, tokenId), | |
| ) | |
| if err != nil { | |
| return errorsmod.Wrap(err, "failed to get NFT") | |
| } | |
| if nft.Owner != oldOwner.String() { | |
| return sdkerrors.Wrap( | |
| sdkerrors.ErrUnauthorized, | |
| "only the owner can transfer the NFT", | |
| ) | |
| } | |
| nft.Owner = newOwner.String() | |
| if err := k.setNft(ctx, nft); err != nil { | |
| return errorsmod.Wrap(err, "failed to set NFT owner") | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 233 to 251, the transfer routine returns
plain formatted strings and drops underlying errors; replace those fmt.Errorf
string-only returns with package-level typed errors (e.g., ErrInvalidTransfer,
ErrNotFound, ErrNotOwner, ErrSetNFTFailed) and wrap the underlying error using
%w where applicable so callers can inspect error types; specifically, return
ErrInvalidTransfer when oldOwner equals newOwner, wrap the NFTs.Get error as
fmt.Errorf("%w: %v", ErrNotFound, err) or use errors.Wrap semantics to preserve
the original err, return ErrNotOwner when ownership mismatch, and when setNft
fails return fmt.Errorf("%w: %w", ErrSetNFTFailed, err) (or equivalent) so the
error type is preserved and underlying error is included.
| func (k Keeper) incrementEdition(ctx context.Context, collectionDenom, tokenId string) error { | ||
| nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get NFT: %w", err) | ||
| } | ||
|
|
||
| nft.Editions += 1 | ||
| return k.setNft(ctx, nft) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap edition persistence error.
nft.Editions += 1
-return k.setNft(ctx, nft)
+if err := k.setNft(ctx, nft); err != nil {
+ return errorsmod.Wrap(err, "failed to set NFT editions")
+}
+return nil📝 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.
| func (k Keeper) incrementEdition(ctx context.Context, collectionDenom, tokenId string) error { | |
| nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) | |
| if err != nil { | |
| return fmt.Errorf("failed to get NFT: %w", err) | |
| } | |
| nft.Editions += 1 | |
| return k.setNft(ctx, nft) | |
| } | |
| func (k Keeper) incrementEdition(ctx context.Context, collectionDenom, tokenId string) error { | |
| nft, err := k.NFTs.Get(ctx, collections.Join(collectionDenom, tokenId)) | |
| if err != nil { | |
| return fmt.Errorf("failed to get NFT: %w", err) | |
| } | |
| nft.Editions += 1 | |
| if err := k.setNft(ctx, nft); err != nil { | |
| return errorsmod.Wrap(err, "failed to set NFT editions") | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In x/nft/keeper/nft.go around lines 262 to 270, the call to k.setNft returns an
error but it's passed through raw; wrap that persistence error with context
before returning. Capture the error from k.setNft(ctx, nft) and return a
formatted error such as fmt.Errorf("failed to persist NFT edition update for
%s/%s: %w", collectionDenom, tokenId, err) so callers get actionable context.
- Added Keeper methods for creating and retrieving drops. - Implemented validation for drop rules including start and end time checks. - Introduced RuleEngine to manage and validate rules. - Created EndTimeRule and StartTimeRule for specific rule validations. - Added protobuf definitions for Drop and Rule types. - Established collections for storing drops and rules with appropriate prefixes. - Implemented unit tests for rules engine validation.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (9)
x/drop/rules/engine_test.go (2)
14-20: Avoid shadowing the package name; use singular “rule”.Minor readability improvement; prevents confusion within package rules.
- rules := types.Rule{ + rule := types.Rule{ StartTime: startTime, EndTime: endTime, } - engine, err := NewRuleEngineFromRule(rules) + engine, err := NewRuleEngineFromRule(rule)
10-32: Positive path test looks good. Consider adding boundary tests.Add tests for:
- Start only (no EndTime → default applied)
- Neither start nor end (engine should not auto-fail)
- At exact boundaries (== StartTime and == EndTime)
proto/bitsong/drop/v1beta1/drop.proto (1)
22-27: Consider optionality of timestamps vs engine defaults.You mark timestamps non-nullable; the engine adds a default EndTime only if EndTime is zero. Ensure behavior when both timestamps are zero is defined (see engine.go comment); consider documenting this in proto comments.
x/drop/keeper/drop.go (1)
29-34: Validate collectionDenom non-empty and existence.Guard against empty denom; optionally verify the collection exists (if API available).
// 2. check if collections has nfts, if yes return error, only empty collections can be dropped +if collectionDenom == "" { + return fmt.Errorf("collection denom must not be empty") +} collSupply := k.nftKeeper.GetSupply(ctx, collectionDenom)x/drop/rules/engine.go (1)
39-47: Deterministic rule evaluation order.Map iteration is random; sort for reproducible behavior and easier debugging.
-import "fmt" +import ( + "fmt" + "sort" + "time" + "github.com/bitsongofficial/go-bitsong/x/drop/types" +) @@ -func (e *RuleEngine) Validate(req *RuleEngineRequest) error { - for name, rule := range e.rules { - if err := rule.Validate(req); err != nil { - return fmt.Errorf("rule %s validation failed: %w", name, err) - } - } +func (e *RuleEngine) Validate(req *RuleEngineRequest) error { + names := make([]string, 0, len(e.rules)) + for name := range e.rules { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + rule := e.rules[name] + if err := rule.Validate(req); err != nil { + return fmt.Errorf("rule %s validation failed: %w", name, err) + } + } return nil }x/drop/keeper/keeper.go (4)
3-12: Add context to panic for easier debugging.Wrap the error with context; current panic(err) loses signal.
import ( + "fmt" "cosmossdk.io/collections" @@ - if err != nil { - panic(err) - } + if err != nil { + panic(fmt.Errorf("drop keeper: schema build failed: %w", err)) + }Also applies to: 53-56
38-51: LGTM: collections maps wired correctly; consider typed keys.Keys are plain strings. If denoms/rule IDs have canonical form, consider a small KeyCodec or normalization helper to avoid inconsistent keys.
20-21: Depend on an interface, not concrete nftkeeper.Keeper.To avoid tight coupling/cycles, define an expected interface in x/drop/types (e.g., ExpectedNftKeeper) and store that in Keeper. Wire the concrete keeper at app level.
Example (outside this file):
- x/drop/types/expected_keepers.go
- type NftKeeper interface { GetSupply(ctx context.Context, denom string) (uint64, error); ... }
- In this file: replace import of nft/keeper with x/drop/types and field to types.NftKeeper.
16-16: Remove unused storeKey field and constructor parameter
storeKey is set in NewKeeper but never referenced in any Keeper methods in x/drop/keeper/keeper.go (nok.storeKeyoccurrences). Drop thestoreKey storetypes.StoreKeyfield and its constructor argument to clean up the API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/drop/types/drop.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
proto/bitsong/drop/v1beta1/drop.proto(1 hunks)proto/buf.yaml(1 hunks)x/drop/keeper/drop.go(1 hunks)x/drop/keeper/keeper.go(1 hunks)x/drop/rules/end_time.go(1 hunks)x/drop/rules/engine.go(1 hunks)x/drop/rules/engine_test.go(1 hunks)x/drop/rules/start_time.go(1 hunks)x/drop/types/keys.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
x/drop/rules/engine_test.go (2)
x/drop/types/drop.pb.go (3)
Rule(69-73)Rule(77-77)Rule(78-80)x/drop/rules/engine.go (2)
NewRuleEngineFromRule(49-78)RuleEngineRequest(10-12)
x/drop/keeper/drop.go (4)
x/drop/keeper/keeper.go (1)
Keeper(14-25)x/drop/types/drop.pb.go (6)
Rule(69-73)Rule(77-77)Rule(78-80)Drop(108-112)Drop(116-116)Drop(117-119)x/nft/types/keys.go (1)
MaxNftsInCollection(25-25)x/drop/types/keys.go (1)
MaxRulesPerDrop(10-10)
x/drop/rules/end_time.go (1)
x/drop/rules/engine.go (1)
RuleEngineRequest(10-12)
x/drop/keeper/keeper.go (2)
x/drop/types/keys.go (4)
StoreKey(7-7)ModuleName(6-6)DropsPrefix(15-15)RulesPrefix(16-16)x/drop/types/drop.pb.go (6)
Drop(108-112)Drop(116-116)Drop(117-119)Rule(69-73)Rule(77-77)Rule(78-80)
x/drop/rules/start_time.go (1)
x/drop/rules/engine.go (1)
RuleEngineRequest(10-12)
x/drop/rules/engine.go (3)
x/drop/types/drop.pb.go (3)
Rule(69-73)Rule(77-77)Rule(78-80)x/drop/rules/start_time.go (1)
StartTimeRule(8-10)x/drop/rules/end_time.go (1)
EndTimeRule(8-10)
🪛 Buf (1.55.1)
proto/bitsong/drop/v1beta1/drop.proto
6-6: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (1)
x/drop/keeper/keeper.go (1)
27-29: LGTM: module-scoped logger.Using logger.With(log.ModuleKey, "x/"+types.ModuleName) is correct and consistent.
|
|
||
| option go_package = "github.com/bitsongofficial/go-bitsong/x/drop/types"; | ||
|
|
||
| import "gogoproto/gogo.proto"; |
There was a problem hiding this comment.
Buf compile error: cannot resolve gogoproto.
The import path is correct, but buf deps list uses the wrong module name. See buf.yaml fix to use buf.build/cosmos/gogoproto.
🧰 Tools
🪛 Buf (1.55.1)
6-6: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In proto/bitsong/drop/v1beta1/drop.proto around line 6 the import
"gogoproto/gogo.proto" is correct but BUF fails because buf.yaml lists the wrong
module; open buf.yaml and replace the incorrect dependency module with the
canonical module "buf.build/cosmos/gogoproto" (or add it if missing), then run
buf mod update / buf build to fetch the dependency so the gogoproto import
resolves correctly.
| uint64 id = 1; | ||
|
|
||
| google.protobuf.Timestamp start_time = 2 | ||
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | ||
|
|
||
| google.protobuf.Timestamp end_time = 3 | ||
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify Rule.id type with storage and validations.
Currently proto uses uint64 but storage keys use string and you have MaxRuleIDLength. If string IDs are intended, switch proto to string; otherwise update storage to use uint64.
Option A (string IDs in proto):
- uint64 id = 1;
+ string id = 1;Option B (numeric IDs end-to-end): change Rules map to collections.Map[collections.Pair[string, uint64], types.Rule] and drop MaxRuleIDLength.
📝 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.
| uint64 id = 1; | |
| google.protobuf.Timestamp start_time = 2 | |
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | |
| google.protobuf.Timestamp end_time = 3 | |
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | |
| } | |
| // File: proto/bitsong/drop/v1beta1/drop.proto | |
| // ————————————————————————————————————————————— | |
| string id = 1; | |
| google.protobuf.Timestamp start_time = 2 | |
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | |
| google.protobuf.Timestamp end_time = 3 | |
| [(gogoproto.stdtime) = true, (gogoproto.nullable) = false, (amino.dont_omitempty) = true]; | |
| } |
🤖 Prompt for AI Agents
In proto/bitsong/drop/v1beta1/drop.proto around lines 20 to 27, the Rule.id
field is declared as uint64 but storage and validation use string keys and
MaxRuleIDLength; unify the ID type end-to-end. Either (A) change the proto field
to string and update any generated code, RPC handlers, and validations to treat
IDs as strings (remove numeric parsing and length checks remain valid), or (B)
keep numeric IDs by updating storage map types and validations to use uint64
keys (change Rules map to collections.Map[collections.Pair[string, uint64],
types.Rule] or equivalent, remove MaxRuleIDLength and any string-length
validation, and ensure all serialization/deserialization and clients
parse/format uint64 consistently). Implement the chosen option across proto
definitions, storage types, validation logic, and any client/server parsing
spots so IDs are consistently typed end-to-end.
| - buf.build/cosmos/cosmos-proto | ||
| - buf.build/cosmos/gogo-proto | ||
| - buf.build/googleapis/googleapis | ||
| - buf.build/protocolbuffers/wellknowntypes |
There was a problem hiding this comment.
Fix buf dependency to resolve gogoproto imports.
Buf reports "gogoproto/gogo.proto: file does not exist" because the module name is wrong in deps. Use cosmos/gogoproto (no hyphen).
deps:
- buf.build/cosmos/cosmos-sdk
- buf.build/cosmos/cosmos-proto
- - buf.build/cosmos/gogo-proto
+ - buf.build/cosmos/gogoproto
- buf.build/googleapis/googleapis
- buf.build/protocolbuffers/wellknowntypes📝 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.
| - buf.build/protocolbuffers/wellknowntypes | |
| deps: | |
| - buf.build/cosmos/cosmos-sdk | |
| - buf.build/cosmos/cosmos-proto | |
| - buf.build/cosmos/gogoproto | |
| - buf.build/googleapis/googleapis | |
| - buf.build/protocolbuffers/wellknowntypes |
🤖 Prompt for AI Agents
In proto/buf.yaml around line 8, the buf dependency entry is incorrect and
causes "gogoproto/gogo.proto: file does not exist"; replace the wrong module
with the correct Cosmos gogoproto module by removing or replacing the current
deps entry and adding buf.build/cosmos/gogoproto (no hyphen) as the dependency
so imports like gogoproto/gogo.proto resolve correctly; save the file and re-run
buf mod update or your build to ensure the new dependency is fetched.
| ignore: | ||
| - testpb |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commit buf.lock for reproducible builds.
Ensure proto/buf.lock is checked in after updating deps, so CI and local builds resolve the same module digests.
Run:
🏁 Script executed:
#!/bin/bash
buf dep update && git status --porcelain | grep -E '^.M|^\?\?' proto/buf.lock || trueLength of output: 60
🏁 Script executed:
#!/bin/bash
# Check if proto/buf.lock is present in the repo
git ls-files | grep -E '^proto/buf\.lock$' || echo "proto/buf.lock not tracked"
# Display contents of proto/buf.yaml to verify if any dependencies are configured
echo
echo "Contents of proto/buf.yaml:"
cat proto/buf.yamlLength of output: 649
Commit updated proto/buf.lock for reproducible builds
Ensure proto/buf.lock is updated and checked in after changing deps so CI and local builds resolve the same module digests. For example:
pushd proto
buf dep update && git status --porcelain | grep -E '^.M|^\?\?' buf.lock || true
popd🤖 Prompt for AI Agents
In proto/buf.yaml around lines 12-13, the project changed proto dependencies but
did not update and commit proto/buf.lock for reproducible builds; run the buf
dependency update inside the proto directory to regenerate proto/buf.lock, add
and commit the updated proto/buf.lock to the repo, and ensure CI/local builds
use the committed lockfile so module digests resolve identically.
| // 3. check drop max available (max is MaxNftsInCollection) | ||
| if math.NewUint(maxAvailable).GT(math.NewUint(nfttypes.MaxNftsInCollection)) { | ||
| return fmt.Errorf("max available %d exceeds max allowed %d", maxAvailable, nfttypes.MaxNftsInCollection) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate maxAvailable lower bound.
Reject zero to avoid nonsensical drops.
// 3. check drop max available (max is MaxNftsInCollection)
+ if maxAvailable == 0 {
+ return fmt.Errorf("max available must be > 0")
+ }
if math.NewUint(maxAvailable).GT(math.NewUint(nfttypes.MaxNftsInCollection)) {
return fmt.Errorf("max available %d exceeds max allowed %d", maxAvailable, nfttypes.MaxNftsInCollection)
}📝 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.
| // 3. check drop max available (max is MaxNftsInCollection) | |
| if math.NewUint(maxAvailable).GT(math.NewUint(nfttypes.MaxNftsInCollection)) { | |
| return fmt.Errorf("max available %d exceeds max allowed %d", maxAvailable, nfttypes.MaxNftsInCollection) | |
| } | |
| // 3. check drop max available (max is MaxNftsInCollection) | |
| if maxAvailable == 0 { | |
| return fmt.Errorf("max available must be > 0") | |
| } | |
| if math.NewUint(maxAvailable).GT(math.NewUint(nfttypes.MaxNftsInCollection)) { | |
| return fmt.Errorf("max available %d exceeds max allowed %d", maxAvailable, nfttypes.MaxNftsInCollection) | |
| } |
🤖 Prompt for AI Agents
In x/drop/keeper/drop.go around lines 36 to 39, add a lower-bound validation for
maxAvailable to reject zero values; before comparing to
nfttypes.MaxNftsInCollection, check if maxAvailable == 0 and return a clear
error (e.g., "max available must be greater than zero") so zero drops are
rejected, then proceed with the existing upper-bound check.
| req := &RuleEngineRequest{ | ||
| currentTime: time.Now(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export request field to allow out-of-package construction.
currentTime is unexported; external callers can’t build requests. Rename to CurrentTime across the rules engine.
- req := &RuleEngineRequest{
- currentTime: time.Now(),
- }
+ req := &RuleEngineRequest{
+ CurrentTime: time.Now(),
+ }Additionally update usages in x/drop/rules/{engine.go,start_time.go,end_time.go} as suggested in their comments.
📝 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.
| req := &RuleEngineRequest{ | |
| currentTime: time.Now(), | |
| } | |
| req := &RuleEngineRequest{ | |
| CurrentTime: time.Now(), | |
| } |
🤖 Prompt for AI Agents
In x/drop/rules/engine_test.go around lines 24-26, the RuleEngineRequest struct
is instantiated using an unexported field currentTime which prevents
out-of-package construction; rename the field to CurrentTime (exported) in the
RuleEngineRequest definition and update all references accordingly. Modify
x/drop/rules/engine.go, x/drop/rules/start_time.go, and x/drop/rules/end_time.go
to use Request.CurrentTime instead of Request.currentTime (or equivalent
variable names), run tests and adjust any comments or documentation that
referenced the old field name.
| type RuleEngineRequest struct { | ||
| currentTime time.Time | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export CurrentTime so callers outside package can construct requests.
Unexported field blocks usage from other modules.
-type RuleEngineRequest struct {
- currentTime time.Time
-}
+type RuleEngineRequest struct {
+ CurrentTime time.Time
+}📝 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.
| type RuleEngineRequest struct { | |
| currentTime time.Time | |
| } | |
| type RuleEngineRequest struct { | |
| CurrentTime time.Time | |
| } |
🤖 Prompt for AI Agents
In x/drop/rules/engine.go around lines 10 to 12, the RuleEngineRequest struct
uses an unexported field currentTime which prevents construction from other
packages; rename the field to CurrentTime (export it) so external callers can
set it, update any constructors/tests/usages in this package to use the new
exported name, and run `go vet`/`go test` to catch and fix remaining references.
| } else { | ||
| err := engine.Register(&EndTimeRule{ | ||
| EndTime: rule.StartTime.Add(7 * 24 * time.Hour), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to register default end time rule: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: default EndTime applied even when StartTime is zero.
If both StartTime and EndTime are zero, EndTime becomes year 0001 + 7d, making all validations fail. Only apply the default when StartTime is set.
- } else {
- err := engine.Register(&EndTimeRule{
- EndTime: rule.StartTime.Add(7 * 24 * time.Hour),
- })
+ } else if !rule.StartTime.IsZero() {
+ err := engine.Register(&EndTimeRule{
+ EndTime: rule.StartTime.Add(7 * 24 * time.Hour),
+ })
if err != nil {
return nil, fmt.Errorf("failed to register default end time rule: %w", err)
}
}📝 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.
| } else { | |
| err := engine.Register(&EndTimeRule{ | |
| EndTime: rule.StartTime.Add(7 * 24 * time.Hour), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to register default end time rule: %w", err) | |
| } | |
| } | |
| } else if !rule.StartTime.IsZero() { | |
| err := engine.Register(&EndTimeRule{ | |
| EndTime: rule.StartTime.Add(7 * 24 * time.Hour), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to register default end time rule: %w", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In x/drop/rules/engine.go around lines 68 to 75, the code unconditionally
registers a default EndTime based on rule.StartTime which causes a bogus EndTime
when StartTime is zero; change the logic to only register the default EndTime
when EndTime is zero AND StartTime is non-zero (e.g. check rule.EndTime.IsZero()
&& !rule.StartTime.IsZero()), otherwise skip registering the default; keep the
existing error handling after the Register call.
| func (r *StartTimeRule) Validate(req *RuleEngineRequest) error { | ||
| if req.currentTime.Before(r.StartTime) { | ||
| return fmt.Errorf("current time %s is before start time %s", req.currentTime, r.StartTime) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exported request field; optional: format times.
Mirror EndTimeRule change for consistency.
-func (r *StartTimeRule) Validate(req *RuleEngineRequest) error {
- if req.currentTime.Before(r.StartTime) {
- return fmt.Errorf("current time %s is before start time %s", req.currentTime, r.StartTime)
+func (r *StartTimeRule) Validate(req *RuleEngineRequest) error {
+ if req.CurrentTime.Before(r.StartTime) {
+ return fmt.Errorf("current time %s is before start time %s", req.CurrentTime.Format(time.RFC3339), r.StartTime.Format(time.RFC3339))
}
return nil
}📝 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.
| func (r *StartTimeRule) Validate(req *RuleEngineRequest) error { | |
| if req.currentTime.Before(r.StartTime) { | |
| return fmt.Errorf("current time %s is before start time %s", req.currentTime, r.StartTime) | |
| } | |
| return nil | |
| func (r *StartTimeRule) Validate(req *RuleEngineRequest) error { | |
| if req.CurrentTime.Before(r.StartTime) { | |
| return fmt.Errorf( | |
| "current time %s is before start time %s", | |
| req.CurrentTime.Format(time.RFC3339), | |
| r.StartTime.Format(time.RFC3339), | |
| ) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In x/drop/rules/start_time.go around lines 16 to 21, the code uses the
unexported field req.currentTime and prints raw time values; change to use the
exported field req.CurrentTime and format both times in the error message (e.g.,
using t.Format(time.RFC3339) or a consistent formatter used by EndTimeRule) so
the error mirrors the EndTimeRule style and provides readable timestamps.
| MaxRulesPerDrop = 5 | ||
| MaxRuleIDLength = 20 | ||
| ) | ||
|
|
||
| var ( | ||
| DropsPrefix = collections.NewPrefix(0) | ||
| RulesPrefix = collections.NewPrefix(1) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align Rule ID type across proto, storage keys, and Go types.
MaxRuleIDLength implies string IDs, but proto Rule.id is uint64 and Rules map uses string key. Pick one (string or uint64) and apply consistently across:
- proto (Rule.id)
- keeper storage map key type
- validations (use MaxRuleIDLength only if string)
🏁 Script executed:
#!/bin/bash
# Locate the Go keys file and display its top lines
file=$(fd -e keys.go)
echo "Found keys file at: $file"
sed -n '1,50p' "$file"
echo; echo "---- Proto definitions for Rule ----"
rg -n "message Rule" -g "*.proto" -C3
echo; echo "---- Check Rule.id field in protos ----"
rg -n "Rule.id" -g "*.proto" -C3Length of output: 530
🏁 Script executed:
#!/bin/bash
# Inspect the keys file for rule ID and prefix definitions
if [ -f x/drop/types/keys.go ]; then
echo "---- x/drop/types/keys.go ----"
sed -n '1,50p' x/drop/types/keys.go
else
echo "x/drop/types/keys.go not found"
fi
echo
# Locate all occurrences of MaxRuleIDLength across the repo
rg -n "MaxRuleIDLength" -C2
echo
# Find where RulesPrefix is used in the drop module
rg -n "RulesPrefix" -g "x/drop/**/*.go" -C3
echo
# Check how collections.NewPrefix is invoked
rg -n "collections.NewPrefix" -C2
echo
# Inspect keeper code to see how rule IDs are serialized into storage keys
rg -n "RulesPrefix.Append" -g "x/drop/keeper/**/*.go" -C3Length of output: 1810
Align Rule ID type across proto, storage keys, and Go types.
In proto, Rule.id is uint64; the keeper’s rules map and MaxRuleIDLength assume a string ID. Update accordingly:
- x/drop/types/keys.go: remove MaxRuleIDLength (or repurpose for a string-ID fallback)
- x/drop/keeper/keeper.go: change
collections.PairKeyCodec(collections.StringKey, collections.StringKey)
to
collections.PairKeyCodec(collections.StringKey, collections.Uint64Key)
🤖 Prompt for AI Agents
In x/drop/types/keys.go around lines 10-17, the MaxRuleIDLength constant no
longer matches the proto definition where Rule.id is a uint64; remove
MaxRuleIDLength (or clearly mark it deprecated/repurposed for any string-ID
fallback) and keep MaxRulesPerDrop and the prefix vars as-is. Then update
x/drop/keeper/keeper.go to use a uint64 key codec for the rule map by replacing
collections.PairKeyCodec(collections.StringKey, collections.StringKey) with
collections.PairKeyCodec(collections.StringKey, collections.Uint64Key) so
storage keys align with the proto Rule.id type and avoid string-length
assumptions.
TODO
authoritywho can edit metadata (name, description and uri), if authority is null, no one can edit the token data, authority could be a wallet, module account or a cw-contractseller_feepayment_addressor[]creators???max_supply, mean how many tokens can be printed from the master edition? If 0, only master will existsminterthe address authorized to mint, if minter is null mean the token cannot be minted anymore, minter could be wallet, a module account or a cw-contractwrapcommandunwrapcommandMsgs
MsgBurnNftMsgBurnEditionMsgSetMinterMsgSetAuthorityQueries
ownerOfnumTokenscollectionInfonftInfoallNftInfo- returns the result of bothNftInfoandOwnerOfAllNftsByOwnerAllCollectionsSummary by CodeRabbit