Implement class freeze for NFTs

[miladz68]

Description

Reviewers checklist:

• Try to write more meaningful comments with clear actions to be taken.
• Nit-picking should be unblocking. Focus on core issues.

Authors checklist

• Provide a concise and meaningful description
• Review the code yourself first, before making the PR.
• Annotate your PR in places that require explanation.
• Think and try to split the PR to smaller PR if it is big.

---

This change is 

[wojtek-coreum]

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

a discussion (no related file):
Stuff I did for class whitelisting + minting here: #675
must be done in this PR for class freezing + minting

---

a discussion (no related file):
What about freezing class for everyone (like global freeze in FT)?

---

---

x/asset/nft/client/cli/tx_test.go line 315 at r1 (raw file):

	args = []string{classID, nftID}
	requireT.NoError(coreumclitestutil.ExecQueryCmd(ctx, cli.CmdQueryFrozen(), args, &frozenResp))
	requireT.False(frozenResp.Frozen)

This is interesting case. Maybe when someone queries for specific nft, we should return true if its class is frozen?

---

x/asset/nft/keeper/keeper.go line 522 at r1 (raw file):

		return err
	}
	s := ctx.KVStore(k.storeKey)

I think we use this pattern in many places. Maybe we could implement booleanStore?

---

x/asset/nft/keeper/keeper.go line 551 at r1 (raw file):

	}

	if bytes.Equal(ctx.KVStore(k.storeKey).Get(key), types.StoreTrue) {

Another candidate to implement in booleanStore

---

x/asset/nft/keeper/keeper.go line 924 at r1 (raw file):

	}

	frozen, err := k.IsFrozen(ctx, classID, nftID)

non-blocking: isFrozen

---

x/asset/nft/keeper/keeper.go line 935 at r1 (raw file):

	}

	classFrozen, err := k.IsClassFrozen(ctx, classID, owner)

non-blocking: isClassFrozen

---

x/asset/nft/keeper/keeper.go line 1034 at r1 (raw file):

	}

	if classDefinition.Issuer == account.String() {

Could it be moved to classDeifnition.IsIssuer like we did in FTs?

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

// MsgKeeper defines subscope of keeper methods required by msg service.
//
//nolint:interfacebloat // We accept the fact that this interface declares more than 10 methods.

This is the third time I see this nolint during past week. Maybe it's time to disble that linter?

---

x/asset/nft/types/keys.go line 109 at r1 (raw file):

	}
	if len(parsedKeys) != 2 {
		err = sdkerrors.Wrapf(ErrInvalidKey, "key must be composed to 2 length prefixed keys")

composed of?

[dzmitryhil]

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @miladz68 and @ysv)

[miladz68]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

Stuff I did for class whitelisting + minting here: #675
must be done in this PR for class freezing + minting

I don't understand which parts you are reffering to, maybe we can have a quick call and discuss.

---

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

What about freezing class for everyone (like global freeze in FT)?

we have not discussed that feature, we can add it after we discuss it with reza.

---

---

x/asset/nft/client/cli/tx_test.go line 315 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

This is interesting case. Maybe when someone queries for specific nft, we should return true if its class is frozen?

but return true if class is frozen for who exactly ? right now, we return true if the owner of the NFT is class-frozen, which I think is the correct way.

---

x/asset/nft/keeper/keeper.go line 522 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I think we use this pattern in many places. Maybe we could implement booleanStore?

I looked at it and the abstraction saves a line of code. I don't think it is worth to introduce that abstraction.
@ysv @dzmitryhil wdyt

---

x/asset/nft/keeper/keeper.go line 551 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Another candidate to implement in booleanStore

discussed above.

---

x/asset/nft/keeper/keeper.go line 924 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

non-blocking: isFrozen

we expose this function to query server

---

x/asset/nft/keeper/keeper.go line 935 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

non-blocking: isClassFrozen

we expose this function to query server

---

x/asset/nft/keeper/keeper.go line 1034 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Could it be moved to classDeifnition.IsIssuer like we did in FTs?

Done.

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

