feat: add batch operation for x/nft module

[dreamer-zq]

Description

Closes: #XXXX

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[codecov]

Codecov Report

Merging #12187 (dd666e8) into main (4c3b7af) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main   #12187      +/-   ##
==========================================
+ Coverage   66.10%   66.12%   +0.01%     
==========================================
  Files         673      674       +1     
  Lines       71097    71155      +58     
==========================================
+ Hits        46999    47051      +52     
- Misses      21458    21462       +4     
- Partials     2640     2642       +2     

Impacted Files	Coverage Δ
x/nft/keeper/nft_batch.go	87.50% <87.50%> (ø)
x/nft/keeper/nft.go	83.56% <100.00%> (+1.20%)	⬆️

[julienrbrt]

I think most of these should use the Mint, Burn, Update, Transfer from the keeper instead of repeating logic. Did you have a reason not to?

[dreamer-zq]

I think most of these should use the Mint, Burn, Update, Transfer from the keeper instead of repeating logic. Did you have a reason not to?

This is indeed the case. I do this now because some logic is repeated, such as class verification, which can reduce gas. Or add some xxxxWithNoCheck (for example: MintWithNoCheck, BatchMintWithNoCheck) methods, the verification logic is implemented by the upper layer, what do you think?

[julienrbrt]

This is indeed the case. I do this now because some logic is repeated, such as class verification, which can reduce gas. Or add some xxxxWithNoCheck (for example: MintWithNoCheck, BatchMintWithNoCheck) methods, the verification logic is implemented by the upper layer, what do you think?

Alright, but the class verification logic is already in Mint, so if you remove it from BatchMint that shouldn't be an issue, right?

The following f.e. has no duplicate class verification, then.

// BatchMint defines a method for minting a batch of nfts
func (k Keeper) BatchMint(ctx sdk.Context,
	tokens []nft.NFT,
	receiver sdk.AccAddress,
) error {
	classIDs := make(map[string]bool, len(tokens))
	for _, token := range tokens {
		if !classIDs[token.ClassId] {
			return sdkerrors.Wrap(nft.ErrClassNotExists, token.ClassId)
		}

		if err := k.Mint(ctx, token, receiver); err != nil {
			return err
		}

		classIDs[token.ClassId] = true
	}
	return nil
}

[julienrbrt]

lgtm!

I see now what you've meant about the repeating checks, these happen for methods with classID as argument (BatchTransfer and BatchBurn). Maybe a transferWithNoCheck and burnWithNoCheck used in Transfer, BatchTransfer, Burn, BatchBurn do indeed make sense for these, not sure 🤷🏾‍♂️

[dreamer-zq]

lgtm!

I see now what you've meant about the repeating checks, these happen for methods with classID as argument (BatchTransfer and BatchBurn). Maybe a transferWithNoCheck and burnWithNoCheck used in Transfer, BatchTransfer, Burn, BatchBurn do indeed make sense for these, not sure 🤷🏾‍♂️

Yes, I want to separate the verification logic, and let the upper application decide how to verify (transferWithNoCheck) or use Transfer directly (sdk is responsible for verification). This way the user has more flexibility, now do you think we should add the noCheck method to these methods? If you agree to this, I can continue to modify these

[julienrbrt]

Thanks @dreamer-zq, one last thing, can we add a changelog? :)

[julienrbrt]

Alright, because of #12233 your tests are failing 🙉 can you please fix them?
➡️ s.app.NFTKeeper to s.nftKeeper should do it.

[tac0turtle]

This needs a spec update as well. please update docs prior to merging next time

[julienrbrt]

Sorry, I forgot to check that. @dreamer-zq Could you submit a follow-up PR?

[dreamer-zq]

This needs a spec update as well. please update docs prior to merging next time

Ok, Is it the documentation in x/nft/spec ? This change does not modify the relevant state, msg, or event. So what exactly should be changed? Is it adr-043-nft-module ?

[dreamer-zq]

Sorry, I forgot to check that. @dreamer-zq Could you submit a follow-up PR?
12293