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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
refactor: using keeper helper fn IsBound instead of portKeeper directly
  • Loading branch information
@seantking
seantking committed Jan 28, 2022
commit 5e6047d484e92c378ae1317de1b722dc619d68a7
2 changes: 1 addition & 1 deletion 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 @@ -27,7 +27,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner s
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s for owner %s", activeChannelID, portID, connectionID, owner)
}

if !k.portKeeper.IsBound(ctx, 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
Member

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

Expand Down