remove GetConnectionIdFromICAPort, use connection ID from host zone

[sampocs]

Context

There are a couple issues with how the connection ID is handled that prevents the re-registering of a host zone after it's been unregistered. This is primarily relevant for testnet, but also indicates a flaw in the current logic.

Namely the issues are:

• The function GetConnectionIdFromICAPort returns the first connection ID that matches the port ID - but there can be more than one connection with a given port ID
• The connection ID for the Delegation and SetWithdrawalAddress ICA's is pulled from the above function instead of from the connection ID on the host zone
• The OnChanOpenAck callback uses the above erroneous function which prevents registering ICAs on a new connection (since it will always match to the old expired connection)

Brief Changelog

• Removed GetConnectionIdFromICAPort
• Used ConnnectionId from host zone in Delegation and SetWithdrawalAddress ICAs
• Used GetConnectionFromChannel in place of the removed function in the OnChanOpenAck callback

[riley-stride]

Nice to get rid of those blocks of unnecessary code!

Do we need changes to ICAs that are submitted via submittxs to ICAs that are not the delegate acct? E.g. the ICA to the withdrawal acct that sweep rewards (submitted in FeeBalanceCallback)

As long as we address that question and integration test this I'm signed off

[sampocs]

Do we need changes to ICAs that are submitted via submittxs to ICAs that are not the delegate acct? E.g. the ICA to the withdrawal acct that sweep rewards (submitted in FeeBalanceCallback)

@riley-stride sorry can you say more on this question / point to a specific example?

I think all other references just grab the connection ID from the host zone (e.g. fee balance callback)

[riley-stride]

Do we need changes to ICAs that are submitted via submittxs to ICAs that are not the delegate acct? E.g. the ICA to the withdrawal acct that sweep rewards (submitted in FeeBalanceCallback)

@riley-stride sorry can you say more on this question / point to a specific example?

I think all other references just grab the connection ID from the host zone (e.g. fee balance callback)

Resolved in Slack with
"There’s only one connection ID per host zone - and each ICA channel belongs to that same connection. So the delegation and withdrawal ICA accounts/channels, share the same connection".

I'm signed off.

[asalzmann]

I thought each ICA had its own connection?

If there's just one connection per host zone, this makes sense.

[EDIT]
In RegisterHostZone, we pass in the zone's connection id when registering new ICAs, so it looks like the connection id is shared across accounts

[sampocs]

Each ICA has it's own channel - and all channels are on the same connection.

example

[asalzmann]

The function GetConnectionIdFromICAPort returns the first connection ID that matches the port ID - but there can be more than one connection with a given port ID

What's the scenario where there are two connections associated with an ICA port ID? Is it due to migrating the connection on an active host zone?

[shellvish]

This looks great to me! This is a great catch, it's quite an edge case.

I'm surprised we use the port_id->connection_id lookup when the connection_id is so readily available, but on-board with changing it now.

[sampocs]

What's the scenario where there are two connections associated with an ICA port ID? Is it due to migrating the connection on an active host zone?

Yeah the use case would be if a light client expired for a host and we had to update the connection. Hope to god we never have to worry about this on mainnet, but it already happened on testnet with dydx unfortunately :(

[ethan-stride]

LGTM
Much simplified, these Mocks are going to help testing in the future because in unit tests I often had to do the exact type of reverse lookups you now have built in.

[ethan-stride]

For ICA controllers I see how the port is unique but what about for transfer channels where the port is always 'transfer'
How was this reverse lookup function working before?