Implemented the whitelisting for the entire class #672

miladz68 · 2023-10-13T09:56:05Z

Description

This PR provides the functionality to whitelist an entire class, instead of every nft individually.

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

ysv

Reviewed 25 of 25 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

docs/api.md line 73 at r1 (raw file):

  
- [coreum/asset/nft/v1/event.proto](#coreum/asset/nft/v1/event.proto)
    - [EventAddedToClassWhitelist](#coreum.asset.nft.v1.EventAddedToClassWhitelist)

I had a few comments about renaming but decided to group all them here

It depends on how we word it either: ClassWhitelist or ClassAdded/removedFromWhitelist everywhere.
Maybe lets discuss one more time

ideas for renaming:
EventAddedToClassWhitelist -> EventNFTClassAddedToWhitelist or EventClassAddedToWhitelist ?
MsgAddToClassWhitelist -> MsgAddClassToWhitelist ?

docs/api.md line 83 at r1 (raw file):

- [coreum/asset/nft/v1/genesis.proto](#coreum/asset/nft/v1/genesis.proto)
    - [BurntNFT](#coreum.asset.nft.v1.BurntNFT)
    - [ClassWhitelistedAccounts](#coreum.asset.nft.v1.ClassWhitelistedAccounts)

what about:
WhitelistedNFTClassAccounts

to follow existing even name logic: (WhitelistedNFTAccounts)

integration-tests/modules/assetnft_test.go line 1500 at r1 (raw file):

// TestAssetNFTClassWhitelist tests nft class whitelisting.
func TestAssetNFTClassWhitelist(t *testing.T) {

good test cases
I would also add a few where you try both class & single NFT whitelisting and check how they interact together (here or inside a separate func)

e.g:
1 add both class & nft whitelist -> OK
2 remove nft whitelist (keep class) -> OK
3 remove class whitelist (keep nft) -> OK for this NFT not ok for other

IDK if it makes sense

integration-tests/modules/assetnft_test.go line 1736 at r1 (raw file):

	// assert the unwhitelisting event
	unWhitelistedEvents, err := event.FindTypedEvents[*assetnfttypes.EventRemovedFromClassWhitelist](res.Events)

nit: unwhitelisted is a single word

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

}

func (k Keeper) addToWhitelistOrRemoveFromWhitelistClass(ctx sdk.Context, classID string, sender, account sdk.AccAddress, setWhitelisted bool) error {

addOrRemoveClassFromWhitelist addOrRemoveFromClassWhitelist ?

x/asset/nft/types/msgs.go line 29 at r1 (raw file):

)

type msgAndLegacyMsg interface {

nice 👍

could also be sdkAndLegacyMsg

miladz68

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

docs/api.md line 73 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I had a few comments about renaming but decided to group all them here

It depends on how we word it either: ClassWhitelist or ClassAdded/removedFromWhitelist everywhere.
Maybe lets discuss one more time

ideas for renaming:
EventAddedToClassWhitelist -> EventNFTClassAddedToWhitelist or EventClassAddedToWhitelist ?
MsgAddToClassWhitelist -> MsgAddClassToWhitelist ?

well my thinking is that we have 2 concepts, whitelist and classWhitelist. Otherwise we will add the class word at the begining and sometime at the end, which will be confusing.

docs/api.md line 83 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

what about:
WhitelistedNFTClassAccounts

to follow existing even name logic: (WhitelistedNFTAccounts)

let's discuss

integration-tests/modules/assetnft_test.go line 1500 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

good test cases
I would also add a few where you try both class & single NFT whitelisting and check how they interact together (here or inside a separate func)

e.g:
1 add both class & nft whitelist -> OK
2 remove nft whitelist (keep class) -> OK
3 remove class whitelist (keep nft) -> OK for this NFT not ok for other

IDK if it makes sense

Added test to keeper_test.go

integration-tests/modules/assetnft_test.go line 1736 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: unwhitelisted is a single word

Fixed everywhere

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

Previously, ysv (Yaroslav Savchuk) wrote…

addOrRemoveClassFromWhitelist addOrRemoveFromClassWhitelist ?

let's disccuss in daily

x/asset/nft/types/msgs.go line 29 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nice 👍

could also be sdkAndLegacyMsg

both Msg and LegacyMsg are part of the sdk, so it would be sdkMsgAndSdkLegacyMsg

miladz68

Reviewable status: 26 of 28 files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

docs/api.md line 73 at r1 (raw file):

Previously, miladz68 (milad) wrote…

well my thinking is that we have 2 concepts, whitelist and classWhitelist. Otherwise we will add the class word at the begining and sometime at the end, which will be confusing.

discussed and decided to keep as is.

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

Previously, miladz68 (milad) wrote…

let's disccuss in daily

decided on addOrRemoveFromClassWhitelist

dzmitryhil

Reviewable status: 26 of 28 files reviewed, 9 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

}

message ClassWhitelistedAccounts {

You use two name for the same - ClassWhitelistedAccounts and WhitelistedAccountsForClass, let's take just on and use it everywhere.

x/asset/nft/client/cli/query.go line 218 at r3 (raw file):

Example:
$ %s query %s whitelisted-accounts [class-id] [id]

Breaking change ? I'm ok with it but we need to add to the change-log for the upgrade.

x/asset/nft/client/cli/query.go line 259 at r3 (raw file):

		Use:   "class-whitelisted-accounts [class-id]",
		Args:  cobra.ExactArgs(1),
		Short: "Query for the list whitelisted accounts for a class of non-fungible tokens",

list whitelisted -> list of whitelisted ? Or use same as in Long.

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

	}

	if bytes.Equal(ctx.KVStore(k.storeKey).Get(classKey), asset.StoreTrue) {

I know that it is not your change, but let's re-think the usage of the helper.go in the root. It looks really ugly.
@miladz68 @ysv @wojtek-coreum .

dzmitryhil

Reviewable status: 26 of 28 files reviewed, 10 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

x/deterministicgas/spec/README.md line 84 at r3 (raw file):

| `/coreum.asset.ft.v1.MsgUnfreeze`                                      | 8500                           |
| `/coreum.asset.ft.v1.MsgUpgradeTokenV1`                                | 25000                          |
| `/coreum.asset.nft.v1.MsgAddToClassWhitelist`                          | 7000                           |

Did you compute that or just took from the nearest value?

dzmitryhil

Reviewable status: 26 of 28 files reviewed, 10 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

x/deterministicgas/spec/README.md line 84 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Did you compute that or just took from the nearest value?

Same for the second message

ysv

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You use two name for the same - ClassWhitelistedAccounts and WhitelistedAccountsForClass, let's take just on and use it everywhere.

agree

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I know that it is not your change, but let's re-think the usage of the helper.go in the root. It looks really ugly.
@miladz68 @ysv @wojtek-coreum .

Could you be more specific ?

miladz68

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

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

agree

We have whitelisting and class-whitelisting so with that logic we should name it ClassWhitelistedNFTAccounts but the NFT is redundant. It is redundant in the previous naming as well, and we

x/asset/nft/client/cli/query.go line 218 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Breaking change ? I'm ok with it but we need to add to the change-log for the upgrade.

it is not a breaking change, the description was incorrect so I fixed it.

x/asset/nft/client/cli/query.go line 259 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

list whitelisted -> list of whitelisted ? Or use same as in Long.

removed the list to match with description.

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Could you be more specific ?

which helper? you mean bytes.Equal ?

x/deterministicgas/spec/README.md line 84 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Same for the second message

I tool the value from the integration test.

miladz68

Reviewable status: 21 of 28 files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

Previously, miladz68 (milad) wrote…

We have whitelisting and class-whitelisting so with that logic we should name it ClassWhitelistedNFTAccounts but the NFT is redundant. It is redundant in the previous naming as well, and we

Actually I read your comment again and I understand it better now. I fixed the query name in proto.

wojtek-coreum

Reviewed 14 of 25 files at r1, 5 of 6 files at r2, 2 of 2 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dzmitryhil and @miladz68)

integration-tests/modules/assetnft_test.go line 1519 at r4 (raw file):

			&assetnfttypes.MsgAddToClassWhitelist{},
			&assetnfttypes.MsgAddToClassWhitelist{},
			&assetnfttypes.MsgRemoveFromWhitelist{},

It should be MsgRemoveFromClassWhitelist

miladz68

Reviewable status: 26 of 28 files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

integration-tests/modules/assetnft_test.go line 1519 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

It should be MsgRemoveFromClassWhitelist

Done.

wojtek-coreum

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dzmitryhil and @miladz68)

wojtek-coreum

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil)

dzmitryhil

Reviewable status: 25 of 28 files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

Previously, miladz68 (milad) wrote…

Actually I read your comment again and I understand it better now. I fixed the query name in proto.

Seach by WhitelistedAccountsForClass, you still use it in the method names.

x/asset/nft/client/cli/query.go line 218 at r3 (raw file):

Previously, miladz68 (milad) wrote…

it is not a breaking change, the description was incorrect so I fixed it.

Got you.

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, miladz68 (milad) wrote…

which helper? you mean bytes.Equal ?

asset.StoreTrue - in the assets root heler file with one constant.

dzmitryhil

Reviewed 13 of 25 files at r1, 3 of 6 files at r2, 1 of 2 files at r3, 7 of 7 files at r4, 1 of 2 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @ysv)

miladz68

Reviewable status: 24 of 28 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

proto/coreum/asset/nft/v1/genesis.proto line 34 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Seach by WhitelistedAccountsForClass, you still use it in the method names.

Done.

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

asset.StoreTrue - in the assets root heler file with one constant.

let's discuss it with yaroslav.

ysv

Reviewed 6 of 7 files at r4, 1 of 2 files at r5, 2 of 3 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil)

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, miladz68 (milad) wrote…

let's discuss it with yaroslav.

will discuss after daily

wojtek-coreum

Reviewed 2 of 3 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil)

