Modified all test-data wasm contracts, added new functionality, messages, queries and added integration tests #525

keyleu · 2023-06-15T15:05:16Z

This change is

dzmitryhil

Looks good!

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

a discussion (no related file):
The CI is failed.

a discussion (no related file):
Why do we need packages/coreum-wasm-sdk commited? What is inside?

keyleu

Thanks!

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

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do we need packages/coreum-wasm-sdk commited? What is inside?

This will be removed after coreum-wasm-sdk v.0.1.2 is on crates.io. I made the PR now with it because so all of you can already check the code. Once I get your approval and the coreum-wasm-sdk is on crates.io I'll remove the package and update the dependencies to use that version.

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The CI is failed.

Yes, I need to discuss this in the meeting because I think it's something related to docker/crust. When building the images it doesn't allow for Cargo.lock to be modified, and since the contract dependencies are updated, this file is automatically updated.

dzmitryhil

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

a discussion (no related file):

Previously, keyleu (Keyne) wrote…

Yes, I need to discuss this in the meeting because I think it's something related to docker/crust. When building the images it doesn't allow for Cargo.lock to be modified, and since the contract dependencies are updated, this file is automatically updated.

Ok, let's discuss.

a discussion (no related file):

Previously, keyleu (Keyne) wrote…

This will be removed after coreum-wasm-sdk v.0.1.2 is on crates.io. I made the PR now with it because so all of you can already check the code. Once I get your approval and the coreum-wasm-sdk is on crates.io I'll remove the package and update the dependencies to use that version.

Clear. So the coreum-wasm-sdk v.0.1.2 should be merged first.

miladz68

Reviewed 17 of 45 files at r1, 13 of 13 files at r2, all commit messages.
Reviewable status: 30 of 48 files reviewed, 15 unresolved discussions (waiting on @keyleu and @ysv)

integration-tests/modules/testdata/wasm/bank-send/src/lib.rs line 3 at r2 (raw file):

pub mod contract;
pub mod error;
pub mod msg;

you need an empty line. notice the red marker

integration-tests/modules/testdata/wasm/bank-send/src/msg.rs line 14 at r2 (raw file):

        recipient: String,
    },
}

you need and empty line

integration-tests/modules/testdata/wasm/ft/README.md line 1 at r2 (raw file):

# FT Contract

I think this README file is not needed !

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/Cargo.toml line 13 at r2 (raw file):

serde = { version = "1.0.164", default-features = false, features = ["derive"] }
schemars = "0.8.12"
cosmwasm-schema = "1.2.6"

empty line needed.

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/.github/workflows/release.yml line 1 at r2 (raw file):

name: release

this file should not exist.

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/src/assetnft.rs line 1 at r2 (raw file):

use cosmwasm_schema::QueryResponses;

I assume you have copied the sdk code here, but that seems to not be the right approach. the correct version of the sdk must be used after the other PR is merged.

integration-tests/modules/testdata/wasm/nft/README.md line 1 at r2 (raw file):

# NFT Contract

I don't think we need a README for a test contract ?!

integration-tests/modules/testdata/wasm/nft/packages/coreum-wasm-sdk/Cargo.toml line 13 at r2 (raw file):

serde = { version = "1.0.164", default-features = false, features = ["derive"] }
schemars = "0.8.12"
cosmwasm-schema = "1.2.6"

empty line needed.

integration-tests/modules/testdata/wasm/nft/packages/coreum-wasm-sdk/.github/workflows/release.yml line 1 at r2 (raw file):

name: release

we should remove the copied package and just point to the new version after it is merged.

integration-tests/modules/testdata/wasm/simple-state/Cargo.toml line 37 at r2 (raw file):

cosmwasm-schema = "1.2.6"
cw-storage-plus = "1.0.1"

empty line needed.

integration-tests/modules/testdata/wasm/simple-state/src/lib.rs line 4 at r2 (raw file):

pub mod error;
pub mod msg;
pub mod state;

