Implement Clawback feature for assetft #804

masihyeganeh · 2024-04-12T11:57:08Z

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

dzmitryhil

Reviewable status: 0 of 17 files reviewed, 9 unresolved discussions (waiting on @masihyeganeh, @miladz68, @wojtek-coreum, and @ysv)

integration-tests/modules/assetft_test.go line 2130 at r1 (raw file):

	)
	requireT.Error(err)
	assertT.True(cosmoserrors.ErrUnauthorized.Is(err))

You can use ErrorIs of the assertT.

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

// Clawback confiscates specified token from the specified account.
func (k Keeper) Clawback(ctx sdk.Context, sender, addr sdk.AccAddress, coin sdk.Coin) error {

I would call sender -> issuer here to make it similar to to other such functions, where the action is allowed for the issuer only (same in the interface in the msg_server).

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

}

func (k Keeper) clawbackChecks(ctx sdk.Context, sender, addr sdk.AccAddress, coin sdk.Coin) error {

nit: I would propose to call the func validateClawbackAllowed or similar.

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

}

func (k Keeper) clawbackChecks(ctx sdk.Context, sender, addr sdk.AccAddress, coin sdk.Coin) error {

IMO there we should also check that clawback is prohibited for

module accounts
IBC escrow addresses
WASM contract - not sure, better to discuss (for example for the bridge it's critical to prohibit, can be solved by not registering such tokens)
groups accounts - not sure, better to discuss
probably, others - better to discuss

@ysv @miladz68 WDYT ?

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

	}

	if def.IsIssuer(addr) {

Also, you need to check that the token's issuer is the sender.

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

	}

	return def.CheckFeatureAllowed(sender, types.Feature_clawback)

nit: I propose to put it to the first place in the func

x/asset/ft/keeper/keeper_test.go line 1267 at r1 (raw file):

}

func TestKeeper_Clawback(t *testing.T) {

You don't test, from what I see.

if def.IsIssuer(addr) {  
    return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "issuer's balance can't be clawed back")  
}

x/asset/ft/types/msgs.go line 436 at r1 (raw file):

}

