Complex ft integration tests (phase 2) #418

pgoos · 2023-03-01T09:48:29Z

This PR implements the following integration test scenarios:

// # Mint + Burn rate
// - burn rate should not apply when tokens are received by the issuer because of minting
//
// # Mint + Send commission
// - send commission should not apply when tokens are received by the issuer because of minting
//
// # Burn + Burn rate
// - burn rate should not apply when tokens are burnt
//
// # Burn + Send commission
// - send commission should not apply when tokens are burnt
//
// # Burn + Freeze
// - when someone wants to burn more than is allowed by non-frozen balance tx should fail
//
// # Burn rate + Freeze
// - when burn rate causes a non-frozen balance to be exceeded, tx should fail
//
// # Burn rate + Send Commission + Freeze
// - when there is enough unfrozen coins for the burn rate, but not enough for commission (and vice versa), tx should fail

This change is

…om/CoreumFoundation/coreum into piotr/complex-ft-integration-tests

dzmitryhil

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

integration-tests/modules/assetft_test.go line 2015 at r3 (raw file):

// TestAssetFTSendCommissionRate_OnMinting verifies send commission rate is not applied on received minted tokens
func TestAssetFTSendCommissionRate_OnMinting(t *testing.T) {

How about merge this test and burning rate? The same comment all tests where you do the same for commission and burn rates.
@miladz68 @ysv @wojtek-coreum what to you think ?

dzmitryhil

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @pgoos, @silverspase, @wojtek-coreum, and @ysv)

pgoos

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

integration-tests/modules/assetft_test.go line 2015 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about merge this test and burning rate? The same comment all tests where you do the same for commission and burn rates.
@miladz68 @ysv @wojtek-coreum what to you think ?

done. Merged burn rate and send commission rate cases on minting/burning scenarios

miladz68

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pgoos, @silverspase, @wojtek-coreum, and @ysv)

integration-tests/modules/assetft_test.go line 2015 at r3 (raw file):

Previously, pgoos (Piotr Gesicki) wrote…

done. Merged burn rate and send commission rate cases on minting/burning scenarios

I am still seeing 2 tests here.

integration-tests/modules/assetft_test.go line 1960 at r4 (raw file):

// TestAssetFTBurnRate_OnMinting verifies both burn rate and send commision rate are not applied on received minted tokens
func TestAssetFTBurnRate_SendCommissionRate_OnMinting(t *testing.T) {

Maybe TestAssetFT_RatesAreNotAppliedOnMinting is a better name ? wdyt ?

integration-tests/modules/assetft_test.go line 2002 at r4 (raw file):

	requireT.NoError(err)
	tokenIssuedEvents, err := event.FindTypedEvents[*assetfttypes.EventIssued](res.Events)

you don't need to assert this event, we already do it in other test and that is enough.

integration-tests/modules/assetft_test.go line 2186 at r4 (raw file):

	requireT.NoError(err)
	tokenIssuedEvents, err := event.FindTypedEvents[*assetfttypes.EventIssued](res.Events)

same comment, asserting event is not needed.

integration-tests/modules/assetft_test.go line 2194 at r4 (raw file):

		FromAddress: issuer.String(),
		ToAddress:   recipient.String(),
		Amount:      sdk.NewCoins(sdk.NewCoin(denom, sdk.NewInt(500))),

let's send 550 so after freeze and burn, there is still 50 balance available.

integration-tests/modules/assetft_test.go line 2287 at r4 (raw file):

	)

	// Issue an fungible token

an => a

integration-tests/modules/assetft_test.go line 2386 at r4 (raw file):

// TestAssetFTFreeze_WithBurnRate_WithSendCommissionRate_1 verifies that
// it is not possible apply a burn rate on sending tokens - outside of freezing limit (when send commission rate value is within limit)

possible => possible to

integration-tests/modules/assetft_test.go line 2387 at r4 (raw file):

// TestAssetFTFreeze_WithBurnRate_WithSendCommissionRate_1 verifies that
// it is not possible apply a burn rate on sending tokens - outside of freezing limit (when send commission rate value is within limit)
func TestAssetFTFreeze_WithBurnRate_WithSendCommissionRate_1(t *testing.T) {

this test postfixed with 1 and the next test are exactly the same, the only difference is burn and commision rates. maybe you can merge them into a single table test.

wojtek-coreum

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

integration-tests/modules/assetft_test.go line 2254 at r4 (raw file):

}

// TestAssetFTFreezeAndBurnRate verifies that it is not possible apply a burn rate on sending tokens - outside of freezing limit

possible to apply

integration-tests/modules/assetft_test.go line 2494 at r4 (raw file):

	})

	// try send from recipient1 to recipient2. Tx should fail because burn rate value

try to

wojtek-coreum

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @pgoos, @silverspase, and @ysv)

a discussion (no related file):
Please remove Piotr/ from PR title

pgoos

Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

Please remove Piotr/ from PR title

Done.

integration-tests/modules/assetft_test.go line 2015 at r3 (raw file):

Previously, miladz68 (milad) wrote…

I am still seeing 2 tests here.

what are you referring to @miladz68 ?

integration-tests/modules/assetft_test.go line 1960 at r4 (raw file):

Previously, miladz68 (milad) wrote…

Maybe TestAssetFT_RatesAreNotAppliedOnMinting is a better name ? wdyt ?

Done.

Code quote:

