Add more tests for drop FT admin feature

[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 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.06%. Comparing base (871f012) to head (83b594d).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #829   +/-   ##
=======================================
  Coverage   37.06%   37.06%           
=======================================
  Files         182      182           
  Lines       52713    52709    -4     
=======================================
- Hits        19536    19535    -1     
+ Misses      29639    29637    -2     
+ Partials     3538     3537    -1     

Flag	Coverage Δ
coreum	32.95% <ø> (+<0.01%)	⬆️
coreum-integration-tests-modules	23.77% <ø> (-0.03%)	⬇️

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: 0 of 2 files reviewed, all discussions resolved (waiting on @miladz68, @wojtek-coreum, and @ysv)

[dzmitryhil]

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

---

x/asset/ft/keeper/before_send.go line 80 at r1 (raw file):

			}

			burnAmount := k.CalculateRate(ctx, def.BurnRate, admin, sender, recipient, coin)

@ysv @miladz68 @wojtek-coreum @masihyeganeh

If we don't have the admin

1. Should we compute and send commission to the extensions (IMO - no)?
2. We defenitelisy shouldn't compute/send commission (both burning and sending) to the admin, since the admin doesn't exist!
   So if we don't need to compute it, that trick with the sdk.AccAddress{} won't be needed.

[miladz68]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)

---

x/asset/ft/keeper/before_send.go line 80 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68 @wojtek-coreum @masihyeganeh

If we don't have the admin

1. Should we compute and send commission to the extensions (IMO - no)?
2. We defenitelisy shouldn't compute/send commission (both burning and sending) to the admin, since the admin doesn't exist!
   So if we don't need to compute it, that trick with the sdk.AccAddress{} won't be needed.

agreed.

[miladz68]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @wojtek-coreum, and @ysv)

---

x/asset/ft/keeper/before_send.go line 80 at r1 (raw file):

Previously, miladz68 (milad) wrote…

agreed.

I mean I agree that we should not compute commision rate anymore. burn rate is fine.

[masihyeganeh]

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

---

x/asset/ft/keeper/before_send.go line 80 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I mean I agree that we should not compute commision rate anymore. burn rate is fine.

Done.

[masihyeganeh]

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

[miladz68]

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

[dzmitryhil]

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

---

x/asset/ft/keeper/before_send.go line 80 at r3 (raw file):

			}

			burnAmount := k.CalculateRate(ctx, def.BurnRate, admin, sender, recipient, coin)

@ysv @miladz68 @wojtek-coreum
WDYT, should we keep it as is, or remove admin and recipient from the k.CalculateRate and add that check here?

What I personally don't like here is that part admin := sdk.AccAddress{}.

Code quote:

			admin := sdk.AccAddress{}
			if def.Admin != "" {
				admin, err = sdk.AccAddressFromBech32(def.Admin)
				if err != nil {
					return sdkerrors.Wrapf(err, "invalid address %s", def.Admin)
				}
			}

			burnAmount := k.CalculateRate(ctx, def.BurnRate, admin, sender, recipient, coin)

---

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

	def.Admin = ""
	if !def.IsFeatureEnabled(types.Feature_extension) {

Please add the comment explaining that here.

---

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

	def.Admin = ""
	if !def.IsFeatureEnabled(types.Feature_extension) {

What about tests?

[masihyeganeh]

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

---

x/asset/ft/keeper/before_send.go line 80 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68 @wojtek-coreum
WDYT, should we keep it as is, or remove admin and recipient from the k.CalculateRate and add that check here?

What I personally don't like here is that part admin := sdk.AccAddress{}.

I like the idea of having all the criterias needed for calculating them in those functions, so I suggest to keep it as is

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Please add the comment explaining that here.

Done.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about tests?

Done.

[miladz68]

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

---

x/asset/ft/keeper/before_send.go line 72 at r5 (raw file):

			}

			admin := sdk.AccAddress{}

as Dima mentioned, why are you assigning admin to empty address ? is it so you can assign value to it in if statement ? if it is so, then try to improve the logic if you can. Maybe move the parsing to inside the calculate function so will parse only if necessary. try to see how it can be improved.

[masihyeganeh]

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

---

x/asset/ft/keeper/before_send.go line 80 at r3 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I like the idea of having all the criterias needed for calculating them in those functions, so I suggest to keep it as is

Done. Let me know if it is better

---

x/asset/ft/keeper/before_send.go line 72 at r5 (raw file):

Previously, miladz68 (milad) wrote…

as Dima mentioned, why are you assigning admin to empty address ? is it so you can assign value to it in if statement ? if it is so, then try to improve the logic if you can. Maybe move the parsing to inside the calculate function so will parse only if necessary. try to see how it can be improved.

Done. Let me know if it is better

[masihyeganeh]

Reviewed 1 of 3 files at r6.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

[masihyeganeh]

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

[masihyeganeh]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

[miladz68]

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

---

x/asset/ft/keeper/before_send.go line 72 at r7 (raw file):

			}

			if def.Admin != "" {

since you are using Must function inside the calculate rate, this function can be safely removed. Actually I see a problem in the logic, look at the next

---

x/asset/ft/keeper/before_send.go line 79 at r7 (raw file):

			}

			burnAmount := k.CalculateRate(ctx, def.BurnRate, def.Admin, sender, recipient, coin)

but burn should be calculated and applied even if there is no admin. so the logic needs adjustment.

[dzmitryhil]

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

---

x/asset/ft/keeper/before_send.go line 80 at r3 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Done. Let me know if it is better

Let's discuss with team.

[masihyeganeh]

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

---

x/asset/ft/keeper/before_send.go line 80 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's discuss with team.

Done.

---

x/asset/ft/keeper/before_send.go line 72 at r7 (raw file):

Previously, miladz68 (milad) wrote…

since you are using Must function inside the calculate rate, this function can be safely removed. Actually I see a problem in the logic, look at the next

Done.

---

x/asset/ft/keeper/before_send.go line 79 at r7 (raw file):

Previously, miladz68 (milad) wrote…

but burn should be calculated and applied even if there is no admin. so the logic needs adjustment.

Done.

[dzmitryhil]

Reviewed 1 of 2 files at r2, 1 of 2 files at r5, 1 of 2 files at r7, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

[miladz68]

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

[masihyeganeh]

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

[dzmitryhil]

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