Implementing features in extension contract

[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.05%. Comparing base (6512e3c) to head (bc5cf2b).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
- Coverage   37.05%   37.05%   -0.01%     
==========================================
  Files         182      182              
  Lines       52694    52698       +4     
==========================================
  Hits        19525    19525              
- Misses      29633    29636       +3     
- Partials     3536     3537       +1     

Flag	Coverage Δ
coreum	32.96% <ø> (+0.11%)	⬆️
coreum-integration-tests-modules	23.76% <ø> (-0.01%)	⬇️

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.

[masihyeganeh]

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

---

integration-tests/modules/assetft_extension_test.go line 270 at r2 (raw file):

Previously, miladz68 (milad) wrote…

extension is not related to signature verification, I think this step can be removed.

Done.

---

integration-tests/modules/assetft_extension_test.go line 551 at r2 (raw file):

Previously, miladz68 (milad) wrote…

you don't need this feature.

Done. But it was in the original test

---

integration-tests/modules/assetft_extension_test.go line 587 at r2 (raw file):

Previously, miladz68 (milad) wrote…

same as above, signature verification is not part of the extension, it is verified in the other test already.

Done.

---

integration-tests/modules/assetft_extension_test.go line 829 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this test does not interact with extesions, so it seems useless to be here. 2.6315789

Done.

---

integration-tests/modules/assetft_extension_test.go line 897 at r2 (raw file):

Previously, miladz68 (milad) wrote…

the extension contract must have the same burn privileges as the admin, meaning that it can burn tokens even if the feature is not enabled. What we want to test here is only that. I think a lot of test cases that are not related.
I can only think of a single meaningful test case here, that the contract is able to burn tokens and assert that burning took place as intended.

Our goal was duplicating all tests we already have, but just add the extension to them. I don't think that it hurts if we have those

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, miladz68 (milad) wrote…

As discussed for minting, there should be only one test case here, that the contract is able to mint tokens if feature is enabled, and assert that minting happened.

I believe that we should test mintable and non-mintable tokens here. But I agree that tests in which the extension is not involved are not needed

---

integration-tests/modules/assetft_extension_test.go line 1245 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this check does not interact with extensions, it is not a meaningful check IMO.

Done.

---

integration-tests/modules/assetft_extension_test.go line 1259 at r2 (raw file):

Previously, miladz68 (milad) wrote…

we don't want to test minting, we know that it works. we just want to check that the extension contract has privileges to mint tokens if the feature is enabled.

I don't agree. It's an integration test, so if the feature is enabled, it should be able to mint which also means that the contract had the privileges to mint

---

x/asset/ft/keeper/keeper.go line 690 at r2 (raw file):

Previously, miladz68 (milad) wrote…

only admin can transfer admin role, the extension contract is not allowed to do that.

Done.

---

x/asset/ft/keeper/keeper.go line 722 at r2 (raw file):

Previously, miladz68 (milad) wrote…

only admin can do this, not the extension contract.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 160 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this case does not interact with extensions, it should be removed

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 164 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this case does not interact with extensions, it should be removed

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 169 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this case does not interact with extensions, it should be removed

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 174 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this case does not interact with extensions, it should be removed

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 178 at r2 (raw file):

Previously, miladz68 (milad) wrote…

this case does not interact with extensions, it should be removed

This one does, doesn't it?

---

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

Previously, miladz68 (milad) wrote…

please add this check now. it must check that the attached funds contain the amount.

Done.

---

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

Previously, miladz68 (milad) wrote…

I guess your Admin PR is merged so you should be able to fix it now.

It is not merged yet:
#827

---

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

Previously, miladz68 (milad) wrote…

this is not accurate, the condition is that the user is not allowed to burn their frozen balance.
here if you only need check that sender's current balance is above their freezing balance and reject the transaction if it is not. because all the money that must be deduced from the sender's balance is already deducted and given to smart contract.

The TODO is exactly copied from the doc

---

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

Previously, miladz68 (milad) wrote…

the smart contract will be able to burn, no matter the feature is enable or not.

It is exactly what you wrote. Isn't it?

[dzmitryhil]

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

---

integration-tests/modules/assetft_extension_test.go line 293 at r3 (raw file):

	multiSendMsg := &banktypes.MsgMultiSend{
		Inputs:  []banktypes.Input{{Address: issuer.String(), Coins: coinsToSend}},
		Outputs: []banktypes.Output{{Address: recipient.String(), Coins: coinsToSend}},

If you want to test the MsgMultiSend use multiple recipients.
I would propose to use both MsgSend with single recipient and MsgMultiSend with multiple.

---

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

    // check that amount is present in the attached funds, and attached funds is enough
    // to cover the transfer.
    let attached_fund = query_bank_balance(

The variable name is not correct, the contract_balance is better name here.

---

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

        }

        if features.contains(&assetft::BLOCK_SMART_CONTRACTS) {

IMO no need to check that feature, since you won't be able to call the contract if the feature is enabled :-)
@miladz68 @ysv @wojtek-coreum WDYT ?

---

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

        // meaningful check.
        if amount == Uint128::new(101) {
            return assert_burning(

Could you please explain the assert_burning and assert_minting usage here?
Because if the burning/minting is disabled no matter whether you do that check here or not, the transaction should fail. So seems that part is not needed.

---

x/asset/ft/types/token.go line 206 at r3 (raw file):

// HasAdminPrivileges returns true if the addr is the admin or the asset extension contract address.
func (def Definition) HasAdminPrivileges(addr sdk.Address) bool {

nit: What was wrong with prev name?

[dzmitryhil]

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

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO no need to check that feature, since you won't be able to call the contract if the feature is enabled :-)
@miladz68 @ysv @wojtek-coreum WDYT ?

I mean if the extensions are enabled it makes no sense to allow the BLOCK_SMART_CONTRACTS feature to be enabled.

---

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

        if amount == Uint128::new(101) {
            return assert_burning(
                info.sender.as_ref(), amount, &token, features.contains(&assetft::BURNING)

IMO what is possible to test here is that you can mint/burn when you intercept the send. For example, you can define at the time of the installation and addresses, and if you send from that address you burn the tokens if you send to that address mint x2. Or any other example. You can ask @miladz68 probably it's a part of a different task?!

[miladz68]

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

---

integration-tests/modules/assetft_extension_test.go line 897 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Our goal was duplicating all tests we already have, but just add the extension to them. I don't think that it hurts if we have those

Actually our goal was to test extensions, and we took the code from previous tests so we don't write it from scratch.

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I believe that we should test mintable and non-mintable tokens here. But I agree that tests in which the extension is not involved are not needed

agreed mintable and non-mintable tokens should both be tested.

---

integration-tests/modules/assetft_extension_test.go line 1259 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I don't agree. It's an integration test, so if the feature is enabled, it should be able to mint which also means that the contract had the privileges to mint

the minting part is already tested, and the mint message does not interact with extensions.

---

integration-tests/modules/assetft_extension_test.go line 1221 at r3 (raw file):

// TestAssetFTExtensionSendingToSmartContractIsDenied verifies that this is not possible to send token to smart contract
// if issuer blocked this operation.
func TestAssetFTExtensionSendingToSmartContractIsDenied(t *testing.T) {

Just a reminder, if you keep adding features on top of this PR, it will get so big that you will have hard time finalizing it.

---

integration-tests/modules/assetft_extension_test.go line 1329 at r3 (raw file):

// TestAssetFTExtensionAttachingToSmartContractIsDenied verifies that this is not possible to attach token to smart
// contract call if issuer blocked this operation.
func TestAssetFTExtensionAttachingToSmartContractCallIsDenied(t *testing.T) {

smart test case 👍

---

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

The TODO is exactly copied from the doc

yes, the external behavior is as described in the doc, meaning cannot burn. but this is not the user burning the token, it is extension burning the token. the extension can burn the tokens that is in its custody as it sees fit. since the user transferred their tokens to the custody of the extension, the only condition that should be checked here, is that the balance of the user after this transfer is not below their frozen balance.

---

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is exactly what you wrote. Isn't it?

what part did I write ? could you be more specific ?

---

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

    // check that amount is present in the attached funds, and attached funds is enough
    // to cover the transfer.
    let attached_fund = query_bank_balance(

don't query bank balance of the contract, inspect attached funds. it is info.funds.

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

I mean if the extensions are enabled it makes no sense to allow the BLOCK_SMART_CONTRACTS feature to be enabled.

it does make sense, we block all other contracts, except the extension contract.

---

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

    // TODO(masih): The user cannot burn the frozen amount if both
    // freezing and burning is enabled.
    if !burning_enabled && sender != token.issuer {

you don't need this check to be able to check that burning works ok. just send 101 tokens with 2 scenarios, burnalbe and non-burnable. In both cases burning should have been done, make sure it was burnt correctly by inspecting the events and total supply.

---

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

    // TODO: Do we need this?
    let issued_from_smart_contract = is_smart_contract(deps, &token.issuer);

I don't think you need this check, but the extension smart contract must be an exception just like the admin.

---

x/asset/ft/keeper/test-contracts/asset-extension/src/error.rs line 27 at r3 (raw file):

    InsufficientFunds {},

    // TODO: Delete this one

apply the TODO.

[masihyeganeh]

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

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, miladz68 (milad) wrote…

agreed mintable and non-mintable tokens should both be tested.

Done.

---

integration-tests/modules/assetft_extension_test.go line 1259 at r2 (raw file):

Previously, miladz68 (milad) wrote…

the minting part is already tested, and the mint message does not interact with extensions.

But it is not calling the native mint method of token. It is simulated with a magic amount and it is actually happened in the extension. So, I believe we need this test. Let me know if you insist on removing it

---

integration-tests/modules/assetft_extension_test.go line 1221 at r3 (raw file):

Previously, miladz68 (milad) wrote…

Just a reminder, if you keep adding features on top of this PR, it will get so big that you will have hard time finalizing it.

You're right. I was actually implementing it while you were reviewing. I won't implement other features in this branch

---

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

Previously, miladz68 (milad) wrote…

yes, the external behavior is as described in the doc, meaning cannot burn. but this is not the user burning the token, it is extension burning the token. the extension can burn the tokens that is in its custody as it sees fit. since the user transferred their tokens to the custody of the extension, the only condition that should be checked here, is that the balance of the user after this transfer is not below their frozen balance.

So, I'll delete the TODO

---

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

Previously, miladz68 (milad) wrote…

what part did I write ? could you be more specific ?

I mean, if the sender of the bankSend is the issuer (or admin) of the token, the smart contract can burn it. If I let all of the smart contract calls be able to burn, then any user that sends a bankSend can burn. Let me know if I'm not clear enough

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

The variable name is not correct, the contract_balance is better name here.

Done.

---

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

Previously, miladz68 (milad) wrote…

don't query bank balance of the contract, inspect attached funds. it is info.funds.

Done.

---

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

Previously, miladz68 (milad) wrote…

it does make sense, we block all other contracts, except the extension contract.

I agree with Milad

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Could you please explain the assert_burning and assert_minting usage here?
Because if the burning/minting is disabled no matter whether you do that check here or not, the transaction should fail. So seems that part is not needed.

If the asset extension feature is enabled, those checks should happen in the extension, but enabling or disabling them still happens using those feature flags

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO what is possible to test here is that you can mint/burn when you intercept the send. For example, you can define at the time of the installation and addresses, and if you send from that address you burn the tokens if you send to that address mint x2. Or any other example. You can ask @miladz68 probably it's a part of a different task?!

This is exactly what you suggest but when the amount is equal to a magic number, instead of a specific account. Both is possible, but this one lets minting to more accounts and you can test more situations

---

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

Previously, miladz68 (milad) wrote…

you don't need this check to be able to check that burning works ok. just send 101 tokens with 2 scenarios, burnalbe and non-burnable. In both cases burning should have been done, make sure it was burnt correctly by inspecting the events and total supply.

But it is testing more than that. It tests that non-burnables won't burn, but for burnable, you can decide who can burn it. Let me know if you insist on removing it

---

x/asset/ft/keeper/test-contracts/asset-extension/src/error.rs line 27 at r3 (raw file):

Previously, miladz68 (milad) wrote…

apply the TODO.

Will do then I finalize this feature

---

x/asset/ft/types/token.go line 206 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

nit: What was wrong with prev name?

It now checks for both token admin and asset extension admin. It's a new method. We will have the IsAdmin one

[miladz68]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @dzmitryhil, @i, @masihyeganeh, @wojtek-coreum, and @ysv)

a discussion (no related file):
Run cargo fmt at the root of the cargo project to make sure that the files are properly formatted.

---

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Done.

you are not testing that trying to mint non-mintable tokens are correctly rejected by the go code. you only reject it in the smart contract which is not what we intend to test.

---

integration-tests/modules/assetft_extension_test.go line 1259 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

But it is not calling the native mint method of token. It is simulated with a magic amount and it is actually happened in the extension. So, I believe we need this test. Let me know if you insist on removing it

but the code below the comment mint tokens and check balance and total supply only tests the native code, which is already tested. I am not saying to remove the entire test, but the parts that don't affect and are not affected by extensions don't need to be tested.

---

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

I mean, if the sender of the bankSend is the issuer (or admin) of the token, the smart contract can burn it. If I let all of the smart contract calls be able to burn, then any user that sends a bankSend can burn. Let me know if I'm not clear enough

you are clear, and that is the behavior that we want to test, so remove this check.

---

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

But it is testing more than that. It tests that non-burnables won't burn, but for burnable, you can decide who can burn it. Let me know if you insist on removing it

asset extension must be able to burn both burnables and non-burnables, so the check should be removed.

[masihyeganeh]

Reviewable status: 10 of 16 files reviewed, 19 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, miladz68 (milad) wrote…

Run cargo fmt at the root of the cargo project to make sure that the files are properly formatted.

Done.

---

---

integration-tests/modules/assetft_extension_test.go line 897 at r2 (raw file):

Previously, miladz68 (milad) wrote…

Actually our goal was to test extensions, and we took the code from previous tests so we don't write it from scratch.

Done.

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, miladz68 (milad) wrote…

you are not testing that trying to mint non-mintable tokens are correctly rejected by the go code. you only reject it in the smart contract which is not what we intend to test.

Done.

---

integration-tests/modules/assetft_extension_test.go line 293 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

If you want to test the MsgMultiSend use multiple recipients.
I would propose to use both MsgSend with single recipient and MsgMultiSend with multiple.

To be honest, this test is copied from the whitelist one to make sure the asset extension is compatible with that

---

x/asset/ft/keeper/keeper.go line 1073 at r2 (raw file):

Previously, miladz68 (milad) wrote…

I think it should be possible to clawback extension contract balance. let's dicuss in the team.

Done. But in another PR

---

x/asset/ft/keeper/keeper_extension_test.go line 235 at r2 (raw file):

Previously, miladz68 (milad) wrote…

add a special condition into the smart contract to ignore default whitelisting, so we will know that we are able to overwrite the default behavior via smart contract.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 238 at r2 (raw file):

Previously, miladz68 (milad) wrote…

same as whitelisting, remove the conditions that do not interact with the extension. and add a special condition that shows we are able to overwrite the default behavior via smart contract.
Same goes for minting and burning.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 438 at r2 (raw file):

Previously, miladz68 (milad) wrote…

Should be fixed as commented above.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 597 at r2 (raw file):

Previously, miladz68 (milad) wrote…

Should be fixed as commented above.

Done.

---

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

Previously, miladz68 (milad) wrote…

you are clear, and that is the behavior that we want to test, so remove this check.

Done.

---

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

Previously, miladz68 (milad) wrote…

maybe remove this check, and let the go code catch and return this error, and also test for it. it will be an interesting test case.

Done.

---

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

Previously, miladz68 (milad) wrote…

asset extension must be able to burn both burnables and non-burnables, so the check should be removed.

Done.

---

x/asset/ft/keeper/test-contracts/asset-extension/src/error.rs line 27 at r3 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Will do then I finalize this feature

Done.

[miladz68]

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

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Done.

you are still returning the error on the contract level and not the go level.

---

integration-tests/modules/assetft_extension_test.go line 293 at r3 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

To be honest, this test is copied from the whitelist one to make sure the asset extension is compatible with that

but I guess it makes sense to test multiple tokens via multisend, one with extension and one without.

---

integration-tests/modules/assetft_extension_test.go line 31 at r6 (raw file):

const (
	MagicAmountDisallowed         = 7

magic is not a good name maybe TriggerAmountDisallowed or AmountDisallowedTrigger

---

x/asset/ft/keeper/keeper_extension_test.go line 26 at r6 (raw file):

const (
	MagicAmountDisallowed         = 7

import these in integration tests if they are accessible, so you are not duplicating them.

---

x/asset/ft/keeper/keeper_extension_test.go line 223 at r6 (raw file):

	// sending magic amount will be transferred despise whitelisted amount being exceeded
	err = bankKeeper.SendCoins(ctx, issuer, recipient, sdk.NewCoins(
		sdk.NewCoin(denom, sdkmath.NewInt(MagicAmountIgnoreWhitelisting))),

what is the value of balance and whitelisted balance here ?

---

x/asset/ft/keeper/keeper_extension_test.go line 324 at r6 (raw file):

	// send magic amount to transfer despite freezing
	err = bankKeeper.SendCoins(ctx, recipient, issuer, sdk.NewCoins(

what is the value of (balance - frozen) here ?

---

x/asset/ft/keeper/keeper_extension_test.go line 438 at r6 (raw file):

	// try to burn as non-issuer
	err = bankKeeper.SendCoins(ctx, recipient, issuer, sdk.NewCoins(

also check total supply.

---

x/asset/ft/keeper/keeper_extension_test.go line 458 at r6 (raw file):

	// burn tokens and check balance and total supply
	err = bankKeeper.SendCoins(ctx, issuer, issuer, sdk.NewCoins(
		sdk.NewCoin(burnableDenom, sdkmath.NewInt(MagicAmountBurning))),

also check total supply.

---

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

    });

    if token.admin != Some(sender.to_string()) {

remove this check, let the error to be thrown by the go code, so we will know that interface works.

[masihyeganeh]

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

---

integration-tests/modules/assetft_extension_test.go line 31 at r6 (raw file):

Previously, miladz68 (milad) wrote…

magic is not a good name maybe TriggerAmountDisallowed or AmountDisallowedTrigger

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 26 at r6 (raw file):

Previously, miladz68 (milad) wrote…

import these in integration tests if they are accessible, so you are not duplicating them.

I don't think I can import them from keeper test. I can have them in a separate file somewhere that both of them can access, but it is not a good idea I think

---

x/asset/ft/keeper/keeper_extension_test.go line 223 at r6 (raw file):

Previously, miladz68 (milad) wrote…

what is the value of balance and whitelisted balance here ?

Done. Added the check for those values

---

x/asset/ft/keeper/keeper_extension_test.go line 324 at r6 (raw file):

Previously, miladz68 (milad) wrote…

what is the value of (balance - frozen) here ?

Done. Added the check for those values

---

x/asset/ft/keeper/keeper_extension_test.go line 438 at r6 (raw file):

Previously, miladz68 (milad) wrote…

also check total supply.

Done.

---

x/asset/ft/keeper/keeper_extension_test.go line 458 at r6 (raw file):

Previously, miladz68 (milad) wrote…

also check total supply.

Done.

[masihyeganeh]

Reviewable status: 13 of 16 files reviewed, 15 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

---

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

Previously, miladz68 (milad) wrote…

remove this check, let the error to be thrown by the go code, so we will know that interface works.

It will not throw any error, because the extension will mint the amount and it has privigates to do it. Am I wrong here?

[masihyeganeh]

Reviewable status: 13 of 16 files reviewed, 15 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

---

integration-tests/modules/assetft_extension_test.go line 1105 at r2 (raw file):

Previously, miladz68 (milad) wrote…

you are still returning the error on the contract level and not the go level.

Done.

---

integration-tests/modules/assetft_extension_test.go line 1259 at r2 (raw file):

Previously, miladz68 (milad) wrote…

but the code below the comment mint tokens and check balance and total supply only tests the native code, which is already tested. I am not saying to remove the entire test, but the parts that don't affect and are not affected by extensions don't need to be tested.

Done.

[miladz68]

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

[masihyeganeh]

Reviewable status: 15 of 16 files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

---

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

Previously, miladz68 (milad) wrote…

I don't think you need this check, but the extension smart contract must be an exception just like the admin.

Done.

[masihyeganeh]

Reviewed 1 of 3 files at r2, 6 of 12 files at r3, 3 of 6 files at r4, 2 of 5 files at r5, 1 of 4 files at r6, 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

[masihyeganeh]

Reviewable status: 15 of 16 files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

---

integration-tests/modules/assetft_extension_test.go line 293 at r3 (raw file):

Previously, miladz68 (milad) wrote…

but I guess it makes sense to test multiple tokens via multisend, one with extension and one without.

Done.

[masihyeganeh]

Reviewed 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @i, @miladz68, @wojtek-coreum, and @ysv)

[miladz68]

Reviewed 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @i, @wojtek-coreum, and @ysv)

[dzmitryhil]

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