miladz68

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil)

x/asset/nft/keeper/keeper.go line 582 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

will discuss after daily

Done.

dzmitryhil

Reviewed 3 of 4 files at r7, 5 of 5 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

ysv

Reviewed 5 of 5 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

Implemented the whitelisting for the entire class

122ad5e

miladz68 requested a review from a team as a code owner October 13, 2023 09:56

miladz68 requested review from dzmitryhil, ysv and wojtek-coreum and removed request for a team October 13, 2023 09:56

pass linter

fd25664

ysv requested changes Oct 13, 2023

View reviewed changes

miladz68 commented Oct 13, 2023

View reviewed changes

miladz68 added 2 commits October 13, 2023 16:34

added tests

5e2b405

pass linter

1a78f0a

miladz68 commented Oct 13, 2023

View reviewed changes

dzmitryhil requested changes Oct 16, 2023

View reviewed changes

ysv requested a review from dzmitryhil October 16, 2023 10:06

ysv previously approved these changes Oct 16, 2023

View reviewed changes

fixed query names

33ae24d

miladz68 commented Oct 16, 2023

View reviewed changes

miladz68 dismissed ysv’s stale review via 33ae24d October 16, 2023 11:12

miladz68 commented Oct 16, 2023

View reviewed changes

wojtek-coreum requested changes Oct 16, 2023

View reviewed changes

miladz68 added 2 commits October 16, 2023 15:37

addressed Pr comment

a7cc5ea

pass test build

2872b4c

miladz68 commented Oct 16, 2023

View reviewed changes

wojtek-coreum reviewed Oct 16, 2023

View reviewed changes

wojtek-coreum previously approved these changes Oct 16, 2023

View reviewed changes

Merge branch 'master' into milad/nft-class-whitelist

b691a72

dzmitryhil requested changes Oct 16, 2023

View reviewed changes

dzmitryhil reviewed Oct 16, 2023

View reviewed changes

addressed PR comments

e7acb0e

miladz68 dismissed wojtek-coreum’s stale review via e7acb0e October 16, 2023 14:37

miladz68 commented Oct 16, 2023

View reviewed changes

ysv requested a review from dzmitryhil October 16, 2023 15:44

ysv previously approved these changes Oct 16, 2023

View reviewed changes

wojtek-coreum previously approved these changes Oct 17, 2023

View reviewed changes

move store.True from asset to asset/ft and asset/nft

455a028

miladz68 dismissed stale reviews from wojtek-coreum and ysv via 455a028 October 17, 2023 10:20

miladz68 commented Oct 17, 2023

View reviewed changes

dzmitryhil approved these changes Oct 17, 2023

View reviewed changes

ysv approved these changes Oct 17, 2023

View reviewed changes

miladz68 merged commit 8dd7f4c into master Oct 17, 2023
8 checks passed

miladz68 deleted the milad/nft-class-whitelist branch October 17, 2023 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented the whitelisting for the entire class #672

Implemented the whitelisting for the entire class #672

miladz68 commented Oct 13, 2023 •

edited by CoreumDevelopment

Loading

ysv left a comment

miladz68 left a comment

miladz68 left a comment

dzmitryhil left a comment

dzmitryhil left a comment

dzmitryhil left a comment

ysv left a comment

miladz68 left a comment

miladz68 left a comment

wojtek-coreum left a comment

miladz68 left a comment

wojtek-coreum left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

dzmitryhil left a comment

miladz68 left a comment

ysv left a comment

wojtek-coreum left a comment

miladz68 left a comment

dzmitryhil left a comment

ysv left a comment

Implemented the whitelisting for the entire class #672

Implemented the whitelisting for the entire class #672

Conversation

miladz68 commented Oct 13, 2023 • edited by CoreumDevelopment Loading

Description

Reviewers checklist:

Authors checklist

ysv left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

miladz68 commented Oct 13, 2023 •

edited by CoreumDevelopment

Loading