Wasm asset NFT / native NFT integration

[dzmitryhil]

This change is 

[wojtek-coreum]

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

a discussion (no related file):
I remember that in the past we had a wasm contract which intentionally sent two submessages from a single call to test that it works. Now I see that we no longer have that test. Do you remember what happened?

---

---

integration-tests/modules/wasm_test.go line 155 at r1 (raw file):

const (
	// tx.
	nonFungibleTokenMethodMint                nonFungibleTokenMethod = "mint"

nftMethodMint

---

integration-tests/modules/wasm_test.go line 173 at r1 (raw file):

//nolint:tagliatelle
type nonFungibleTokenClass struct {

nftClass

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 1 at r1 (raw file):

use coreum_wasm_sdk::assetnft::{

instead of importing everything like here with aliases I would prefer to have just this:

use coreum_wasm_sdk::assetnft;

and then e.g.:

assetnft::Msg::IssueClass {
    // ...
}

This is what namespaces are for, and by creating aliases for all the imported types you reinvent the wheel here.

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 124 at r1 (raw file):

    Owner { id: String },
    Supply {},
    Nft { id: String }, // we use Nft not NFT since NFT is decoded as n_f_t

NFT

---

integration-tests/modules/testdata/wasm/sdk/src/assetnft.rs line 14 at r1 (raw file):

    pub uri: Option<String>,
    pub uri_hash: Option<String>,
    pub data: Option<String>,

Wouldn't Bytes be a better choice than String?

---

integration-tests/modules/testdata/wasm/sdk/src/nft.rs line 11 at r1 (raw file):

    pub uri: Option<String>,
    pub uri_hash: Option<String>,
    pub data: Option<String>,

Wouldn't Bytes be a better choice than String?

---

x/wasm/handler/custom.go line 40 at r1 (raw file):

	URI         string                       `json:"uri"`
	URIHash     string                       `json:"uri_hash"`
	Data        string                       `json:"data"`

why not []byte?

---

x/wasm/handler/custom.go line 105 at r1 (raw file):

	URI         string                       `json:"uri"`
	URIHash     string                       `json:"uri_hash"`
	Data        string                       `json:"data"`

why not []byte?

---

x/wasm/handler/custom.go line 173 at r1 (raw file):

				return nil, err
			}
			msgs := make([]sdk.Msg, 0)

			if decodedMsg == nil {
				return nil, nil
			}

			if err := decodedMsg.ValidateBasic(); err != nil {
				return nil, errors.WithStack(err)
			}

			return []sdk.Msg{decodedMsg}, nil

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

}

func executeQuery[T, K any](

K is always a []byte

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

}

func executeQuery[T, K any](

Instead of having T wouldn't it be simpler to accept resp interface{} and pass it to marshaller?

[dzmitryhil]

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

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

I remember that in the past we had a wasm contract which intentionally sent two submessages from a single call to test that it works. Now I see that we no longer have that test. Do you remember what happened?

We wanted to test that the wasm supports that functionality, and it's supported. So since we no longer have a use case for it, I removed that part during the contract rebuild. But if you guys @ysv @miladz68 @wojtek-coreum consider it important I can add an additional method to check it.

---

---

integration-tests/modules/wasm_test.go line 155 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

nftMethodMint

Done.

Code quote:

nonFungibleTokenMethodMint

---

integration-tests/modules/wasm_test.go line 173 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

nftClass

Done.

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 1 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

instead of importing everything like here with aliases I would prefer to have just this:

use coreum_wasm_sdk::assetnft;

and then e.g.:

assetnft::Msg::IssueClass {
    // ...
}

This is what namespaces are for, and by creating aliases for all the imported types you reinvent the wheel here.

It's just a matter of taste. But I'm OK to use it in the style you proposed. Updated.

Code quote:

ClassResponse as AssetNFTClassResponse

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 124 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

NFT

I wrote the comment near the Nft check it, please.

---

integration-tests/modules/testdata/wasm/sdk/src/assetnft.rs line 14 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Wouldn't Bytes be a better choice than String?

will be discussed in the first thread

---

integration-tests/modules/testdata/wasm/sdk/src/nft.rs line 11 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Wouldn't Bytes be a better choice than String?

The Bytes is not a standard type. If you want to use it we need to find which to use std::io::Bytes or bytes::Bytes or something else. If you know which type fits here better let's use it.
@ysv @miladz68 what are your thoughts?

---

x/wasm/handler/custom.go line 40 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

why not []byte?

will be discussed in the first thread

---

x/wasm/handler/custom.go line 105 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

why not []byte?

will be discussed in the first thread

---

x/wasm/handler/custom.go line 173 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

			if decodedMsg == nil {
				return nil, nil
			}

			if err := decodedMsg.ValidateBasic(); err != nil {
				return nil, errors.WithStack(err)
			}

			return []sdk.Msg{decodedMsg}, nil

updated

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

K is always a []byte

Nope, the K is different in each func.

Code quote:

func executeQuery[T, K any](

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Instead of having T wouldn't it be simpler to accept resp interface{} and pass it to marshaller?

Nope, I use the T in the reqExecutor check one more time the usage of that func.

[ysv]

Wasm asset NFT + NFT and new design

1. name is not really clear
2. NFT is repeated twice

Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

We wanted to test that the wasm supports that functionality, and it's supported. So since we no longer have a use case for it, I removed that part during the contract rebuild. But if you guys @ysv @miladz68 @wojtek-coreum consider it important I can add an additional method to check it.

I prefer to keep it

---

---

integration-tests/modules/wasm_test.go line 39 at r1 (raw file):

	fungibleTokenWASM []byte
	//go:embed testdata/wasm/nft/artifacts/nft.wasm
	nonFungibleTokenWASM []byte

minor: I would prefer to use ftWASM & nftWASM here

---

integration-tests/modules/wasm_test.go line 155 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Done.

still see old version

---

integration-tests/modules/wasm_test.go line 173 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Done.

I agree everything should be renamed to ft/nft if possible

---

integration-tests/modules/testdata/wasm/sdk/src/core.rs line 1 at r1 (raw file):

use crate::{assetft, assetnft, nft};

minor: is this file named core because it is core for SDK or because of coreum ?
If this is related to coreum maybe coreum.rs ?

---

integration-tests/modules/testdata/wasm/sdk/src/nft.rs line 11 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The Bytes is not a standard type. If you want to use it we need to find which to use std::io::Bytes or bytes::Bytes or something else. If you know which type fits here better let's use it.
@ysv @miladz68 what are your thoughts?

Frankly speaking I'm not able to comment here

Probably bytes would be better but only if:

• we are sure which Bytes we should use
• it is common to use bytes in Rust

---

x/wasm/handler/custom.go line 1 at r1 (raw file):

package handler

I would probably prefer to split it into files because NFT & AssetNFT too similar & it is confusing

---

x/wasm/handler/custom.go line 34 at r1 (raw file):

//
//nolint:tagliatelle // we keep the name same as consume
type AssetNFTMsgIssueClass struct {

I would prefer to avoid usage of these types as much as possible

At least add comments to them

---

x/wasm/handler/custom.go line 40 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

will be discussed in the first thread

even if we use string on Rust side I would prefer to use []byte here if possible

[wojtek-coreum]

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

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 124 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I wrote the comment near the Nft check it, please.

ah, ok :-)

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 3 at r2 (raw file):

use coreum_wasm_sdk::assetnft;
use coreum_wasm_sdk::core::{CoreumMsg, CoreumQueries};
use coreum_wasm_sdk::nft::{

same here - about importing only namespace

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Nope, I use the T in the reqExecutor check one more time the usage of that func.

Maybe I don't see sth obvious but when I look at it again I think that this function should be removed completely because it doesn't add any value.
Code could be like this instead:

if nftQuery.NFT != nil {
	nftRes, err := nftQueryServer.NFT(ctx, nftQuery.NFT)
	if err != nil {
		return nil, err
	}

	if nftRes.Nft == nil {
		return nil, nil
	}

	var dataString string
	if nftRes.Nft.Data != nil {
		dataString = string(nftRes.Nft.Data.Value)
	}
	return json.Marshal(NFTResponse{
		NFT: NFT{
			ClassID: nftRes.Nft.ClassId,
			ID:      nftRes.Nft.Id,
			URI:     nftRes.Nft.Uri,
			URIHash: nftRes.Nft.UriHash,
			Data:    dataString,
		},
	})
}

---

x/wasm/handler/custom.go line 405 at r2 (raw file):

					RoyaltyRate: classRes.Class.RoyaltyRate,
				},
			}, err

here and below: nil instead of err

[dzmitryhil]

It means asset NFT and NFT which is native.

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

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

I prefer to keep it

I can add mint_and_send ar you ok?

---

---

integration-tests/modules/wasm_test.go line 39 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

minor: I would prefer to use ftWASM & nftWASM here

Done.

---

integration-tests/modules/wasm_test.go line 155 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

still see old version

Updated.

---

integration-tests/modules/wasm_test.go line 173 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I agree everything should be renamed to ft/nft if possible

Done.

---

integration-tests/modules/testdata/wasm/nft/src/contract.rs line 3 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

same here - about importing only namespace

Done.

Code quote:

coreum_wasm_sdk::nft::{

---

integration-tests/modules/testdata/wasm/sdk/src/core.rs line 1 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

minor: is this file named core because it is core for SDK or because of coreum ?
If this is related to coreum maybe coreum.rs ?

core for SDK

---

integration-tests/modules/testdata/wasm/sdk/src/nft.rs line 11 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Frankly speaking I'm not able to comment here

Probably bytes would be better but only if:

• we are sure which Bytes we should use
• it is common to use bytes in Rust

I'm not sure which Bytes we should use.

---

x/wasm/handler/custom.go line 1 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I would probably prefer to split it into files because NFT & AssetNFT too similar & it is confusing

Agree, updated.

---

x/wasm/handler/custom.go line 34 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I would prefer to avoid usage of these types as much as possible

At least add comments to them

We can't avoid usage of them, the comment is added. This is the IssueClass method with string represented data field.

---

x/wasm/handler/custom.go line 40 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

even if we use string on Rust side I would prefer to use []byte here if possible

This model is temp model which we use to decode the RUST message, on the rust side it's string. What is the reason to use []byte here?

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Maybe I don't see sth obvious but when I look at it again I think that this function should be removed completely because it doesn't add any value.
Code could be like this instead:

if nftQuery.NFT != nil {
	nftRes, err := nftQueryServer.NFT(ctx, nftQuery.NFT)
	if err != nil {
		return nil, err
	}

	if nftRes.Nft == nil {
		return nil, nil
	}

	var dataString string
	if nftRes.Nft.Data != nil {
		dataString = string(nftRes.Nft.Data.Value)
	}
	return json.Marshal(NFTResponse{
		NFT: NFT{
			ClassID: nftRes.Nft.ClassId,
			ID:      nftRes.Nft.Id,
			URI:     nftRes.Nft.Uri,
			URIHash: nftRes.Nft.UriHash,
			Data:    dataString,
		},
	})
}

And why is it better? You will have the same code fore each if, IMO it isn't better.

---

x/wasm/handler/custom.go line 405 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

here and below: nil instead of err

Done.

Code quote:

RoyaltyRate: classRes.Class.RoyaltyRate,

[wojtek-coreum]

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

---

x/wasm/handler/custom.go line 469 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

And why is it better? You will have the same code fore each if, IMO it isn't better.

Exactly, this is the same code without additional indirection introduced by that function and callback

[ysv]

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

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I can add mint_and_send ar you ok?

ok for me

---

[wojtek-coreum]

Reviewed 2 of 5 files at r3, 2 of 2 files at r4.
Reviewable status: 26 of 29 files reviewed, 4 unresolved discussions (waiting on @miladz68, @silverspase, and @ysv)

[dzmitryhil]

Reviewable status: 17 of 29 files reviewed, all discussions resolved (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

integration-tests/modules/testdata/wasm/sdk/src/assetnft.rs line 14 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

will be discussed in the first thread

Let's do it in separate PR.

---

integration-tests/modules/testdata/wasm/sdk/src/nft.rs line 11 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I'm not sure which Bytes we should use.

Let's do it in separate PR.

---

x/wasm/handler/custom.go line 40 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

This model is temp model which we use to decode the RUST message, on the rust side it's string. What is the reason to use []byte here?

Let's do it in separate PR.

[ysv]

Reviewed 4 of 5 files at r3, 9 of 9 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @silverspase)

---

x/wasm/handler/message.go line 33 at r5 (raw file):

//
//nolint:tagliatelle // we keep the name same as consume
type AssetNFTMsgIssueClass struct {

we can keep these structures unexposed to outside world assetNFTMsgIssueClass

same for other structures

[dzmitryhil]

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

---

x/wasm/handler/message.go line 33 at r5 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

we can keep these structures unexposed to outside world assetNFTMsgIssueClass

same for other structures

Done.

Code quote:

type AssetNFTMsgIssueClass struct {

[ysv]

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

Offline

[miladz68]

Reviewed 16 of 27 files at r1, 2 of 5 files at r3, 4 of 9 files at r5, 5 of 5 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase)