new coreum_wasm_sdk features (version upgrade) and integration tests #536

keyleu · 2023-06-26T14:21:18Z

This change is

dzmitryhil

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

integration-tests/modules/testdata/wasm/ft/package/coreum-wasm-sdk/.gitignore line 1 at r1 (raw file):

.idea

It's possible to import the crate from the git, that's how you can tests that the SDK is valid. Could you please do it and remove the copy, since not clear now what will be left.

dzmitryhil

Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions (waiting on @keyleu, @miladz68, @silverspase, and @ysv)

a discussion (no related file):
IMO it's a bad idea to mix up those 2 PRs. Not it is'n clear what to review :-(
It would be nice to separate them.

keyleu

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

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO it's a bad idea to mix up those 2 PRs. Not it is'n clear what to review :-(
It would be nice to separate them.

base64 is only like 2 lines of code in query.go

integration-tests/modules/testdata/wasm/ft/package/coreum-wasm-sdk/.gitignore line 1 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

It's possible to import the crate from the git, that's how you can tests that the SDK is valid. Could you please do it and remove the copy, since not clear now what will be left.

ok will do that

keyleu

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

a discussion (no related file):

Previously, keyleu (Keyne) wrote…

base64 is only like 2 lines of code in query.go

still want me to separate them?

dzmitryhil

Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions (waiting on @keyleu, @miladz68, @silverspase, and @ysv)

a discussion (no related file):

Previously, keyleu (Keyne) wrote…

still want me to separate them?

But better to split it anyways, for the git history and PRs consistency.

keyleu

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

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But better to split it anyways, for the git history and PRs consistency.

done, removed the base64 changes from here and separated in different PR

dzmitryhil

Reviewable status: 0 of 27 files reviewed, 6 unresolved discussions (waiting on @keyleu, @miladz68, @silverspase, and @ysv)

integration-tests/modules/contract.go line 1 at r3 (raw file):

package modules

This file make sence, only if it's located in the modules/testdata/wasm, but if you do it that way update it for IBC tests as well. But not sure that this file is needed at all.

integration-tests/modules/contract.go line 14 at r3 (raw file):

	//nolint
	//go:embed testdata/wasm/ft/artifacts/ft.wasm
	FTWASM []byte

Why the FT is uppercase but bft is lower?

integration-tests/modules/wasm_test.go line 1172 at r3 (raw file):

	royaltyRate := sdk.MustNewDecFromStr("0.1")
	dataString := "data"
	databytes, err := base64.StdEncoding.DecodeString(dataString)

Rename that var plz to dataBase64Bytes or similar since it collides with the dataBytes variable.

integration-tests/modules/wasm_test.go line 1172 at r3 (raw file):

	royaltyRate := sdk.MustNewDecFromStr("0.1")
	dataString := "data"
	databytes, err := base64.StdEncoding.DecodeString(dataString)

Not clear why do you need to Decode the string here. If the fix with the decode/encode on the handler level is correct no extra decoding is needed. Right?

keyleu

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

integration-tests/modules/contract.go line 1 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

This file make sence, only if it's located in the modules/testdata/wasm, but if you do it that way update it for IBC tests as well. But not sure that this file is needed at all.

Will modify as agreed

integration-tests/modules/contract.go line 14 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why the FT is uppercase but bft is lower?

like we discussed, it's for upgrades.

integration-tests/modules/wasm_test.go line 1172 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Rename that var plz to dataBase64Bytes or similar since it collides with the dataBytes variable.

will move to the other PR and rename

integration-tests/modules/wasm_test.go line 1172 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Not clear why do you need to Decode the string here. If the fix with the decode/encode on the handler level is correct no extra decoding is needed. Right?

It's for the test class that we compare with, like discussed.

dzmitryhil

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

keyleu

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

integration-tests/upgrade/ft.go line 28 at r8 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

this comment might be removed

done

wojtek-coreum

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @silverspase, and @ysv)

miladz68

Reviewed 3 of 4 files at r7, 4 of 5 files at r8, 3 of 4 files at r10, all commit messages.
Reviewable status: 31 of 32 files reviewed, 3 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)

miladz68

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

dzmitryhil

Reviewed 8 of 27 files at r1, 10 of 12 files at r2, 1 of 1 files at r3, 2 of 5 files at r5, 3 of 4 files at r7, 4 of 5 files at r8, 4 of 4 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

wojtek-coreum

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

…-and-encode

* Base64 encoding/decoding * Move data * Add data to compare decode for tests * Encode before * Added data generation

…-and-encode

dzmitryhil

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

integration-tests/modules/wasm_test.go line 1177 at r11 (raw file):

		data[i] = uint8(i)
	}
	encodedData := base64.StdEncoding.EncodeToString(data)

You don't need the for. Why you do the make([]byte, 256) it will fill the bytes slice with the default byte values.

dzmitryhil

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

wojtek-coreum

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @keyleu, @silverspase, and @ysv)

integration-tests/upgrade/ft.go line 170 at r11 (raw file):

		},
		BurnRate:           burnRate.String(),
		SendCommissionRate: sendCommissionRate.String(),

I don't think it should be needed. We have many unit and integration tests where those values are skipped and they work.

integration-tests/upgrade/ft.go line 212 at r11 (raw file):

		InitialAmount:      issuanceAmount.String(),
		Description:        "my wasm fungible token",
		BurnRate:           burnRate.String(),

ditto

keyleu

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

integration-tests/upgrade/ft.go line 170 at r11 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I don't think it should be needed. We have many unit and integration tests where those values are skipped and they work.