empty line needed.

integration-tests/modules/testdata/wasm/simple-state/src/msg.rs line 23 at r2 (raw file):

pub struct CountResponse {
    pub count: i32,
}

empty line needed.

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

use cw_storage_plus::Item;

pub const COUNTER: Item<i32> = Item::new("counter");

empty line neded.

keyleu

Reviewable status: 30 of 48 files reviewed, 16 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Clear. So the coreum-wasm-sdk v.0.1.2 should be merged first.

Correct, this one should not be merged yet.

integration-tests/modules/testdata/wasm/bank-send/src/lib.rs line 3 at r2 (raw file):

Previously, miladz68 (milad) wrote…

you need an empty line. notice the red marker

Done.

integration-tests/modules/testdata/wasm/bank-send/src/msg.rs line 14 at r2 (raw file):

Previously, miladz68 (milad) wrote…

you need and empty line

Done.

integration-tests/modules/testdata/wasm/ft/README.md line 1 at r2 (raw file):

# FT Contract

Really? I added all the information here because the coreum-wasm-sdk package links to this repository for new users looking for Smart contract examples.

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/Cargo.toml line 13 at r2 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed.

Done.

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/.github/workflows/release.yml line 1 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this file should not exist.

Removed.

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/src/assetnft.rs line 1 at r2 (raw file):

Previously, miladz68 (milad) wrote…

I assume you have copied the sdk code here, but that seems to not be the right approach. the correct version of the sdk must be used after the other PR is merged.

Yes, that's the plan. We don't merge this until I've removed the package and updated the version. I needed to do it this way for now because the other PR is not merged yet and the new version is not on crates.io

integration-tests/modules/testdata/wasm/nft/README.md line 1 at r2 (raw file):

Previously, miladz68 (milad) wrote…

I don't think we need a README for a test contract ?!

Same as above.

integration-tests/modules/testdata/wasm/nft/packages/coreum-wasm-sdk/Cargo.toml line 13 at r2 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed.

Done.

integration-tests/modules/testdata/wasm/nft/packages/coreum-wasm-sdk/.github/workflows/release.yml line 1 at r2 (raw file):

Previously, miladz68 (milad) wrote…

we should remove the copied package and just point to the new version after it is merged.

Done.

integration-tests/modules/testdata/wasm/simple-state/src/lib.rs line 4 at r2 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed.

Done.

integration-tests/modules/testdata/wasm/simple-state/src/msg.rs line 23 at r2 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed.

Done.

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

Previously, miladz68 (milad) wrote…

empty line neded.

Done.

keyleu

Reviewable status: 26 of 48 files reviewed, 15 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

integration-tests/modules/testdata/wasm/ft/README.md line 1 at r2 (raw file):

Previously, miladz68 (milad) wrote…

I think this README file is not needed !

Like agreed, we are keeping it.

dzmitryhil

Reviewed 24 of 45 files at r1, 13 of 13 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @miladz68 and @ysv)

keyleu

Reviewable status: 41 of 52 files reviewed, 15 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Ok, let's discuss.

Ok this is fixed now.

dzmitryhil

Reviewed 10 of 11 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @keyleu, @miladz68, and @ysv)

keyleu

Reviewable status: 25 of 52 files reviewed, 15 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

a discussion (no related file):

Previously, keyleu (Keyne) wrote…

Correct, this one should not be merged yet.

Ok, now it's using the v0.1.2, so it can be reviewed

integration-tests/modules/testdata/wasm/simple-state/Cargo.toml line 37 at r2 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed.

done!

integration-tests/modules/testdata/wasm/ft/packages/coreum-wasm-sdk/src/assetnft.rs line 1 at r2 (raw file):

Previously, keyleu (Keyne) wrote…

Yes, that's the plan. We don't merge this until I've removed the package and updated the version. I needed to do it this way for now because the other PR is not merged yet and the new version is not on crates.io

package is now removed.

miladz68