TestAssetFTBurnRate_SendCommissionRate_OnMinting(

integration-tests/modules/assetft_test.go line 2002 at r4 (raw file):

Previously, miladz68 (milad) wrote…

you don't need to assert this event, we already do it in other test and that is enough.

removed.

integration-tests/modules/assetft_test.go line 2186 at r4 (raw file):

Previously, miladz68 (milad) wrote…

same comment, asserting event is not needed.

removed.

integration-tests/modules/assetft_test.go line 2194 at r4 (raw file):

Previously, miladz68 (milad) wrote…

let's send 550 so after freeze and burn, there is still 50 balance available.

updated to 550.

Code quote:

TestAssetFTFreezeAndBurn

integration-tests/modules/assetft_test.go line 2254 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

possible to apply

Done.

integration-tests/modules/assetft_test.go line 2287 at r4 (raw file):

Previously, miladz68 (milad) wrote…

an => a

done

integration-tests/modules/assetft_test.go line 2386 at r4 (raw file):

Previously, miladz68 (milad) wrote…

possible => possible to

Done.

integration-tests/modules/assetft_test.go line 2494 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

try to

done

miladz68

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pgoos, @silverspase, @wojtek-coreum, and @ysv)

pgoos

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

integration-tests/modules/assetft_test.go line 2387 at r4 (raw file):

Previously, miladz68 (milad) wrote…

this test postfixed with 1 and the next test are exactly the same, the only difference is burn and commision rates. maybe you can merge them into a single table test.

updated. Created table driven test

miladz68

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

…om/CoreumFoundation/coreum into piotr/complex-ft-integration-tests

wojtek-coreum

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

dzmitryhil

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

dzmitryhil

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

dzmitryhil

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

wojtek-coreum

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

pgoos added 2 commits March 1, 2023 08:14

add new cases part 1

8d39255

add new FT cases part 2

83a65f9

pgoos requested review from ysv, miladz68, wojtek-coreum, dzmitryhil and vertex451 as code owners March 1, 2023 09:48

pgoos and others added 5 commits March 1, 2023 10:54

Merge branch 'master' into piotr/complex-ft-integration-tests

7bc78a7

fix + add missing case

c665715

Merge branch 'piotr/complex-ft-integration-tests' of https://github.c…

0297ea6

…om/CoreumFoundation/coreum into piotr/complex-ft-integration-tests

update value in assertions

ddd9d2f

Merge branch 'master' into piotr/complex-ft-integration-tests

1077d32

dzmitryhil suggested changes Mar 2, 2023

View reviewed changes

dzmitryhil reviewed Mar 2, 2023

View reviewed changes

pgoos added 2 commits March 2, 2023 17:54

Merge branch 'master' into piotr/complex-ft-integration-tests

22d217b

combine cases for burn rate and send commission rate

a6a85c8

pgoos commented Mar 2, 2023

View reviewed changes

fix linting errors

ed64886

miladz68 requested changes Mar 3, 2023

View reviewed changes

wojtek-coreum requested changes Mar 3, 2023

View reviewed changes

Fix linting errors and remove redundant case

bbcd763

pgoos changed the title ~~Piotr/complex ft integration tests~~ Complex ft integration tests (phase 2) Mar 3, 2023

pgoos added 2 commits March 3, 2023 12:06

fix another lint issue

9f121a4

addressed comments

192441c

pgoos commented Mar 3, 2023

View reviewed changes

miladz68 reviewed Mar 3, 2023

View reviewed changes

replace 2 explicit tests with 1 table driven test

1b4d81c

pgoos commented Mar 3, 2023

View reviewed changes

miladz68 previously approved these changes Mar 3, 2023

View reviewed changes

apply t.Parallel() at the top test level

afacf72

pgoos dismissed miladz68’s stale review via afacf72 March 3, 2023 16:38

pgoos and others added 4 commits March 6, 2023 15:08

Merge branch 'master' into piotr/complex-ft-integration-tests

f365e0d

fix linter

d057b5b

Merge branch 'piotr/complex-ft-integration-tests' of https://github.c…

f81a763

…om/CoreumFoundation/coreum into piotr/complex-ft-integration-tests

Merge branch 'master' into piotr/complex-ft-integration-tests

b28c1b5

wojtek-coreum previously approved these changes Mar 7, 2023

View reviewed changes

dzmitryhil approved these changes Mar 7, 2023

View reviewed changes

dzmitryhil previously approved these changes Mar 7, 2023

View reviewed changes

fix typo in assertion

0fb0a33

pgoos dismissed stale reviews from dzmitryhil and wojtek-coreum via 0fb0a33 March 7, 2023 13:49

dzmitryhil approved these changes Mar 7, 2023

View reviewed changes

wojtek-coreum approved these changes Mar 7, 2023

View reviewed changes

pgoos merged commit 93ca77c into master Mar 7, 2023

pgoos deleted the piotr/complex-ft-integration-tests branch March 7, 2023 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complex ft integration tests (phase 2) #418

Complex ft integration tests (phase 2) #418

pgoos commented Mar 1, 2023 •

edited

Loading

dzmitryhil left a comment

dzmitryhil left a comment

pgoos left a comment

miladz68 left a comment

wojtek-coreum left a comment

wojtek-coreum left a comment

pgoos left a comment

miladz68 left a comment

pgoos left a comment

miladz68 left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

dzmitryhil left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

Complex ft integration tests (phase 2) #418

Complex ft integration tests (phase 2) #418

Conversation

pgoos commented Mar 1, 2023 • edited Loading

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

pgoos 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

pgoos left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

pgoos 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

dzmitryhil 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

pgoos commented Mar 1, 2023 •

edited

Loading