func (m MsgClawback) ValidateBasic() error {

Missing unit test for the func. Check the unit tests of other ValidateBasic.

x/deterministicgas/config.go line 86 at r1 (raw file):

		MsgToMsgURL(&assetfttypes.MsgGloballyFreeze{}):      constantGasFunc(5_000),
		MsgToMsgURL(&assetfttypes.MsgGloballyUnfreeze{}):    constantGasFunc(5_000),
		MsgToMsgURL(&assetfttypes.MsgClawback{}):            constantGasFunc(73_500),

How did you compute that value? If you took the gas spent, then it's not correct. You need to sub the FixedGas: 65000.

codecov · 2024-04-16T08:54:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.58%. Comparing base (c15e198) to head (69bd51a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   36.84%   35.58%   -1.27%     
==========================================
  Files         165      165              
  Lines       48383    48956     +573     
==========================================
- Hits        17827    17420     -407     
- Misses      27189    28149     +960     
- Partials     3367     3387      +20

Flag	Coverage Δ
coreum	`32.46% <ø> (+<0.01%)`	⬆️
coreum-integration-tests-modules	`17.05% <ø> (-5.82%)`	⬇️

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: 0 of 21 files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

integration-tests/modules/assetft_test.go line 2130 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can use ErrorIs of the assertT.

Almost all of the errors handled like this in the integration test. I can either change all of them, or not change this. Let's decide

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would call sender -> issuer here to make it similar to to other such functions, where the action is allowed for the issuer only (same in the interface in the msg_server).

I agree, but there is another task that I'm working on to change the issuer to admin. I can either rename it there or just keep it like this, so it can be valid for both cases

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

nit: I would propose to call the func validateClawbackAllowed or similar.

I see that other functions that check the same thing are called like this. Let me know if I should unify them like what you suggested, or leave it like this

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO there we should also check that clawback is prohibited for

module accounts

IBC escrow addresses

WASM contract - not sure, better to discuss (for example for the bridge it's critical to prohibit, can be solved by not registering such tokens)

groups accounts - not sure, better to discuss

probably, others - better to discuss

@ysv @miladz68 WDYT ?

I asked @miladz68 about the IBC and WASM, and he said that this feature is not used there. Correct me if I'm wrong here

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Also, you need to check that the token's issuer is the sender.

It is already checking in IsFeatureAllowed

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

nit: I propose to put it to the first place in the func

We need the def to call this, so only thing that remains is the def.IsIssuer that comes before this. I'm not sure if it gives any improvement

x/asset/ft/keeper/keeper_test.go line 1267 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You don't test, from what I see.

if def.IsIssuer(addr) {  
    return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "issuer's balance can't be clawed back")  
}

Done.

x/asset/ft/types/msgs.go line 436 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Missing unit test for the func. Check the unit tests of other ValidateBasic.

Done.

x/deterministicgas/config.go line 86 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

How did you compute that value? If you took the gas spent, then it's not correct. You need to sub the FixedGas: 65000.

Yeah, this is one of the problems I faced. I used some small number here and let the tests run. It failed but said how much gas actually used. But when I subtracted the 65k from it, it failed again. The 73500 worked. But now I changed it back to 8500 and it works.
So it's fixed.

dzmitryhil

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

integration-tests/modules/assetft_test.go line 2130 at r1 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Almost all of the errors handled like this in the integration test. I can either change all of them, or not change this. Let's decide

No need to update them for others, that approach is better, and for new tests we used it.

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

I agree, but there is another task that I'm working on to change the issuer to admin. I can either rename it there or just keep it like this, so it can be valid for both cases

Right, so you will rename the issuer, not the sender. So this attribute will be renamed to admin.

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

I see that other functions that check the same thing are called like this. Let me know if I should unify them like what you suggested, or leave it like this

I see only one freezingChecks. Let's discuss this with the team if you wish.
Check the https://reviewable.io/reviews/CoreumFoundation/coreum/800#-NvaJ0AW6OsyMyMgsgNV discussion it is relevant IMO.

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

I asked @miladz68 about the IBC and WASM, and he said that this feature is not used there. Correct me if I'm wrong here

WDYM @miladz68 ?

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is already checking in IsFeatureAllowed

I see, right.

miladz68

Reviewed 12 of 17 files at r1, 8 of 9 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, @wojtek-coreum, and @ysv)

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Right, so you will rename the issuer, not the sender. So this attribute will be renamed to admin.

I think sender is more correct, the assertion that sender is equal to admin/issuer is part of logic, it is not needed to be reflected in the var name.

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

WDYM @miladz68 ?

module accounts and escrow address must be prohibited.
I don't think wasm and groups should be prohibited.

x/asset/ft/keeper/keeper_test.go line 1267 at r4 (raw file):

}

func TestKeeper_Clawback(t *testing.T) {

more conditions to test
clawback frozen token must succeed
clawback from module account must fail
clawback from ibc escrow address must fail
clawback from group account as dima mentioned

x/asset/ft/types/msgs.go line 452 at r4 (raw file):

	if issuer.String() == m.Account {
		return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "issuer's balance can't be clawed back")

this should not be in validate basics, it belongs to the keeper.

x/asset/ft/types/msgs.go line 456 at r4 (raw file):

	if issuer.String() != m.Sender {
		return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "only issuer can claw back balance")

this check does not belong here, it belongs to the keeper.

dzmitryhil

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

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

Previously, miladz68 (milad) wrote…

I think sender is more correct, the assertion that sender is equal to admin/issuer is part of logic, it is not needed to be reflected in the var name.

I agree, that in general it might be sender, but we used issuer in such cases, that's why I proposed. Let's discuss with the team.

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

Previously, miladz68 (milad) wrote…

module accounts and escrow address must be prohibited.
I don't think wasm and groups should be prohibited.

Let's discuss with the team.

masihyeganeh

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

integration-tests/modules/assetft_test.go line 2130 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

No need to update them for others, that approach is better, and for new tests we used it.

Done.

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

Previously, miladz68 (milad) wrote…

I think sender is more correct, the assertion that sender is equal to admin/issuer is part of logic, it is not needed to be reflected in the var name.

I agree with Milad. I imagine that "sender" means the person who sent the transaction/message. In the body of the logic we need to decide who it actually is

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's discuss with the team.

It is decided that we should only prohibit module accounts

x/asset/ft/keeper/keeper_test.go line 1267 at r4 (raw file):

Previously, miladz68 (milad) wrote…

more conditions to test
clawback frozen token must succeed
clawback from module account must fail
clawback from ibc escrow address must fail
clawback from group account as dima mentioned

It is decided that we should only prohibit module accounts. other than that, what should happen when you clawback a frozen token? should it unfreeze first?

x/asset/ft/types/msgs.go line 452 at r4 (raw file):

Previously, miladz68 (milad) wrote…

this should not be in validate basics, it belongs to the keeper.

I was mimicking the freeze feature. So, it should change too?

x/asset/ft/types/msgs.go line 456 at r4 (raw file):

Previously, miladz68 (milad) wrote…

this check does not belong here, it belongs to the keeper.

I was mimicking the freeze feature. So, it should change too?

masihyeganeh

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

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is decided that we should only prohibit module accounts

Done.

x/asset/ft/keeper/keeper_test.go line 1267 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is decided that we should only prohibit module accounts. other than that, what should happen when you clawback a frozen token? should it unfreeze first?

Done.

x/asset/ft/types/msgs.go line 452 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I was mimicking the freeze feature. So, it should change too?

Done.

x/asset/ft/types/msgs.go line 456 at r4 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

I was mimicking the freeze feature. So, it should change too?

Done.

miladz68

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

ysv

Reviewed 5 of 17 files at r1, 4 of 9 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, and @wojtek-coreum)