This is the third time I see this nolint during past week. Maybe it's time to disble that linter?

I don't think we should disable it.

---

x/asset/nft/types/keys.go line 109 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

composed of?

Done.

[wojtek-coreum]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

a discussion (no related file):

Previously, miladz68 (milad) wrote…

I don't understand which parts you are reffering to, maybe we can have a quick call and discuss.

When new NFT is minted and sent to recipient other than issuer, and class is frozen for that recipient, it means recipient won't be able to send it. We may say that this is ok or we might say that it should be prohibited.

---

a discussion (no related file):

Previously, miladz68 (milad) wrote…

we have not discussed that feature, we can add it after we discuss it with reza.

let's discuss during daily

---

---

x/asset/nft/client/cli/tx_test.go line 315 at r1 (raw file):

Previously, miladz68 (milad) wrote…

but return true if class is frozen for who exactly ? right now, we return true if the owner of the NFT is class-frozen, which I think is the correct way.

Ah I see now that this query is just for class. So if class is frozen for an account and we query for freeze status of the token for an account (providing class id, nft id and address) it will return true?

---

x/asset/nft/keeper/keeper.go line 522 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I looked at it and the abstraction saves a line of code. I don't think it is worth to introduce that abstraction.
@ysv @dzmitryhil wdyt

I'm saying that we could have sth like this:

s := newBooleanStore(ctx.KVStore(k.storeKey))
s.Set(key, boolValue)
s.IsTrue(key)
s.IsFalse(key)

---

x/asset/nft/keeper/keeper.go line 924 at r1 (raw file):

Previously, miladz68 (milad) wrote…

we expose this function to query server

I mean variable name

---

x/asset/nft/keeper/keeper.go line 935 at r1 (raw file):

Previously, miladz68 (milad) wrote…

we expose this function to query server

I mean variable name

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I don't think we should disable it.

@ysv @dzmitryhil

[dzmitryhil]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

x/asset/nft/keeper/keeper.go line 522 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I'm saying that we could have sth like this:

s := newBooleanStore(ctx.KVStore(k.storeKey))
s.Set(key, boolValue)
s.IsTrue(key)
s.IsFalse(key)

I don't see big benifits from it.

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

@ysv @dzmitryhil

I don't have strong opining here, but that's the same discussion we had before, where we decide to use nolint.

[dzmitryhil]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

[miladz68]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @wojtek-coreum and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

When new NFT is minted and sent to recipient other than issuer, and class is frozen for that recipient, it means recipient won't be able to send it. We may say that this is ok or we might say that it should be prohibited.

prohibiting sending to recipient is related to whitelisting, I see no reason to prohibit it in case of freezing.

---

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

let's discuss during daily

Sure, let's discuss.

---

---

x/asset/nft/client/cli/tx_test.go line 315 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Ah I see now that this query is just for class. So if class is frozen for an account and we query for freeze status of the token for an account (providing class id, nft id and address) it will return true?

that query does not exist, you either query for NFT freeze (class-id nft-id) or class-freeze (class-id account).

---

x/asset/nft/keeper/keeper.go line 522 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I don't see big benifits from it.

I agree with Dima, I see no tangible benefits.

---

x/asset/nft/keeper/keeper.go line 924 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I mean variable name

Done

---

x/asset/nft/keeper/keeper.go line 935 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I mean variable name

Done

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I don't have strong opining here, but that's the same discussion we had before, where we decide to use nolint.

I agree with Dima, it is exactly the same discussion as we had before, nothing has changed since then.

[wojtek-coreum]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @ysv)

a discussion (no related file):

Previously, miladz68 (milad) wrote…

Sure, let's discuss.

Changed to non-blocking as it might be implemented separately. But should be discussed.

---

---

x/asset/nft/keeper/msg_server.go line 16 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I agree with Dima, it is exactly the same discussion as we had before, nothing has changed since then.

My point is that so far we have never decided to split the interface after seeing this issue. That's why my thesis is that this linter is useless. Changed to non-blocking

[wojtek-coreum]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ysv)

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ysv)