If you don't add them you get an error, they are not optional.

integration-tests/upgrade/ft.go line 212 at r11 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

ditto

same as above

keyleu

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

integration-tests/modules/wasm_test.go line 1177 at r11 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You don't need the for. Why you do the make([]byte, 256) it will fill the bytes slice with the default byte values.

without the for it provides the same byte value for each element in the vector (0x0). What I want is all the possible byte values.

keyleu

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

integration-tests/upgrade/ft.go line 170 at r11 (raw file):

Previously, keyleu (Keyne) wrote…

If you don't add them you get an error, they are not optional.

The error comes from the wasm handler. It throws an ABCI error.

miladz68

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)

wojtek-coreum

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

integration-tests/upgrade/ft.go line 170 at r11 (raw file):

Previously, keyleu (Keyne) wrote…

The error comes from the wasm handler. It throws an ABCI error.

ah I see, because we pass that message to smart contract to be executed from there. Okay.

wojtek-coreum

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

Added all changes

b9289b9

keyleu requested a review from a team as a code owner June 26, 2023 14:21

keyleu requested review from dzmitryhil, vertex451, miladz68 and ysv and removed request for a team June 26, 2023 14:21

keyleu added 5 commits June 26, 2023 15:24

upgrade cargo

77ef5ca

cargo updates

c4c1b50

wasm files

b73e5a1

Export all wasm files for CI

5d89395

undo

9c8f997

dzmitryhil requested changes Jun 27, 2023

View reviewed changes

keyleu commented Jun 27, 2023

View reviewed changes

remove package

af5236b

keyleu commented Jun 27, 2023

View reviewed changes

dzmitryhil requested changes Jun 27, 2023

View reviewed changes

Remove base64 changes

ceae60b

keyleu changed the title ~~base64 changes to read / write from wasm handler and integration tests for new coreum_wasm_sdk features~~ new coreum_wasm_sdk features (version upgrade) and integration tests Jun 27, 2023

keyleu commented Jun 27, 2023

View reviewed changes

Added build for contract.go

ec66f23

dzmitryhil requested changes Jun 27, 2023

View reviewed changes

keyleu added 2 commits June 27, 2023 10:41

add integration tests

5017d89

remove base64 data in wasm_test

73451d3

keyleu commented Jun 27, 2023

View reviewed changes

keyleu added 3 commits June 27, 2023 11:18

modified location of contract.go under testdata/wasm

f001ed7

linter for wasm files

65331ab

pattern syntax

ce90345

dzmitryhil previously approved these changes Jun 27, 2023

View reviewed changes

remove comment

d2add33

keyleu commented Jun 28, 2023

View reviewed changes

wojtek-coreum previously approved these changes Jun 28, 2023

View reviewed changes

resolve conflict

7deab4b

keyleu dismissed wojtek-coreum’s stale review via 7deab4b June 28, 2023 12:37

merged

5ea8a46

miladz68 reviewed Jun 28, 2023

View reviewed changes

miladz68 previously approved these changes Jun 28, 2023

View reviewed changes

dzmitryhil previously approved these changes Jun 28, 2023

View reviewed changes

wojtek-coreum previously approved these changes Jun 28, 2023

View reviewed changes

keyleu added 2 commits June 28, 2023 16:50

Merge branch 'master' into keyne/wasm-token-upgrade-and-base64-decode…

6cde676

…-and-encode

Base64 encoding/decoding of datastring (#537)

dfbb3df

* Base64 encoding/decoding * Move data * Add data to compare decode for tests * Encode before * Added data generation

keyleu dismissed stale reviews from wojtek-coreum, miladz68, and dzmitryhil via dfbb3df June 28, 2023 16:05

keyleu added 2 commits June 28, 2023 17:08

Merge branch 'master' into keyne/wasm-token-upgrade-and-base64-decode…

6910b30

…-and-encode

Add burning and send commision rate 0

072a83c

dzmitryhil requested changes Jun 29, 2023

View reviewed changes

dzmitryhil approved these changes Jun 29, 2023

View reviewed changes

wojtek-coreum requested changes Jun 29, 2023

View reviewed changes

keyleu commented Jun 29, 2023

View reviewed changes

dzmitryhil requested review from wojtek-coreum and miladz68 June 29, 2023 08:42

miladz68 approved these changes Jun 29, 2023

View reviewed changes

wojtek-coreum approved these changes Jun 29, 2023

View reviewed changes

keyleu merged commit 0891258 into master Jun 29, 2023

keyleu deleted the keyne/wasm-token-upgrade-and-base64-decode-and-encode branch June 29, 2023 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new coreum_wasm_sdk features (version upgrade) and integration tests #536

new coreum_wasm_sdk features (version upgrade) and integration tests #536

keyleu commented Jun 26, 2023 •

edited by CoreumDevelopment

Loading

dzmitryhil left a comment

dzmitryhil 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

dzmitryhil left a comment

keyleu left a comment

wojtek-coreum left a comment

miladz68 left a comment

miladz68 left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

keyleu left a comment

keyleu left a comment

keyleu left a comment

miladz68 left a comment

wojtek-coreum left a comment

wojtek-coreum left a comment

new coreum_wasm_sdk features (version upgrade) and integration tests #536

new coreum_wasm_sdk features (version upgrade) and integration tests #536

Conversation

keyleu commented Jun 26, 2023 • edited by CoreumDevelopment Loading

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

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

dzmitryhil left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

miladz68 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

wojtek-coreum 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

wojtek-coreum 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

miladz68 left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

keyleu commented Jun 26, 2023 •

edited by CoreumDevelopment

Loading