Reviewed 11 of 11 files at r3, 4 of 11 files at r4, 27 of 27 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @ysv)

dzmitryhil

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

integration-tests/modules/wasm_test.go line 966 at r6 (raw file):

	requireT.NoError(json.Unmarshal(queryOut, &wasmTokensRes))
	requireT.Equal(
		expectedToken.Denom, wasmTokensRes.Tokens[0].Denom,

Why do you validate denom only?

keyleu

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

integration-tests/modules/wasm_test.go line 966 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you validate denom only?

The wasm sdk version 0.1.1 had a structure

pub struct Token {
pub denom: String,
pub issuer: String,
pub symbol: String,
pub subunit: String,
pub precision: u32,
pub description: Option,
pub features: Option<Vec>,
pub burn_rate: String,
pub send_commission_rate: String,
}

But the proto file includes a uint32 version = 6 at the end.
In the individual token test it does this before equaling:
wasmTokenRes.Token.Version = expectedToken.Version // test should work with any version
to make sure version is the same.

If you want I can change it to validate everything and hardcode the version in the same way, I thought checking the denom looks more understandable to see that we receive the same token (all the fields are the same anyways).

I can change it if you want.

dzmitryhil

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

integration-tests/modules/wasm_test.go line 1565 at r6 (raw file):

	requireT.NoError(json.Unmarshal(queryOut, &nftClassesQueryRes))

	var found = classInArray(nftClass{

You can replace with

requireT.Contains(nftClass{
		ID:          expectedClass.Id,
		Name:        expectedClass.Name,
		Symbol:      expectedClass.Symbol,
		Description: expectedClass.Description,
		URI:         expectedClass.URI,
		URIHash:     expectedClass.URIHash,
		Data:        dataString,
	}, nftClassesQueryRes.Classes)

dzmitryhil

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

integration-tests/modules/wasm_test.go line 966 at r6 (raw file):

Previously, keyleu (Keyne) wrote…

The wasm sdk version 0.1.1 had a structure

pub struct Token {
pub denom: String,
pub issuer: String,
pub symbol: String,
pub subunit: String,
pub precision: u32,
pub description: Option,
pub features: Option<Vec>,
pub burn_rate: String,
pub send_commission_rate: String,
}

But the proto file includes a uint32 version = 6 at the end.
In the individual token test it does this before equaling:
wasmTokenRes.Token.Version = expectedToken.Version // test should work with any version
to make sure version is the same.

If you want I can change it to validate everything and hardcode the version in the same way, I thought checking the denom looks more understandable to see that we receive the same token (all the fields are the same anyways).

I can change it if you want.

IMO better to set the version validate the entire struct.

keyleu

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

integration-tests/modules/wasm_test.go line 966 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO better to set the version validate the entire struct.

Ok I'll change it.

integration-tests/modules/wasm_test.go line 1565 at r6 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can replace with

requireT.Contains(nftClass{
		ID:          expectedClass.Id,
		Name:        expectedClass.Name,
		Symbol:      expectedClass.Symbol,
		Description: expectedClass.Description,
		URI:         expectedClass.URI,
		URIHash:     expectedClass.URIHash,
		Data:        dataString,
	}, nftClassesQueryRes.Classes)

I had a problem with this because it returned it in different orders (only happened with nft module)

keyleu

Reviewable status: 51 of 52 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

integration-tests/modules/wasm_test.go line 1565 at r6 (raw file):

Previously, keyleu (Keyne) wrote…

I had a problem with this because it returned it in different orders (only happened with nft module)

There were 3 classes in the array, and sometimes it would be in position 0, sometimes 1 and sometimes 2. So I just checked it was in there to ensure tests didn't fail.

keyleu

Reviewable status: 51 of 52 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

integration-tests/modules/wasm_test.go line 1565 at r6 (raw file):

Previously, keyleu (Keyne) wrote…

There were 3 classes in the array, and sometimes it would be in position 0, sometimes 1 and sometimes 2. So I just checked it was in there to ensure tests didn't fail.

Oh nevermind, you meant Contains. My mistake. Ok, will change.

dzmitryhil

Reviewable status: 51 of 52 files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)

miladz68

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

keyleu added 2 commits June 15, 2023 15:56

Added new queries, messages and integration tests

6d2b8d3

Removed as submodule

f73c689

keyleu requested review from dzmitryhil, miladz68, ysv and wojtek-coreum June 15, 2023 15:05

keyleu requested a review from a team as a code owner June 15, 2023 15:05

dzmitryhil requested changes Jun 16, 2023

View reviewed changes

Documentation and parsing

c5d6178

keyleu commented Jun 16, 2023

View reviewed changes

dzmitryhil requested changes Jun 16, 2023

View reviewed changes

wojtek-coreum removed their request for review June 16, 2023 11:47

miladz68 requested changes Jun 16, 2023

View reviewed changes

Remove files and parsing

0f41840

keyleu commented Jun 16, 2023

View reviewed changes

dzmitryhil reviewed Jun 19, 2023

View reviewed changes

Added new queries for NFT module and integration tests

c22a7ab

keyleu commented Jun 19, 2023

View reviewed changes

Merge branch 'master' into keyne/new-messages-with-tests

71607d4

dzmitryhil reviewed Jun 19, 2023

View reviewed changes

keyleu added 4 commits June 19, 2023 12:57

linting

8ec659e

linting and integration tests

1873422

test denom

7cd184b

Remove package

bbb1478

keyleu commented Jun 19, 2023

View reviewed changes

miladz68 previously approved these changes Jun 20, 2023

View reviewed changes

Merge master into keyne/new-messages-with-tests

9aeb49e

dzmitryhil requested changes Jun 20, 2023

View reviewed changes

keyleu commented Jun 20, 2023

View reviewed changes

dzmitryhil requested changes Jun 20, 2023

View reviewed changes

keyleu commented Jun 20, 2023

View reviewed changes

Validate entire token struct

6cf04be

keyleu dismissed miladz68’s stale review via 6cf04be June 20, 2023 10:44

keyleu commented Jun 20, 2023

View reviewed changes

keyleu added 4 commits June 20, 2023 11:48

removed loop for contains

99c1a96

Merge branch 'master' into keyne/new-messages-with-tests

ff759e5

lint

b7b91fe

Contains mistake

19957b6

dzmitryhil approved these changes Jun 20, 2023

View reviewed changes

miladz68 approved these changes Jun 20, 2023

View reviewed changes

keyleu merged commit b59d31f into master Jun 20, 2023

keyleu deleted the keyne/new-messages-with-tests branch June 20, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified all test-data wasm contracts, added new functionality, messages, queries and added integration tests #525

Modified all test-data wasm contracts, added new functionality, messages, queries and added integration tests #525

keyleu commented Jun 15, 2023 •

edited by CoreumDevelopment

Loading

dzmitryhil left a comment

keyleu left a comment

dzmitryhil left a comment

miladz68 left a comment

keyleu left a comment

keyleu left a comment

dzmitryhil left a comment

keyleu left a comment

dzmitryhil left a comment

keyleu left a comment

miladz68 left a comment

dzmitryhil left a comment

keyleu left a comment

dzmitryhil left a comment

dzmitryhil left a comment

keyleu left a comment

keyleu left a comment

keyleu left a comment

dzmitryhil left a comment

miladz68 left a comment

Modified all test-data wasm contracts, added new functionality, messages, queries and added integration tests #525

Modified all test-data wasm contracts, added new functionality, messages, queries and added integration tests #525

Conversation

keyleu commented Jun 15, 2023 • edited by CoreumDevelopment Loading

dzmitryhil left a comment

Choose a reason for hiding this comment

keyleu 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

keyleu left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

keyleu 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

keyleu 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

keyleu left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

keyleu 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

keyleu commented Jun 15, 2023 •

edited by CoreumDevelopment

Loading