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

refactor: RegisterInterchainAccount #814

Merged
merged 10 commits into from
Feb 2, 2022
Merged

refactor: RegisterInterchainAccount #814

Show file tree
Hide file tree
Changes from 2 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
14 changes: 9 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,17 @@ 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")
if !k.IsBound(ctx, 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.

I switched this to use our custom keeper fn IsBound which also ensures the capability has been claimed for this portID. Otherwise, we would panic below (the tests picked this up & thanks @damiannolan for the second pair of eyes).

Copy link
Contributor

Choose a reason for hiding this comment

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

This can still panic. The purpose of using the portKeeper.IsBound was that ports claimed outside of ics27 (by another IBC module) would cause a panic here since you are only checking the portID's claimed by the controller keeper and not the portID's binded to via 05-port.

The code needs to be more nuanced:

switch:
case portkeeper.IsBound() && !k.IsBound():
    return err (controller keeper doesn't own capability)
    
case !portKeeper.IsBound():
    k.BindPort
    k.ClaimCapability
    
}

Or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, It might be better to panic on the case that the controller doesn't own the capability since this means another module on the chain claimed a capability in the namespace of ics27. No real difference since a panic just results in a failed transaction anyways. A panic might just be more likely to surface the problem (we should likely investigate closer if that error ever did occur)

Copy link
Contributor

Choose a reason for hiding this comment

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

we should still use port keeper IsBound() though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, leave it as is then?

Copy link
Contributor

@colin-axner colin-axner Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No, the current code unintentionally has ok behaviour. If the port is bounded by a different moudle, it will panic on BindPort(), but as far as I can tell this is unintentional. I'd prefer to restructure the code such that we have clearly defined expected paths such that if anything changes in the future, we aren't resting the correctness of our code on assumptions (what if BindPort returns an error instead of panic'ing)

I would probably use my switch above, but panic instead of returning an error. I'd also add a test case for this scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, gotcha. Thanks for making it clear 👍

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.Wrap(err, "unable to bind to newly generated portID")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be useful to include portID in this error msg, unless its indicated in the wrapped err

}
}

connectionEnd, err := k.channelKeeper.GetConnection(ctx, connectionID)
Expand Down
9 changes: 6 additions & 3 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,11 +23,13 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
"success", func() {}, true,
},
{
"port is already bound",
"port is already bound for owner",
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().ICAControllerKeeper.ClaimCapability(suite.chainA.GetContext(), cap, host.PortPath(TestPortID))
suite.Require().NoError(err)
},
false,
true,
},
{
"fails to generate port-id",
Expand Down
17 changes: 9 additions & 8 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,12 +13,13 @@ 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")
ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version")
ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 12, "invalid account address")
ErrUnsupported = sdkerrors.Register(ModuleName, 13, "interchain account does not support this action")
ErrInvalidControllerPort = sdkerrors.Register(ModuleName, 14, "invalid controller port")
ErrInvalidHostPort = sdkerrors.Register(ModuleName, 15, "invalid host port")
ErrInvalidTimeoutTimestamp = sdkerrors.Register(ModuleName, 16, "timeout timestamp must be in the future")
ErrInvalidCodec = sdkerrors.Register(ModuleName, 17, "codec is not supported")
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")
ErrInvalidControllerPort = sdkerrors.Register(ModuleName, 15, "invalid controller port")
ErrInvalidHostPort = sdkerrors.Register(ModuleName, 16, "invalid host port")
ErrInvalidTimeoutTimestamp = sdkerrors.Register(ModuleName, 17, "timeout timestamp must be in the future")
ErrInvalidCodec = sdkerrors.Register(ModuleName, 18, "codec is not supported")
)