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

Update query handler and separate contracts with legacy bindings and stargate/grpc for transactions #827

Merged
merged 16 commits into from
May 13, 2024

Conversation

keyleu
Copy link
Collaborator

@keyleu keyleu commented May 7, 2024

Description

  • As discussed, we're deprecating the wasm_handler. This PR separates contracts for both asset FT and asset NFT into two contracts, one using the proto types and stargate (grpc when we upgrade to v50 and cosmwasm 2.0) for transactions and one using the handler (which we won't keep updating at the moment).

  • The queries we will deprecate them when we upgrade because of the discussion regarding how stargate queries are marshalled currently, so for now we keep using the handler for the queries (which we needed to update anyways for backward compatibility with the Data field)

  • Updated the wasm_handler queier to work with DataDynamic types.

  • Added examples of doing Clawback from Smart Contract.

  • Left to do for the future, in next upgrade: Whitelist Stargate queries and write tests in the new FT/NFT contracts.

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 May 7, 2024 18:06
@keyleu keyleu requested review from masihyeganeh, dzmitryhil, miladz68 and ysv and removed request for a team May 7, 2024 18:06
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.06%. Comparing base (0da3336) to head (5c3ebd0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   37.05%   37.06%   +0.01%     
==========================================
  Files         182      182              
  Lines       52698    52713      +15     
==========================================
+ Hits        19525    19536      +11     
- Misses      29636    29639       +3     
- Partials     3537     3538       +1     
Flag Coverage Δ
coreum 32.95% <ø> (-0.01%) ⬇️
coreum-integration-tests-modules 23.79% <ø> (+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 28 files reviewed, 3 unresolved discussions (waiting on @keyleu, @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/ft/src/contract.rs line 331 at r1 (raw file):

    Ok(Response::new()
        .add_attribute("method", "globally_unfreeze")

clawback


x/wasm/handler/query.go line 668 at r1 (raw file):

}

func unmarshalData(data *codectypes.Any) (string, error) {

Use switch instead

	switch data.TypeUrl {
	case "/" + proto.MessageName((*assetnfttypes.DataBytes)(nil)):
		var datab assetnfttypes.DataBytes
		err := proto.Unmarshal(data.Value, &datab)
		if err != nil {
			return "", errors.WithStack(err)
		}
		return base64.StdEncoding.EncodeToString(datab.Data), nil
	case "/" + proto.MessageName((*assetnfttypes.DataDynamic)(nil)):
		var datadynamic assetnfttypes.DataDynamic
		err := proto.Unmarshal(data.Value, &datadynamic)
		if err != nil {
			return "", errors.WithStack(err)
		}
		bytes, err := datadynamic.Marshal()
		if err != nil {
			return "", errors.WithStack(err)
		}
		return base64.StdEncoding.EncodeToString(bytes), nil
	default:
		return "", errors.Errorf("unsupported data type %s", data.TypeUrl)
	}
}



x/wasm/handler/query.go line 668 at r1 (raw file):

}

func unmarshalData(data *codectypes.Any) (string, error) {

I see that you use the unmarshalData for both class data and NFT data, and it might be OK, but be attentive, with the transactions, since the DataDynamic is for the NFT data only !!!

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 28 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/ft/src/contract.rs line 331 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

clawback

Fixed


x/wasm/handler/query.go line 668 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Use switch instead

	switch data.TypeUrl {
	case "/" + proto.MessageName((*assetnfttypes.DataBytes)(nil)):
		var datab assetnfttypes.DataBytes
		err := proto.Unmarshal(data.Value, &datab)
		if err != nil {
			return "", errors.WithStack(err)
		}
		return base64.StdEncoding.EncodeToString(datab.Data), nil
	case "/" + proto.MessageName((*assetnfttypes.DataDynamic)(nil)):
		var datadynamic assetnfttypes.DataDynamic
		err := proto.Unmarshal(data.Value, &datadynamic)
		if err != nil {
			return "", errors.WithStack(err)
		}
		bytes, err := datadynamic.Marshal()
		if err != nil {
			return "", errors.WithStack(err)
		}
		return base64.StdEncoding.EncodeToString(bytes), nil
	default:
		return "", errors.Errorf("unsupported data type %s", data.TypeUrl)
	}
}


Changed, ty for this. Was actually looking for a way to get the name instead of writing it.


x/wasm/handler/query.go line 668 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I see that you use the unmarshalData for both class data and NFT data, and it might be OK, but be attentive, with the transactions, since the DataDynamic is for the NFT data only !!!

Yes, I'm aware of this, since it's only used for queries it's OK because it will always be DataBytes for Class. I did it to not create unnecessary duplication of code.

dzmitryhil
dzmitryhil previously approved these changes May 9, 2024
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 28 files reviewed, all discussions resolved (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

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


integration-tests/contracts/modules/ft-legacy/Cargo.toml line 38 at r2 (raw file):

thiserror = "1.0.50"
# TODO(keyleu): Update dependency once final version of coreum-wasm-sdk crate is pushed
coreum-wasm-sdk = { git = "https://github.com/CoreumFoundation/coreum-wasm-sdk.git", branch = "keyne/upgrade-wasm-sdk" }

I see that you are using coreum-wasm-sdk = "0.2.4" for ntf-legacy and the new branch for ft-legacy. I think it makes sense to actually use the old released version for both


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

	requireT.NoError(err)

	classID := assetnfttypes.BuildClassID(issueClassReqNoWhitelist.Symbol, sdk.MustAccAddressFromBech32(contractAddrNoWhitelist)) //nolint:lll

You could actually work around the line limit by moving the parameters to a new line


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

	// Mint a mutable NFT with both Owner and Issuer as editors

nit: unnecessary blank line

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: 24 of 28 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)


integration-tests/contracts/modules/ft-legacy/Cargo.toml line 38 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I see that you are using coreum-wasm-sdk = "0.2.4" for ntf-legacy and the new branch for ft-legacy. I think it makes sense to actually use the old released version for both

Actually need it for ft-legacy because I added the globally_frozen field to Token which wasn't present there (nothing was added to nft).


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

Previously, masihyeganeh (Masih Yeganeh) wrote…

You could actually work around the line limit by moving the parameters to a new line

Fixed.


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

Previously, masihyeganeh (Masih Yeganeh) wrote…

nit: unnecessary blank line

removed.

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: 23 of 28 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

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

@keyleu keyleu merged commit cb42e0e into master May 13, 2024
9 checks passed
@keyleu keyleu deleted the keyne/split-contracts-legacy branch May 13, 2024 11:16
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