Validate that FT and NFT features are in the defined set

[wojtek-coreum]

This change is 

[dzmitryhil]

Reviewable status: 0 of 22 files reviewed, 5 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

integration-tests/upgrade/ft.go line 581 at r2 (raw file):

		Description:   "AAA Description",
		InitialAmount: sdk.NewInt(1000),
		Features: []assetfttypes.Feature{

What about testing the duplicated features, they should be removed as well? Same comment for the NFT.

---

x/asset/ft/keeper/keeper.go line 97 at r2 (raw file):

// IterateAllDefinitions iterates over all token definitions and applies the provided callback.
// If true is returned from the callback, iteration is halted.
func (k Keeper) IterateAllDefinitions(ctx sdk.Context, cb func(types.Definition) (bool, error)) error {

Why do you need to update the interface of the method, you can achieve what you need with the previous version as well. (same for the NFT)

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

// MigrateFeatures migrates asset ft features state from v1 to v2.
// It removes features which are outside the allowed scope.
func MigrateFeatures(ctx sdk.Context, keeper FTKeeper, allowedFeatures map[int32]string) error {

As far as I remember we wanted to remove duplicated features as well, since it was possible to do it before.

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

// MigrateFeatures migrates asset ft features state from v1 to v2.
// It removes features which are outside the allowed scope.
func MigrateFeatures(ctx sdk.Context, keeper FTKeeper, allowedFeatures map[int32]string) error {

The input is allowedFeatures map[int32]string but that isn't true since you filter the IBC later. IMO better to define the list here without using it as a function param. (same for the NFT)

---

x/asset/ft/types/token.go line 186 at r2 (raw file):

// ValidateFeatures verifies that provided features belong to the defined set.
func ValidateFeatures(features []Feature) error {

Unit tests? (same for the NFT)

[wojtek-coreum]

Reviewable status: 0 of 22 files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

x/asset/ft/keeper/keeper.go line 97 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need to update the interface of the method, you can achieve what you need with the previous version as well. (same for the NFT)

I need to be able to return the error from the cb

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

As far as I remember we wanted to remove duplicated features as well, since it was possible to do it before.

Good point, completely missed that!

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The input is allowedFeatures map[int32]string but that isn't true since you filter the IBC later. IMO better to define the list here without using it as a function param. (same for the NFT)

I need to define it as a param to be able to write unit test. After the fix, I'm able to create the token only with the valid set of features. So to test that invalid features are filtered out, I trick the list of allowed features passed to the MigrateFeatures.

[wojtek-coreum]

Reviewable status: 0 of 24 files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

integration-tests/upgrade/ft.go line 581 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about testing the duplicated features, they should be removed as well? Same comment for the NFT.

Done.

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Good point, completely missed that!

Done

---

x/asset/ft/types/token.go line 186 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Unit tests? (same for the NFT)

Done.

[dzmitryhil]

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

---

x/asset/ft/keeper/keeper.go line 97 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I need to be able to return the error from the cb

Why do you need this? You can do VIA error assigning, but in general I don't see a reason to return it.

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I need to define it as a param to be able to write unit test. After the fix, I'm able to create the token only with the valid set of features. So to test that invalid features are filtered out, I trick the list of allowed features passed to the MigrateFeatures.

In that case list all allowed feature as inmut without the filtration of the IBC in the func.

[dzmitryhil]

Reviewed 11 of 22 files at r1, 13 of 14 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

[wojtek-coreum]

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

---

x/asset/ft/keeper/keeper.go line 97 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need this? You can do VIA error assigning, but in general I don't see a reason to return it.

doing it via assigning is a weird hack when no proper interface is available

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

In that case list all allowed feature as inmut without the filtration of the IBC in the func.

I've redone it

[dzmitryhil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I've redone it

The if f == types.Feature_ibc { still looks redundant. If you know that you support only the list of features, better to define that list, instead of filtering the ibc;

[dzmitryhil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @ysv)

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The if f == types.Feature_ibc { still looks redundant. If you know that you support only the list of features, better to define that list, instead of filtering the ibc;

@ysv @miladz68 WDYT ?

[wojtek-coreum]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @ysv)

---

x/asset/ft/legacy/v1/features.go line 17 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68 WDYT ?

Done.

[dzmitryhil]

Reviewable status: 23 of 25 files reviewed, all discussions resolved (waiting on @miladz68, @silverspase, and @ysv)

[miladz68]

Reviewed 10 of 22 files at r1, 12 of 14 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

[miladz68]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

[dzmitryhil]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)