Handle whitelisting and freezing for IBC transfers

[wojtek-coreum]

This change is 

[dzmitryhil]

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

---

integration-tests/ibc/asset_ft_test.go line 293 at r3 (raw file):

	sendCoin := sdk.NewCoin(denom, sdk.NewInt(1000))
	halfCoin := sdk.NewCoin(denom, sdk.NewInt(500))

I would call it frozenCoin or similar, to highlight the purpose.

---

integration-tests/ibc/asset_ft_test.go line 476 at r3 (raw file):

	// send coins back to coreum, it should fail
	_, err = gaiaChain.ExecuteIBCTransfer(ctx, gaiaRecipient, ibcSendCoin, coreumChain.ChainContext, coreumIssuer)

I would propose here to send to the issuer and normal account as well.

---

integration-tests/ibc/asset_ft_test.go line 506 at r3 (raw file):

	requireT.NoError(coreumChain.AwaitForBalance(ctx, coreumIssuer, sendCoin))
}

What about the additional test to send to the blocked address on the peer chain?

---

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

		},
		{
			name: "issuer_sender_ibc_escrow_sender_ibc_escrow_receiver",

What was the reason to remvoe that test?

---

x/wibc/info.go line 1 at r3 (raw file):

package wibc

I would call the file, trace and struct Info -> PackageTrace (same for the methods). So it would be the wibc.PackageTrace. Which is more precise IMO.

---

x/wibc/info.go line 48 at r3 (raw file):

// NewInfoMiddleware returns new info middleware.
func NewInfoMiddleware(module porttypes.IBCModule, action Action) InfoMiddleware {

IMO it's better to have the constructor method, just after the type declaration, not before.

---

x/wibc/info.go line 56 at r3 (raw file):

// InfoMiddleware adds information about IBC port and channel of the received packet to the context.
type InfoMiddleware struct {

Why don't you use the embeded struct here? You could remove a lot of methods in that case.

---

x/wibc/info.go line 105 at r3 (raw file):

// OnAcknowledgementPacket simply calls the implementation of the wrapped module.
func (im InfoMiddleware) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress) error {

Could you please explain why don't we need to intercept the OnAcknowledgementPacket and OnTimeoutPacket for the case when an account was unwhitelisted and then the transfer ibc tx is rolled back.

---

x/wibctransfer/types/action.go line 7 at r3 (raw file):

const (
	// ActionOut is used when IBC transfer to another chain is initialized by executing ibctransfertypes.MsgTransfer message.
	ActionOut wibc.Action = "ibcTransferOut"

IMO it's not very clear when the enum is defined in one package but, the values are in different. They should be near otherwise no need ti use the enum.

[dzmitryhil]

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

[wojtek-coreum]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

integration-tests/ibc/asset_ft_test.go line 506 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the additional test to send to the blocked address on the peer chain?

Failing IBC transfer tests will be added in another PR

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

What was the reason to remvoe that test?

Because the situation where escrow address sends funds to escrow address is impossible. There is no such logic in IBC module.

---

x/wibc/info.go line 56 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why don't you use the embeded struct here? You could remove a lot of methods in that case.

Because in that case this logic would work only with transfer module

---

x/wibc/info.go line 105 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Could you please explain why don't we need to intercept the OnAcknowledgementPacket and OnTimeoutPacket for the case when an account was unwhitelisted and then the transfer ibc tx is rolled back.

We need to do this. Failing IBC transfers will be supported in the next PR.

---

x/wibctransfer/types/action.go line 7 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO it's not very clear when the enum is defined in one package but, the values are in different. They should be near otherwise no need ti use the enum.

The logic for intercepting the transfer is common to any IBC module but actions are specific to the module.

[miladz68]

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

---

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

}

func isIBCAction(ctx sdk.Context, action wibc.Action) bool {

I think having 2 function is more reasonable here than having 2 enum types.
the 2 functions can be called. isTokenReceive and isTokenSend, or something better could be found.

I think the most accurate naming is isIBCTokenTransferOut and isIBCPacketIn

---

x/wibc/info.go line 43 at r3 (raw file):

// WithGOInfo stores IBC info inside GO context.
func WithGOInfo(ctx context.Context, info Info) context.Context {

This function is used once inside module.
firstly I think it should not be defined, since it is only used once. if you disagree, at least it should not be exported
also it should Go, not GO

---

x/wibc/info.go line 58 at r3 (raw file):

type InfoMiddleware struct {
	module porttypes.IBCModule
	action Action

I don't think the action should be part of the struct. just use it directly in OnRecvPacket

[wojtek-coreum]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

x/wibc/info.go line 58 at r3 (raw file):

Previously, miladz68 (milad) wrote…

I don't think the action should be part of the struct. just use it directly in OnRecvPacket

I don't understand this comment

[dzmitryhil]

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

---

integration-tests/ibc/asset_ft_test.go line 506 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Failing IBC transfer tests will be added in another PR

Clear, resolved.

---

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

Previously, wojtek-coreum (Wojtek) wrote…

Because the situation where escrow address sends funds to escrow address is impossible. There is no such logic in IBC module.

What about the case of when I send back coins from the chain and the destination is the escrow address?

---

x/wibc/info.go line 56 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Because in that case this logic would work only with transfer module

Could you please elaborate a bit on it, I didn't get it.

---

x/wibc/info.go line 105 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

We need to do this. Failing IBC transfers will be supported in the next PR.

Ok, clear.

---

x/wibctransfer/types/action.go line 7 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

The logic for intercepting the transfer is common to any IBC module but actions are specific to the module.

In that case, the Action should be either string or interface.

[miladz68]

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

---

x/wibc/info.go line 58 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I don't understand this comment

you introduce action into the struct and keep it there. you later used it in onRecvPacket method. the only acceptable value to be used on that function is ActionIn. instead of passing it to the struct and then pass it to the method, just the value directly in onRecvPacket method.

[ysv]

Reviewed 8 of 11 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

---

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

Previously, miladz68 (milad) wrote…

I think having 2 function is more reasonable here than having 2 enum types.
the 2 functions can be called. isTokenReceive and isTokenSend, or something better could be found.

I think the most accurate naming is isIBCTokenTransferOut and isIBCPacketIn

I agree with Milad's idea here
We should expose as abstract interface from wibc module as possible without defining internals of how/what is stored inside context

IMO proper methods to be exposed by wibc is (like Milad mentioned):

func isIBCTokenTransferOut(ctx sdk.Context) bool {}
func isIBCTokenTransferIn(ctx sdk.Context) bool {}

& things like GetInfo, Info etc are internal stuff

---

x/wibc/info.go line 1 at r3 (raw file):

package wibc

also I disagree with wibc name here
You don't really wrap/override whole IBC module but just introduce a middleware to inject additional info

---

x/wibc/info.go line 56 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Could you please elaborate a bit on it, I didn't get it.

I'm not sure if this approach will work but it is a good option & worth trying IMO

Upd:
Tried locally Seems to be working fine for me:

// InfoMiddleware adds information about IBC port and channel of the received packet to the context.
type InfoMiddleware struct {
	porttypes.IBCModule
	action Action
}

This way you don't need to copy all methods but only ones which are overridden

---

x/wibc/info.go line 58 at r3 (raw file):

Previously, miladz68 (milad) wrote…

you introduce action into the struct and keep it there. you later used it in onRecvPacket method. the only acceptable value to be used on that function is ActionIn. instead of passing it to the struct and then pass it to the method, just the value directly in onRecvPacket method.

I agree with Milad don't see a point to store action Action inside InfoMiddleware

---

x/wibctransfer/keeper/keeper.go line 23 at r3 (raw file):

// NewKeeper returns a new Wrapper instance.
func NewKeeper(

NewKeeper method returns Wrapper. I think either method or structure should be renamed

what about smth like TransferKeeperWrapper ?

At least inside wbank we had this kind of naming convention

---

x/wibctransfer/types/action.go line 7 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

In that case, the Action should be either string or interface.

I tend to agree with Dzmitry
Feels like we should put the whole thingy into a single module or do smth else with it

Lets discuss

[wojtek-coreum]

Reviewable status: 4 of 13 files reviewed, 10 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

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

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the case of when I send back coins from the chain and the destination is the escrow address?

then the source is not escrow

---

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

Previously, ysv (Yaroslav Savchuk) wrote…

I agree with Milad's idea here
We should expose as abstract interface from wibc module as possible without defining internals of how/what is stored inside context

IMO proper methods to be exposed by wibc is (like Milad mentioned):

func isIBCTokenTransferOut(ctx sdk.Context) bool {}
func isIBCTokenTransferIn(ctx sdk.Context) bool {}

& things like GetInfo, Info etc are internal stuff

Done.

---

x/wibctransfer/keeper/keeper.go line 23 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

NewKeeper method returns Wrapper. I think either method or structure should be renamed

what about smth like TransferKeeperWrapper ?

At least inside wbank we had this kind of naming convention

Done.

---

x/wibc/info.go line 1 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would call the file, trace and struct Info -> PackageTrace (same for the methods). So it would be the wibc.PackageTrace. Which is more precise IMO.

I redone it completely

---

x/wibc/info.go line 1 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

also I disagree with wibc name here
You don't really wrap/override whole IBC module but just introduce a middleware to inject additional info

I redone it completely

---

x/wibc/info.go line 43 at r3 (raw file):

Previously, miladz68 (milad) wrote…

This function is used once inside module.
firstly I think it should not be defined, since it is only used once. if you disagree, at least it should not be exported
also it should Go, not GO

Done.

---

x/wibc/info.go line 56 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I'm not sure if this approach will work but it is a good option & worth trying IMO

Upd:
Tried locally Seems to be working fine for me:

// InfoMiddleware adds information about IBC port and channel of the received packet to the context.
type InfoMiddleware struct {
	porttypes.IBCModule
	action Action
}

This way you don't need to copy all methods but only ones which are overridden

I redone this completely

---

x/wibc/info.go line 58 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I agree with Milad don't see a point to store action Action inside InfoMiddleware

I redone this completely

---

x/wibctransfer/types/action.go line 7 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I tend to agree with Dzmitry
Feels like we should put the whole thingy into a single module or do smth else with it

Lets discuss

I redone this completely

[wojtek-coreum]

Reviewable status: 3 of 14 files reviewed, 10 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

integration-tests/ibc/asset_ft_test.go line 293 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would call it frozenCoin or similar, to highlight the purpose.

The logic behind this name is that I use it to freeze the first half, and send the second half. So this var is used in two contexts.

[wojtek-coreum]

Reviewable status: 3 of 14 files reviewed, 10 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

integration-tests/ibc/asset_ft_test.go line 476 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would propose here to send to the issuer and normal account as well.

Done.

[dzmitryhil]

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

---

x/wibctransfer/middleware.go line 37 at r7 (raw file):

// IsDirectionOut returns true if context is tagged with an outgoing transfer.
func IsDirectionOut(ctx sdk.Context) bool {

IMO both func IsDirectionOut(ctx sdk.Context) bool { and func IsDirectionIn(ctx sdk.Context) bool { should be in the types/direction.go.

[wojtek-coreum]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

---

integration-tests/ibc/asset_ft_test.go line 154 at r8 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

IMO it is reasonable to make sure that DirectionOut overrides DirectionIn but not vice versa
Also it is edge case which is important to test

Let's discuss after daily

---

integration-tests/ibc/asset_ft_test.go line 384 at r8 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

yes but is just more realistic scenario

Done.

---

x/asset/ft/keeper/before_send_test.go line 350 at r7 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

in that case escrow address is treated like any other account

Do you have a proper place to put this as comment ? That DirectionOut will override DirectionIn

It's not like DirectionOut overrides DirectionIn. When IBC transfer is received then the receiver is treated like a normal address and all the ft features are applied normally.

---

x/wibctransfer/types/direction.go line 16 at r8 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

DirectionIn is used when incoming IBC transfer comes to the target chain.

when IBC transfer comes from peer chain to Coreum

Done.

[ysv]

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

---

integration-tests/ibc/asset_ft_test.go line 154 at r8 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Let's discuss after daily

ok

---

x/asset/ft/keeper/before_send_test.go line 350 at r7 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

It's not like DirectionOut overrides DirectionIn. When IBC transfer is received then the receiver is treated like a normal address and all the ft features are applied normally.

Lets discuss after daily also

[miladz68]

Reviewed 3 of 9 files at r4, 3 of 6 files at r5, 2 of 4 files at r8, 2 of 3 files at r9, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @wojtek-coreum)

[dzmitryhil]

Reviewed 2 of 4 files at r8, 3 of 3 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @wojtek-coreum)

[ysv]

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

---

x/asset/ft/keeper/before_send_test.go line 350 at r7 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Lets discuss after daily also

Lets change test to not mention escrow but have a flag to define if it is IBC transfer or not

[wojtek-coreum]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

---

x/asset/ft/keeper/before_send_test.go line 350 at r7 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Lets change test to not mention escrow but have a flag to define if it is IBC transfer or not

Done.

[miladz68]

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

[miladz68]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

[ysv]

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

---

integration-tests/chain.go line 191 at r18 (raw file):

	)

	return txRes, err

do you really need to have access to err in tests ?

because I thought the idea is to pass t into such helper functions & make assertions inside eg ExecuteIBCTransfer

---

integration-tests/ibc/asset_ft_test.go line 242 at r18 (raw file):

func TestIBCAssetFTFreezing(t *testing.T) {
	t.Parallel()
	t.Helper()

but it is not a helper but a real test, no ?

---

integration-tests/ibc/asset_ft_test.go line 336 at r18 (raw file):

func TestEscrowAddressIsResistantToFreezingAndWhitelisting(t *testing.T) {
	t.Parallel()
	t.Helper()

but it is not a helper but a real test, no ?

[wojtek-coreum]

Reviewable status: 15 of 16 files reviewed, 3 unresolved discussions (waiting on @silverspase and @ysv)

---

integration-tests/chain.go line 191 at r18 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

do you really need to have access to err in tests ?

because I thought the idea is to pass t into such helper functions & make assertions inside eg ExecuteIBCTransfer

Yes I really need it. In case of freezing I need to know that transfer failed without exiting the test

---

integration-tests/ibc/asset_ft_test.go line 242 at r18 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

but it is not a helper but a real test, no ?

Done.

---

integration-tests/ibc/asset_ft_test.go line 336 at r18 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

but it is not a helper but a real test, no ?

Done.

[ysv]

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

[miladz68]

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

[dzmitryhil]

Reviewed 1 of 1 files at r12, 2 of 3 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase)