Handle burn rate in extensions

[masihyeganeh]

Description

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 

[miladz68]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, @wojtek-coreum, and @ysv)

---

integration-tests/modules/assetft_extension_test.go line 1484 at r1 (raw file):

	denom := tokenIssuedEvents[0].Denom

	// send from issuer to recipient1 (burn must not apply)

I am not seeing the check on the contract ! that the issuer (actually the admin) is exempted from the burn rate. then why is it working ? I assume the calculate rate function is not working as intended. I assume it will be fixed in the other PR that you have.

---

integration-tests/modules/assetft_extension_test.go line 1499 at r1 (raw file):

	requireT.NoError(err)
	// assert that we don't receive events with empty amounts
	requireT.NotContains(txRes.RawLog, `{"key":"amount"}`)

you should filter the events by event type, this is not the correct way to do it. look at other tests to see how events are looked up.

---

integration-tests/modules/assetft_extension_test.go line 1546 at r1 (raw file):

	})

	// send from recipient2 to issuer (burn must not apply)

change issuer to admin, we now work with admins from now on.

---

x/asset/ft/keeper/keeper_extension_test.go line 754 at r1 (raw file):

}

func TestKeeper_Extension_BurnRate_BankMultiSend(t *testing.T) {

I assume this is the exact copy from the other test right ?

---

x/asset/ft/keeper/test-contracts/asset-extension/src/contract.rs line 267 at r1 (raw file):

        };

        burn_amount = Uint128::zero();

why are you changing burn_amount to zero ? should you just return here ?

[masihyeganeh]

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

---

integration-tests/modules/assetft_extension_test.go line 1484 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I am not seeing the check on the contract ! that the issuer (actually the admin) is exempted from the burn rate. then why is it working ? I assume the calculate rate function is not working as intended. I assume it will be fixed in the other PR that you have.

Done. Yes, it is fixed after rebasing the branch

---

integration-tests/modules/assetft_extension_test.go line 1499 at r1 (raw file):

Previously, miladz68 (milad) wrote…

you should filter the events by event type, this is not the correct way to do it. look at other tests to see how events are looked up.

This test is copied from the assetft integration test. I'm not sure why it is decided to keep it this way, but there should be a reason

---

integration-tests/modules/assetft_extension_test.go line 1546 at r1 (raw file):

Previously, miladz68 (milad) wrote…

change issuer to admin, we now work with admins from now on.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 754 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I assume this is the exact copy from the other test right ?

This test is copied from the assetft integration test but the extension is added

---

x/asset/ft/keeper/test-contracts/asset-extension/src/contract.rs line 267 at r1 (raw file):

Previously, miladz68 (milad) wrote…

why are you changing burn_amount to zero ? should you just return here ?

Done.

[miladz68]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

[miladz68]

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

a discussion (no related file):
Wait for my PR for sudo to be merged and make this PR against master.

---

[masihyeganeh]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, miladz68 (milad) wrote…

Wait for my PR for sudo to be merged and make this PR against master.

Done.

---

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.06%. Comparing base (5a7ab26) to head (1fd10f4).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #834   +/-   ##
=======================================
  Coverage   37.06%   37.06%           
=======================================
  Files         182      182           
  Lines       52722    52722           
=======================================
  Hits        19544    19544           
  Misses      29640    29640           
  Partials     3538     3538           

Flag	Coverage Δ
coreum	32.95% <ø> (ø)
coreum-integration-tests-modules	23.81% <ø> (ø)

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.

[dzmitryhil]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @miladz68, @wojtek-coreum, and @ysv)

---

x/asset/ft/keeper/keeper_extension_test.go line 725 at r3 (raw file):

	ba.assertCoinDistribution(denom, map[*sdk.AccAddress]int64{
		&recipient:  392,

Could you please use AmountIgnoreBurnRateTrigger in the assertions when the extensions uses the rate as initial - AmountIgnoreBurnRateTrigger. In this way it is easier to validate (in both tests ).

---

x/asset/ft/keeper/keeper_extension_test.go line 820 at r3 (raw file):

	// create 2 recipient for every admin to allow for complex test cases
	recipients = append(recipients, sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()))
	recipients = append(recipients, sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()))

You can simply use and call it once.

	recipients := lo.RepeatBy(4, func(index int) sdk.AccAddress {
		return sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
	})

[miladz68]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

[masihyeganeh]

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

---

x/asset/ft/keeper/keeper_extension_test.go line 725 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Could you please use AmountIgnoreBurnRateTrigger in the assertions when the extensions uses the rate as initial - AmountIgnoreBurnRateTrigger. In this way it is easier to validate (in both tests ).

I prefer to add comments to each line and keep the calculated number there. Comments added

---

x/asset/ft/keeper/keeper_extension_test.go line 820 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can simply use and call it once.

	recipients := lo.RepeatBy(4, func(index int) sdk.AccAddress {
		return sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
	})

Thank you. Yeah, it's better

[masihyeganeh]

Reviewed 2 of 3 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

[wojtek-coreum]

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

[dzmitryhil]

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

---

x/asset/ft/keeper/keeper_extension_test.go line 725 at r3 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I prefer to add comments to each line and keep the calculated number there. Comments added

But why? If you already have those constants?

[masihyeganeh]

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

---

x/asset/ft/keeper/keeper_extension_test.go line 725 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But why? If you already have those constants?

Because there are more than one transfer in each test and it helps for each transfer to know the exact balance of accounts after previous transfer to check if the calculations are right

[masihyeganeh]

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

[wojtek-coreum]

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

[miladz68]

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

[miladz68]

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

[wojtek-coreum]

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

[masihyeganeh]

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