refactor: RegisterInterchainAccount

[seantking]

Description

closes: #802

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[seantking]

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).

[colin-axner]

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

[colin-axner]

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)

[colin-axner]

we should still use port keeper IsBound() though

[seantking]

So, leave it as is then?

[colin-axner]

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

[seantking]

Alright, gotcha. Thanks for making it clear 👍

[seantking]

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

[codecov-commenter]

Codecov Report

Merging #814 (491c552) into main (25fb89d) will decrease coverage by 0.02%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main     #814      +/-   ##
==========================================
- Coverage   79.63%   79.60%   -0.03%     
==========================================
  Files         147      147              
  Lines       10693    10695       +2     
==========================================
- Hits         8515     8514       -1     
- Misses       1761     1763       +2     
- Partials      417      418       +1     

Impacted Files	Coverage Δ
...7-interchain-accounts/controller/keeper/account.go	63.63% <62.50%> (-7.34%)	⬇️

[seantking]

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.

[damiannolan]

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

[colin-axner]

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

[seantking]

That is an excellent point

[AdityaSripal]

The current diffs look good.

But I think we should change the key format of active channel. Currently it is connectionID/portID.

I think it makes more sense to have it be: portID/connectionID. That way its easy to iterate over the store and figure out which channels are owned by a given owner address.

cc: @colin-axner

[colin-axner]

Great work! Thanks for following up with my code nit

[colin-axner]

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)

[colin-axner]

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

[colin-axner]

It would be nice if there was a test though using the same owner address on different connections

[damiannolan]

bad bot @mergify