Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify NFT contract to use new protos for updatable NFTs and include examples #812

Merged
merged 6 commits into from
May 1, 2024

Conversation

keyleu
Copy link
Collaborator

@keyleu keyleu commented Apr 26, 2024

Description

This PR uses the new generated rust proto types (which are in a temporary branch of the coreum-wasm-sdk until we upgrade) in the integration tests contracts to mint and update NFT data. Remarks:

  1. The idea is to move away from the wasm_handler (deprecate it) and start using the protos with grpc directly (in cosmwasm 2.0, Stargate is renamed to grpc) so that we don't have to rely in custom bindings, keep consistency between all procedures and also not need to keep bloating the wasm_handler when it's not necessary at all because we can interact directly with grpc from the smart contracts.
  2. For queries we can keep adding them for convenience because they still need to be whitelisted.
  3. One thing I realized is an inconsistent behavior between wasm (due to the handler's code) and directly using grpc. When we don't pass data in grpc, the NFT is created with a DataBytes structure with an empty byte array inside, however, when we were doing it with wasm, there was no DataBytes structure at all (no data field). It's just a small thing and not really important but shows why we should try to avoid complicated logic and extending the handler to avoid such behaviors (It would also have avoided all the Base64 problem we had 1 year ago).
  4. Left one TODO to replace the contract dependency to the new coreum-wasm-sdk version when the final version is uploaded to crates.io

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 Reviewable

@keyleu keyleu requested a review from a team as a code owner April 26, 2024 14:47
@keyleu keyleu requested review from masihyeganeh, dzmitryhil, miladz68 and ysv and removed request for a team April 26, 2024 14:47
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.79%. Comparing base (5b30d9b) to head (de215f8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #812   +/-   ##
=======================================
  Coverage   36.79%   36.79%           
=======================================
  Files         165      165           
  Lines       49342    49342           
=======================================
  Hits        18156    18156           
  Misses      27742    27742           
  Partials     3444     3444           
Flag Coverage Δ
coreum 32.36% <ø> (ø)
coreum-integration-tests-modules 22.90% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @keyleu, @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/nft/src/contract.rs line 174 at r2 (raw file):

    let mint_bytes = mint.to_proto_bytes();

    let msg = CosmosMsg::Stargate {

What about the migration of other messages to use the CosmosMsg::Stargate?
And what about the coreum SDK, what will it provide?

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/nft/src/contract.rs line 174 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the migration of other messages to use the CosmosMsg::Stargate?
And what about the coreum SDK, what will it provide?

  1. There is no need for migration, all messages work using Stargate/Grpc using the right proto structures, which we will have in the wasm sdk (we have them there already because they are used for test tube)

  2. Coreum wasm SDK provides all the proto structures required and will be updated/regenerated with every upgrade (we have a tool for that), and also api for queries (unless you guys want to start whitelisting like other chains do then we don't even need this). And also some constants that are useful, for example the values of the features for both assetft and nft (minting, burning, soulbound etc).
    All current msg structures can be kept there for people who are already using them and since we will keep the wasm handler because we need backward compatibility we can keep those messages in the wasm sdk too.

Actually the wasm handler is not needed for anything, because queries can be whitelisted.

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/nft/src/contract.rs line 174 at r2 (raw file):

Previously, keyleu (Keyne) wrote…
  1. There is no need for migration, all messages work using Stargate/Grpc using the right proto structures, which we will have in the wasm sdk (we have them there already because they are used for test tube)

  2. Coreum wasm SDK provides all the proto structures required and will be updated/regenerated with every upgrade (we have a tool for that), and also api for queries (unless you guys want to start whitelisting like other chains do then we don't even need this). And also some constants that are useful, for example the values of the features for both assetft and nft (minting, burning, soulbound etc).
    All current msg structures can be kept there for people who are already using them and since we will keep the wasm handler because we need backward compatibility we can keep those messages in the wasm sdk too.

Actually the wasm handler is not needed for anything, because queries can be whitelisted.

I meant the wasm handler was not really needed for anything, now it's needed because we need backward compatibility

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @keyleu, @masihyeganeh, @miladz68, and @ysv)


integration-tests/modules/wasm_test.go line 1687 at r2 (raw file):

	requireT.NoError(err)

	// FIXME: When minting an NFT with empty data it creates an []byte{} in the data

Here is the fix for the test.

	emptyData, err := codectypes.NewAnyWithValue(&assetnfttypes.DataBytes{Data: nil})
	requireT.NoError(err)
	expectedNFT := &nfttypes.NFT{
		ClassId: classIDNoWhitelist,
		Id:      mintImmutableNFTReq.ID,
		Data:    emptyData,
	}

	gotNFT := nftResp.Nft
	// encode the data `from` and `to` proto.Any to load same state as `codectypes.NewAnyWithValue` does
	var gotNFTDataBytes assetnfttypes.DataBytes
	requireT.NoError(gotNFTDataBytes.Unmarshal(gotNFT.Data.Value))
	gotNFTData, err := codectypes.NewAnyWithValue(&gotNFTDataBytes)
	requireT.NoError(err)
	gotNFT.Data = gotNFTData

	requireT.Equal(
		expectedNFT, gotNFT,
	)

miladz68
miladz68 previously approved these changes Apr 30, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @keyleu, @masihyeganeh, and @ysv)

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


integration-tests/modules/wasm_test.go line 1687 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Here is the fix for the test.

	emptyData, err := codectypes.NewAnyWithValue(&assetnfttypes.DataBytes{Data: nil})
	requireT.NoError(err)
	expectedNFT := &nfttypes.NFT{
		ClassId: classIDNoWhitelist,
		Id:      mintImmutableNFTReq.ID,
		Data:    emptyData,
	}

	gotNFT := nftResp.Nft
	// encode the data `from` and `to` proto.Any to load same state as `codectypes.NewAnyWithValue` does
	var gotNFTDataBytes assetnfttypes.DataBytes
	requireT.NoError(gotNFTDataBytes.Unmarshal(gotNFT.Data.Value))
	gotNFTData, err := codectypes.NewAnyWithValue(&gotNFTDataBytes)
	requireT.NoError(err)
	gotNFT.Data = gotNFTData

	requireT.Equal(
		expectedNFT, gotNFT,
	)

Thanks for this! Changed it.

@keyleu keyleu requested review from dzmitryhil and miladz68 May 1, 2024 09:35
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@keyleu keyleu merged commit 23d9cdc into master May 1, 2024
10 checks passed
@keyleu keyleu deleted the keyne/wasm-nft-updatable branch May 1, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants