Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: RegisterInterchainAccount #814

Merged
merged 10 commits into from
Feb 2, 2022
Merged

refactor: RegisterInterchainAccount #814

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f2bac3
refactor: error if active channel exists & OPEN, no longer err on por…
seantking Jan 28, 2022
5e6047d
refactor: using keeper helper fn IsBound instead of portKeeper directly
seantking Jan 28, 2022
93faf7a
nit: comment:
seantking Jan 31, 2022
491c552
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Jan 31, 2022
2bb2b86
fix: adding check that capability is not claimed by controller
seantking Feb 2, 2022
6ed1869
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Feb 2, 2022
d75285f
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Feb 2, 2022
aa9192b
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Feb 2, 2022
cc63188
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Feb 2, 2022
be141c1
Merge branch 'main' into sean/issue#802-update-register-ica
seantking Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions modules/apps/27-interchain-accounts/controller/keeper/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,20 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner s
return err
}

if k.portKeeper.IsBound(ctx, portID) {
return sdkerrors.Wrap(icatypes.ErrPortAlreadyBound, portID)
// if there is an active channel for this portID / connectionID return an error
activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID)
Copy link
Contributor Author

@seantking seantking Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By checking this we're ensuring that an owner address can only have one interchain account per connection. Ideally, an owner could have many accounts per connection. I think this is fine for v1 though.

There's still nothing stopping ica-auth dev from using owner input parameter however they please though (increment a sequence & append or w/e), so it's not a total blocker anyway.

cc @AdityaSripal @colin-axner @damiannolan

if found {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s for owner %s", activeChannelID, portID, connectionID, owner)
}

cap := k.BindPort(ctx, portID)
if err := k.ClaimCapability(ctx, cap, host.PortPath(portID)); err != nil {
return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
switch {
case k.portKeeper.IsBound(ctx, portID) && !k.IsBound(ctx, portID):
return sdkerrors.Wrapf(icatypes.ErrPortAlreadyBound, "another module has claimed capability for and bound port with portID: %s", portID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return sdkerrors.Wrapf(icatypes.ErrPortAlreadyBound, "another module has claimed capability for and bound port with portID: %s", portID)
return sdkerrors.Wrapf(icatypes.ErrPortAlreadyBound, "another module has claimed capability for the bound port: %s", portID)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#828 - not to be done now, but thought this change might help in the future

case !k.portKeeper.IsBound(ctx, portID):
cap := k.BindPort(ctx, portID)
if err := k.ClaimCapability(ctx, cap, host.PortPath(portID)); err != nil {
Copy link
Contributor Author

@seantking seantking Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage is down as we're not covering this line. Not sure what the best way to test this is, or if we even should bother. We're already checking above to ensure the capability is not claimed.

return sdkerrors.Wrapf(err, "unable to bind to newly generated portID: %s", portID)
}
}

connectionEnd, err := k.channelKeeper.GetConnection(ctx, connectionID)
Expand Down
9 changes: 5 additions & 4 deletions modules/apps/27-interchain-accounts/controller/keeper/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)

Expand All @@ -22,9 +23,11 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
"success", func() {}, true,
},
{
"port is already bound",
"port is already bound for owner but capability is claimed by another module",
func() {
suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
cap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
err := suite.chainA.GetSimApp().TransferKeeper.ClaimCapability(suite.chainA.GetContext(), cap, host.PortPath(TestPortID))
suite.Require().NoError(err)
},
false,
},
Expand Down Expand Up @@ -56,7 +59,6 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
false,
},
}

for _, tc := range testCases {
tc := tc

Expand All @@ -77,7 +79,6 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
} else {
suite.Require().Error(err)
}

})
}
}
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ var (
ErrInvalidRoute = sdkerrors.Register(ModuleName, 7, "invalid route")
ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 8, "interchain account not found")
ErrInterchainAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "interchain account is already set")
ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner")
ErrActiveChannelAlreadySet = sdkerrors.Register(ModuleName, 11, "active channel already set for this owner")
ErrActiveChannelAlreadySet = sdkerrors.Register(ModuleName, 10, "active channel already set for this owner")
ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 11, "no active channel for this owner")
Comment on lines +16 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a side note, this is state machine breaking (ABCI code changes), so I'd generally refrain from reorganizing error code ordering. It is hard to come up with any logical ordering when the code are grouped linearly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an excellent point

ErrInvalidVersion = sdkerrors.Register(ModuleName, 12, "invalid interchain accounts version")
ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 13, "invalid account address")
ErrUnsupported = sdkerrors.Register(ModuleName, 14, "interchain account does not support this action")
Expand Down