a discussion (no related file):
partially reviewed
will continue the review

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

// TestAssetFTClawback checks clawback functionality of fungible tokens.
func TestAssetFTClawback(t *testing.T) {

looks good but we will probably have to add more tests for IBC escrow & WASM & Module account.
Unless we do all of them in unit tests

Code quote:

TestAssetFTClawback

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

	issuer := chain.GenAccount()
	from := chain.GenAccount()

nit: usually we call it sender I think

Code quote:

from

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

	)
	requireT.Error(err)
	assertT.True(cosmoserrors.ErrUnauthorized.Is(err))

I'm wondering if we get ErrUnauthorized in this case

Because this address doesn't exist on chain (since it is not funded) so failure should happen on estimation step, shouldn't it ?

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

	fungibleTokenClawbackEvts, err := event.FindTypedEvents[*assetfttypes.EventAmountClawedBack](res.Events)
	requireT.NoError(err)
	assertT.EqualValues(&assetfttypes.EventAmountClawedBack{

I can see that clawback is a single word

Shouldn't it be just Clawback everywhere instead of clawBack ?

Pls check all places

Code quote:

EventAmountClawedBack

dzmitryhil

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

masihyeganeh

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

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

Previously, ysv (Yaroslav Savchuk) wrote…

looks good but we will probably have to add more tests for IBC escrow & WASM & Module account.
Unless we do all of them in unit tests

I actually have unit test for module account. But I'm not sure how to test the other two. And I'm not sure if it is needed or not. Because there is no restriction on any other kind of accounts

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

Previously, ysv (Yaroslav Savchuk) wrote…

nit: usually we call it sender I think

This one is a little different. You are clawbacking "from" an account. So, It's not "sender". But if "from" is not good enough, I can change it to "account" maybe

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

Previously, ysv (Yaroslav Savchuk) wrote…

I'm wondering if we get ErrUnauthorized in this case

Because this address doesn't exist on chain (since it is not funded) so failure should happen on estimation step, shouldn't it ?

We actually get ErrUnauthorized (as you can see that this test passes), because it is checked that the sender is actually the issuer of the token in checks

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

Previously, ysv (Yaroslav Savchuk) wrote…

I can see that clawback is a single word

Shouldn't it be just Clawback everywhere instead of clawBack ?

Pls check all places

At least my IDE is not happy when I'm handling it like a single word.
Let me know if you still believe that it should not be separated

dzmitryhil

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

miladz68

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

ysv

Reviewed 5 of 17 files at r1, 1 of 1 files at r3, 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @wojtek-coreum)

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

Because there is no restriction on any other kind of accounts

I would still test this since they are different type of accounts and might behave differently.
@miladz68 @dzmitryhil WDYT ?

Also it might make sense to check behaviour of vesting accounts in such case

But I'm not sure how to test the other two

It is for sure doable via integration tests but maybe we can do it somehow simpler via unit

Lets discuss on call

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

This one is a little different. You are clawbacking "from" an account. So, It's not "sender". But if "from" is not good enough, I can change it to "account" maybe

I see, then sender is not ok.

Maybe account

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

We actually get ErrUnauthorized (as you can see that this test passes), because it is checked that the sender is actually the issuer of the token in checks

yeah tests succeed and I'm confused why

that is strange since account existence is checked on ante handler which runs before keeper logic. So it should fail on acc existence step IMO.
And I checked you don't verify issuer on ValidateBasic.

Maybe I misunderstood smth
Could you pls dig dipper and check ?

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

At least my IDE is not happy when I'm handling it like a single word.
Let me know if you still believe that it should not be separated

I prefer Clawbacked since we use Clawback everywhere as single word.
IMO better for consistency

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

	if def.IsIssuer(addr) {
		return sdkerrors.Wrap(cosmoserrors.ErrUnauthorized, "issuer's balance can't be clawed back")

this is more question for next PR:

How it should behave once we split admin & issuer.
Maybe for issuer it should behave the same way as for other addresses ? It is like sending to yourself of bank send it is not prohibited

Lets at least leave todo here for the next PR

miladz68

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

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

Previously, ysv (Yaroslav Savchuk) wrote…

Because there is no restriction on any other kind of accounts

I would still test this since they are different type of accounts and might behave differently.
@miladz68 @dzmitryhil WDYT ?

Also it might make sense to check behaviour of vesting accounts in such case

But I'm not sure how to test the other two

It is for sure doable via integration tests but maybe we can do it somehow simpler via unit

Lets discuss on call

adding wasm tests won't hurt. if you question on how to get wasm accounts, let me know. for IBC a new test should be added to the IBC folder I guess.

masihyeganeh

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

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

Previously, miladz68 (milad) wrote…

adding wasm tests won't hurt. if you question on how to get wasm accounts, let me know. for IBC a new test should be added to the IBC folder I guess.

Done.

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

Previously, ysv (Yaroslav Savchuk) wrote…

yeah tests succeed and I'm confused why

that is strange since account existence is checked on ante handler which runs before keeper logic. So it should fail on acc existence step IMO.
And I checked you don't verify issuer on ValidateBasic.

Maybe I misunderstood smth
Could you pls dig dipper and check ?

It is checked here:

coreum/x/asset/ft/types/token.go

Line 186 in aa9144d

if def.IsIssuer(addr) {

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

Previously, ysv (Yaroslav Savchuk) wrote…

I prefer Clawbacked since we use Clawback everywhere as single word.
IMO better for consistency

Let's stick to current implementation

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

Previously, ysv (Yaroslav Savchuk) wrote…

this is more question for next PR:

How it should behave once we split admin & issuer.
Maybe for issuer it should behave the same way as for other addresses ? It is like sending to yourself of bank send it is not prohibited

Lets at least leave todo here for the next PR

It is already fixed in the next PR. IsIssuer is replaced by IsAdmin. Will change this too in that PR

miladz68

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

dzmitryhil

Reviewed 9 of 17 files at r1, 4 of 9 files at r2, 1 of 1 files at r3, 8 of 9 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @wojtek-coreum and @ysv)

ysv

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

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

Previously, masihyeganeh (Masih Yeganeh) wrote…

It is already fixed in the next PR. IsIssuer is replaced by IsAdmin. Will change this too in that PR

but just changing this line to IsAdmin is not enough IMO
We should change logic here I think

For this PR it is ok to leave just TODO and we can discuss logic in the next one

masihyeganeh

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

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

Previously, ysv (Yaroslav Savchuk) wrote…

but just changing this line to IsAdmin is not enough IMO
We should change logic here I think

For this PR it is ok to leave just TODO and we can discuss logic in the next one

Done.

ysv

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

miladz68

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

masihyeganeh

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

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

partially reviewed
will continue the review

Done?

masihyeganeh

Reviewed 9 of 17 files at r1, 4 of 9 files at r2, 1 of 1 files at r3, 7 of 9 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

masihyeganeh

Reviewed 4 of 4 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

ysv

Reviewed 4 of 4 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

a discussion (no related file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

Done?

yes

masihyeganeh requested a review from a team as a code owner April 12, 2024 11:57

masihyeganeh requested review from dzmitryhil, miladz68, ysv and wojtek-coreum and removed request for a team April 12, 2024 11:57

dzmitryhil suggested changes Apr 16, 2024

View reviewed changes

masihyeganeh force-pushed the masih/clawback branch from 3c8d695 to 81c1be1 Compare April 16, 2024 08:39

masihyeganeh commented Apr 16, 2024

View reviewed changes

dzmitryhil suggested changes Apr 16, 2024

View reviewed changes

miladz68 requested changes Apr 17, 2024

View reviewed changes

masihyeganeh added 6 commits April 17, 2024 14:42

Implement Clawback feature for assetft

af95c4c

Add test for clawback from issuer

0212b8d

Fixed issues mentioned in review

b67c44c

Ignore mac files

cf70aa7

Fix deterministicgas unit test

2b8ed7c

Fix ValidateBasic unit tests

b88b052

dzmitryhil suggested changes Apr 17, 2024

View reviewed changes

masihyeganeh commented Apr 18, 2024

View reviewed changes

Fix changes requested in review

cd94cac

masihyeganeh force-pushed the masih/clawback branch from 8ed5956 to cd94cac Compare April 18, 2024 06:32

masihyeganeh added 2 commits April 18, 2024 10:10

Fix linting

4ecce23

Fix integration test

8470580

miladz68 approved these changes Apr 18, 2024

View reviewed changes

ysv requested a review from dzmitryhil April 18, 2024 11:15

ysv requested changes Apr 18, 2024

View reviewed changes

dzmitryhil approved these changes Apr 18, 2024

View reviewed changes

masihyeganeh commented Apr 19, 2024

View reviewed changes

dzmitryhil requested a review from ysv April 19, 2024 11:40

dzmitryhil previously approved these changes Apr 19, 2024

View reviewed changes

miladz68 approved these changes Apr 23, 2024

View reviewed changes

ysv requested review from dzmitryhil and miladz68 April 23, 2024 16:38

ysv requested changes Apr 23, 2024

View reviewed changes

miladz68 previously approved these changes Apr 25, 2024

View reviewed changes

Add more integration tests

cd64746

masihyeganeh commented Apr 25, 2024

View reviewed changes

masihyeganeh dismissed stale reviews from miladz68 and dzmitryhil via cd64746 April 25, 2024 13:34

Fix integration test

0960ac0

miladz68 previously approved these changes Apr 26, 2024

View reviewed changes

dzmitryhil requested a review from ysv April 26, 2024 09:52

dzmitryhil reviewed Apr 26, 2024

View reviewed changes

ysv requested changes Apr 26, 2024

View reviewed changes

Add TODO comment

5f2a4dd

masihyeganeh dismissed miladz68’s stale review via 5f2a4dd April 26, 2024 15:49

masihyeganeh commented Apr 26, 2024

View reviewed changes

ysv approved these changes Apr 26, 2024

View reviewed changes

miladz68 approved these changes Apr 29, 2024

View reviewed changes

masihyeganeh commented Apr 29, 2024

View reviewed changes

Merge branch 'master' into masih/clawback

69bd51a

masihyeganeh commented Apr 29, 2024

View reviewed changes

ysv approved these changes Apr 29, 2024

View reviewed changes

masihyeganeh merged commit a3cb359 into master Apr 29, 2024
10 checks passed

masihyeganeh deleted the masih/clawback branch April 29, 2024 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Clawback feature for assetft #804

Implement Clawback feature for assetft #804

masihyeganeh commented Apr 12, 2024 •

edited by CoreumDevelopment

Loading

dzmitryhil left a comment

codecov bot commented Apr 16, 2024 •

edited

Loading

masihyeganeh left a comment

dzmitryhil left a comment

miladz68 left a comment

dzmitryhil left a comment

masihyeganeh left a comment

masihyeganeh left a comment

miladz68 left a comment

ysv left a comment

dzmitryhil left a comment

masihyeganeh left a comment

dzmitryhil left a comment

miladz68 left a comment

ysv left a comment

miladz68 left a comment

masihyeganeh left a comment

miladz68 left a comment

dzmitryhil left a comment

ysv left a comment

masihyeganeh left a comment

ysv left a comment

miladz68 left a comment

masihyeganeh left a comment

masihyeganeh left a comment

masihyeganeh left a comment

ysv left a comment

Implement Clawback feature for assetft #804

Implement Clawback feature for assetft #804

Conversation

masihyeganeh commented Apr 12, 2024 • edited by CoreumDevelopment Loading

Description

Reviewers checklist:

Authors checklist

dzmitryhil left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2024 • edited Loading

Codecov Report

masihyeganeh left a comment

Choose a reason for hiding this comment

dzmitryhil 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

masihyeganeh left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

masihyeganeh 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

ysv left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

masihyeganeh left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

masihyeganeh commented Apr 12, 2024 •

edited by CoreumDevelopment

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading