Feat/new register runtx flows #52

seantking · 2021-06-23T16:39:35Z

This PR updates the core interchain account module based on the new specification: cosmos/ibc#567

How to run

Check out the latest readme to test -> https://github.com/cosmos/interchain-accounts/tree/feat/new-register-runtx-flows#readme

seantking · 2021-06-24T17:52:20Z

.

seantking · 2021-06-24T17:57:38Z

x/ibc-account/keeper/account.go

+ return err
+ }
+
+ //TODO: Add call to OnChanOpenInit. The hermes relayer currently does not support this.


This is being worked on at the moment by the hermes team

seantking · 2021-06-25T15:32:38Z

x/ibc-account/keeper/account.go

 }

 // Determine account's address that will be created.
-func (k Keeper) GenerateAddress(identifier string, salt []byte) []byte {
- return tmhash.SumTruncated(append([]byte(identifier), salt...))
+func (k Keeper) GenerateAddress(identifier string) []byte {


@milosevic I think this may be an issue.

We need to have a way to deterministically generate the interchain account address on both chains from an identifier. Right now this works as I'm just running the same chain x2 but I guess this would return a different address depending on chain configuration?

The controller chain needs to know what the registered account address is (perhaps for sending ics20 transfers). As we are registering the account in the channel handshake callbacks the sending side has no way to retrieve what the interchain account address is.

Although, the more I think on it, it's not a must-have. We can always set the interchain address on the controller side at a later time (maybe parse it from an ack packet) or just have the host side have a query to get the interchain account address (for the short term this is totally fine)

This seems fine to me from a deterministic point of view. Are you worried about collisions?

I think the only way around this, for now, is to add another command to set the address of the registered account on the controller side (we will need to parse it from the IBC ack). We need the address of the interchain account for practically every msg we want to send.

seantking · 2021-06-25T15:33:53Z

x/ibc-account/keeper/handshake.go

- return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort)
- }
-
- if version != types.Version {


Still need to update this check. I was getting some errors, so I moved on.

seantking · 2021-06-25T15:34:04Z

x/ibc-account/keeper/handshake.go

- return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort)
- }
-
- if version != types.Version {


seantking · 2021-06-25T15:34:33Z

x/ibc-account/keeper/handshake.go

+ channelID string,
+ counterpartyVersion string,
+) error {
+ k.SetActiveChannel(ctx, portID, channelID)


I was thinking maybe it would be better to store the full Channel type rather than the id.

I think the ID is fine, any reason to store full channel?

I think the ID is also fine. Agreed.

seantking · 2021-06-25T15:35:07Z

x/ibc-account/keeper/handshake.go

+
+// May want to use these for re-opening a channel when it is closed
+//// OnChanCloseInit implements the IBCModule interface
+//func (am AppModule) OnChanCloseInit(


We could potentially try to reopen the channel when it closes here. Perhaps this adds to much edge case complexity tho.

Don't try to reopen. Just open a new channel with the same portID

And "unset" the active channel here

seantking · 2021-06-25T15:36:38Z

x/ibc-account/keeper/relay.go

-// TryRunTx attemps to send messages to source channel.
-func (k Keeper) TryRunTx(ctx sdk.Context, sourcePort, sourceChannel, typ string, data interface{}) ([]byte, error) {
- sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel)
+ sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePortId, activeChannelId)


I need to check here if the channel is closed. It actually isn't doing this check at the moment. The error that returns from TryRunTx if there is no active channel is just the standard IBC error (because the channel is not open). It works as intended though, just not the correct error message.

seantking · 2021-06-25T15:38:08Z

x/ibc-account/keeper/relay.go

- }
+func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData, ack channeltypes.Acknowledgement) error {
+ switch ack.Response.(type) {
+ case *channeltypes.Acknowledgement_Error:


I updated this to use the newer channeltypes.Acknowledgement. The latest ibc-go is using a newer standard but I'm still using an older version of the SDK at the moment.

seantking · 2021-06-25T15:39:29Z

x/ibc-account/types/account.go

@@ -13,27 +11,23 @@ import (
 authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 )

+const (
+ IcaPrefix string = "ics27-1-"


@milosevic Every registered port is prefixed with this. I was thinking it is a good way to track what version of ics27 an account was registered under (this may be useful for migrating accounts in the future, or even just to help with port collisions)

seantking · 2021-06-30T07:58:59Z

x/ibc-account/keeper/grpc_query.go

@@ -52,8 +52,7 @@ func (k Keeper) IBCAccountFromData(ctx context.Context, req *types.QueryIBCAccou
 }

 sdkCtx := sdk.UnwrapSDKContext(ctx)
- identifier := types.GetIdentifier(req.Port, req.Channel)
- address := k.GenerateAddress(identifier, []byte(req.Data))
+ address := k.GenerateAddress("test")


I'm going to update the grpc_query stuff today, and probably add a new cli command for querying for interchain accounts.

AdityaSripal

Excellent work!! This looks so much cleaner!

My main requests are just copies from what i said in spec PR, mainly around the handshake callbacks

AdityaSripal · 2021-06-30T13:01:30Z

x/ibc-account/keeper/account.go

 }

 // Determine account's address that will be created.
-func (k Keeper) GenerateAddress(identifier string, salt []byte) []byte {
- return tmhash.SumTruncated(append([]byte(identifier), salt...))
+func (k Keeper) GenerateAddress(identifier string) []byte {


This seems fine to me from a deterministic point of view. Are you worried about collisions?

AdityaSripal · 2021-06-30T13:02:41Z

x/ibc-account/keeper/handshake.go

- if version != types.Version {
- return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid version: %s, expected %s", version, "ics20-1")
- }
-


You should still check module has bound to given port and it is in expected format

AdityaSripal · 2021-06-30T13:03:06Z

x/ibc-account/keeper/handshake.go

@@ -54,15 +46,6 @@ func (k Keeper) OnChanOpenTry(
 version,
 counterpartyVersion string,
 ) error {
- boundPort := k.GetPort(ctx)


AdityaSripal · 2021-06-30T13:03:40Z

x/ibc-account/keeper/handshake.go

+ channelID string,
+ counterpartyVersion string,
+) error {
+ k.SetActiveChannel(ctx, portID, channelID)


I think the ID is fine, any reason to store full channel?

AdityaSripal · 2021-06-30T13:04:00Z

x/ibc-account/keeper/handshake.go

+
+// May want to use these for re-opening a channel when it is closed
+//// OnChanCloseInit implements the IBCModule interface
+//func (am AppModule) OnChanCloseInit(


Don't try to reopen. Just open a new channel with the same portID

AdityaSripal · 2021-06-30T13:04:33Z

x/ibc-account/keeper/handshake.go

+// portID,
+// channelID string,
+//) error {
+// return nil


"unset" active channel

I'm thinking we can create some issues for the work that is left to do. I.e. unset active channels + additional checks in the channel callbacks.

AdityaSripal · 2021-06-30T13:09:01Z

x/ibc-account/module.go

@@ -191,7 +188,7 @@ func (am AppModule) OnChanCloseInit(
 channelID string,
 ) error {
 // Disallow user-initiated channel closing for interchain account channels
- return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
+ return nil


This will allow anyone to close a channel. If you want to let people voluntarily close a channel, you need to do at least some kind of authentication here

Updated this 👍

AdityaSripal · 2021-06-30T13:09:44Z

x/ibc-account/module.go

@@ -191,7 +188,7 @@ func (am AppModule) OnChanCloseInit(
 channelID string,
 ) error {
 // Disallow user-initiated channel closing for interchain account channels
- return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
+ return nil


You should also "unset" active channel

AdityaSripal · 2021-06-30T13:09:54Z

x/ibc-account/module.go

@@ -191,7 +188,7 @@ func (am AppModule) OnChanCloseInit(
 channelID string,
 ) error {
 // Disallow user-initiated channel closing for interchain account channels
- return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
+ return nil
 }

 func (am AppModule) OnChanCloseConfirm(


unset active channel

AdityaSripal · 2021-06-30T13:13:12Z

x/ibc-account/keeper/handshake.go

 host "github.com/cosmos/cosmos-sdk/x/ibc/core/24-host"
-
- "github.com/cosmos/interchain-accounts/x/ibc-account/types"
 )

 func (k Keeper) OnChanOpenInit(


These functions should also error if the active channel is already set

That is a good point

lgtm-com · 2021-07-09T12:30:46Z

This pull request introduces 1 alert when merging 0643e22 into 63755ee - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-07-09T16:39:50Z

This pull request introduces 1 alert when merging f8b148c into 63755ee - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-07-13T07:23:51Z

This pull request introduces 1 alert when merging 8d6ad18 into 63755ee - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

seantking added 8 commits June 18, 2021 13:16

feat: adding OnChanOpenInit call for register account

eb62b60

feat: adding event

da77fda

test

ff88935

testing

55f67da

feat: binding port on register & adding handshake callbacks

3de1bb0

feat: adding set/get for active channel

c5f5247

refactor: clean up getter/setter

e7472f6

chore: adding channel handshake handlers to module.go

86e5749

seantking commented Jun 24, 2021

View reviewed changes

seantking added 2 commits June 25, 2021 15:45

refactor: updating acknowledgement packet format

e453375

fix: returning nil for OnChanClose

1240960

seantking commented Jun 25, 2021

View reviewed changes

This was referenced Jun 27, 2021

fix: removing tx encoder #53

Merged

feat: adding gov demo #49

Closed

fix(hermes-relayer-dev-env): Add missing gas_price #55

Closed

seantking commented Jun 30, 2021

View reviewed changes

AdityaSripal suggested changes Jun 30, 2021

View reviewed changes

ancazamfir and others added 4 commits June 30, 2021 09:59

Upgrade hermes, add chann_init

a7cb92f

fix: removing tx encoder (#53)

704baa9

adding call to OnChanOpenInit & updating send proto

0d40c48

feat: adding connection tracking and setting address on controller side

a1d7e97

seantking force-pushed the feat/new-register-runtx-flows branch from b9a8611 to a1d7e97 Compare July 4, 2021 20:04

ancazamfir and others added 2 commits July 5, 2021 15:30

Emit channelOpenInit event, cleanup hermes script

bb48c77

feat: updating send to use ica addr

0643e22

seantking added 2 commits July 9, 2021 17:54

feat: adding authentication step on host chain, updating channel scripts

ea85e35

documentation: updating readme

b7f60f9

seantking force-pushed the feat/new-register-runtx-flows branch from 7dc1c41 to b7f60f9 Compare July 9, 2021 16:09

fix: user cannot close channel

f8b148c

seantking force-pushed the feat/new-register-runtx-flows branch from 13a28bc to f8b148c Compare July 9, 2021 16:25

ancazamfir mentioned this pull request Jul 12, 2021

Issues during testing interchain accounts application informalsystems/hermes#1191

Closed

5 tasks

seantking added 2 commits July 13, 2021 08:45

refactor: renaming core fns

1cf6368

doc: update

8d6ad18

seantking force-pushed the feat/new-register-runtx-flows branch from 64150f0 to 8d6ad18 Compare July 13, 2021 07:05

seantking merged commit b8b76c0 into master Jul 13, 2021

seantking deleted the feat/new-register-runtx-flows branch July 22, 2021 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/new register runtx flows #52

Feat/new register runtx flows #52

seantking commented Jun 23, 2021 •

edited

Loading

seantking commented Jun 24, 2021

seantking Jun 24, 2021

seantking Jun 25, 2021 •

edited

Loading

AdityaSripal Jun 30, 2021

seantking Jul 2, 2021

seantking Jun 25, 2021

seantking Jun 25, 2021

seantking Jun 25, 2021

AdityaSripal Jun 30, 2021

seantking Jun 30, 2021

seantking Jun 25, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

seantking Jun 25, 2021

seantking Jun 25, 2021

seantking Jun 25, 2021

seantking Jun 30, 2021

AdityaSripal left a comment

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

seantking Jul 9, 2021

AdityaSripal Jun 30, 2021

seantking Jul 9, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

AdityaSripal Jun 30, 2021

seantking Jun 30, 2021

lgtm-com bot commented Jul 9, 2021

lgtm-com bot commented Jul 9, 2021

lgtm-com bot commented Jul 13, 2021

Feat/new register runtx flows #52

Feat/new register runtx flows #52

Conversation

seantking commented Jun 23, 2021 • edited Loading

This PR updates the core interchain account module based on the new specification: cosmos/ibc#567

How to run

seantking commented Jun 24, 2021

Choose a reason for hiding this comment

seantking Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 9, 2021

lgtm-com bot commented Jul 9, 2021

lgtm-com bot commented Jul 13, 2021

seantking commented Jun 23, 2021 •

edited

Loading

seantking Jun 25, 2021 •

edited

Loading