feat: adding InitChannel util function #534

seantking · 2021-11-11T16:39:25Z

Description

Adding a helper function to open a new channel in the case that a channel closes. This is necessary so that a controller chain can regain access to an ICA.

closes: #509

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 · 2021-11-11T16:39:56Z

modules/apps/27-interchain-accounts/keeper/account.go

+// Opens a new channel on a particular port given a connection
+// This is a helper function to open a new channel
+// in case of a channel closing and the controller chain needs to regain access to an interchain account on the host chain
+func (k Keeper) InitChannel(ctx sdk.Context, portID, connectionID string) error {


Not sure about the naming here. I would like some opinions on this.

ReopenChannel? I'm not sure either, but I wouldn't want external users to start using InitChannel in place of InitInterchainAccount

Yeah, good point.

modules/apps/27-interchain-accounts/keeper/account.go

seantking · 2021-11-11T16:46:04Z

modules/apps/27-interchain-accounts/keeper/account_test.go

+			portID, _ = types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointA.Counterparty.ConnectionID)
+
+			// bind port and claim capability
+			cap := suite.chainA.GetSimApp().ICAKeeper.BindPort(suite.chainA.GetContext(), portID)


Also not entirely sure what the best method for testing this out is or how far we want to go with testing. I initially made a call to InitInterchainAccount and finished the handshake, and then tried to call InitChannel and finish a second handshake, but I ran into verification issues. This is probs overkill anyway.

Basically, I'm just binding the port fresh here and then checking that we can finish a handshake.

seantking · 2021-11-11T16:46:41Z

modules/apps/27-interchain-accounts/keeper/account_test.go

+				// finish the channel handshake
+
+				// commit state changes for proof verification
+				path.EndpointA.Chain.App.Commit()


maybe overkill finishing the handshake here?

I have no strong preference, but might be better to split this off into its own test, ie one test doing table tests and another finishing the handshake in the happy case

codecov-commenter · 2021-11-11T16:46:57Z

Codecov Report

Merging #534 (48f61cd) into interchain-accounts (3ff5bf8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           interchain-accounts     #534      +/-   ##
=======================================================
+ Coverage                79.10%   79.11%   +0.01%     
=======================================================
  Files                      134      134              
  Lines                    10347    10354       +7     
=======================================================
+ Hits                      8185     8192       +7     
  Misses                    1753     1753              
  Partials                   409      409

Impacted Files	Coverage Δ
...ules/apps/27-interchain-accounts/keeper/account.go	`83.78% <100.00%> (+3.78%)`	⬆️

seantking · 2021-11-12T10:53:06Z

@AdityaSripal @colin-axner @damiannolan I'm actually questioning now if this is even necessary at all and maybe should be removed from the spec as well.

In the case that a channel closes, an ICA-auth module could just call the OnChanOpenInit function directly I imagine. Or just use a relayer to re-open a channel. Although perhaps this util will make things a bit easier.

AdityaSripal · 2021-11-12T11:01:20Z

Hmm it seems like the way you have it here is the cleanest option.

If you want to reopen the channel directly, you could do

icaKeeper.channelKeeper.ChanOpenInit(...)
// callback doesn't get called by channelKeeper's ChanOpenInit function
// it gets called by the msg server
icaModule.OnChanOpenInit(...)

But sending the message to IBC's msgserver, just abstracts the above lines for you since that is basically what the message server does. And it seems like easier maintanence to me, if core IBC ever refactors its message handling, then you don't have to reimplement the channel msg server function within ICA

colin-axner · 2021-11-12T12:48:45Z

modules/apps/27-interchain-accounts/keeper/account.go

+// Opens a new channel on a particular port given a connection
+// This is a helper function to open a new channel
+// in case of a channel closing and the controller chain needs to regain access to an interchain account on the host chain
+func (k Keeper) InitChannel(ctx sdk.Context, portID, connectionID string) error {


ReopenChannel? I'm not sure either, but I wouldn't want external users to start using InitChannel in place of InitInterchainAccount

colin-axner · 2021-11-12T12:49:02Z

modules/apps/27-interchain-accounts/keeper/account.go

+
+// Opens a new channel on a particular port given a connection
+// This is a helper function to open a new channel
+// in case of a channel closing and the controller chain needs to regain access to an interchain account on the host chain


godoc should begin with the name of the function

colin-axner · 2021-11-12T12:50:05Z

modules/apps/27-interchain-accounts/keeper/account_test.go

@@ -67,3 +68,74 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestInitChannel() {


this should test that when a channel is closed and the portID is set to inactive, a new channel can be opened

colin-axner · 2021-11-12T12:53:56Z

modules/apps/27-interchain-accounts/keeper/account_test.go

+				// finish the channel handshake
+
+				// commit state changes for proof verification
+				path.EndpointA.Chain.App.Commit()


I have no strong preference, but might be better to split this off into its own test, ie one test doing table tests and another finishing the handshake in the happy case

colin-axner · 2021-11-12T13:00:01Z

@seantking I also question whether we need this. My hesitancy is that we don't fully understand when this might be needed. If the function is just creating a MsgChannelOpenInit then I wonder if it is best to let the caller handle this complexity (since it isn't very much). I don't have a preference though

crodriguezvega · 2021-11-16T09:25:44Z

Not sure if this is feasible, but would there be any way of using the InitInterchainAccount for this use case as well? Maybe the function needs to be renamed to something else more descriptive, but if we have only one function to initialize the interchain account whether the channel is open or not, then I guess the API is simplified for the user. For example, if the channel is closed and the caller calls InitInterchainAccount again (assuming with the same arguments as for the first time it was invoked), then you skip the port binding and do only the opening of the channel.

colin-axner · 2021-12-01T15:41:43Z

One point of consideration is that this change could be released in a minor release. Perhaps it is best to not add it for now and add it later if it becomes desirable? Relayers could always just send the MsgChanOpenInit themselves?

seantking · 2022-01-10T14:16:45Z

Closing this for the now as we are deciding to leave this out of the API for the time being. @AdityaSripal We may want to remove this from the spec if we decide to leave it out of the release.

Cleanup keeper result types

* WIP Provider interface * Add definitions to the txProvider and begin migration of cosmos specific logic to cosmos provider * get connection based msgs behind TxProvider * finish implementing TxProvider interface * WIP: implementing QueryProvider * finish implementing QueryProvider * Define and implement the KeyProvider interface * Merge PR cosmos#530: Config refactor to handle provider abstraction * WIP: refactor config for provider abstraction * bump gopkg.in/yaml from v2 to v3 * refactor config for provider abstraction * better var naming & doc tweaks * Merge PR cosmos#533: Refactor core relayer code to use provider interface * refactor cli based key management * refactor path cli based commands * testing new config * WIP: refactor chain cli based commands * delete dev and testnet cmds * Merge PR cosmos#534: State Based Relaying, Reenable tests, Add Osmosis to tests * Migrate https://github.com/strangelove-ventures/relayer/pull/35 to this PR * Merge PR cosmos#535: Working tests and Strategy removal * Add osmosis to supported chains for test env * Remove more stuff and add make test-short * Add build osmosis container make file * WIP: push everything behind provider interface * WIP: push everything behind provider interface pt 2 * WIP: swap out old function calls for new provider calls * IT'S ALIVE! (bug fix time) * Most tests working! 🎉🚀💯 * update filename for fetch tests Co-authored-by: Jack Zampolin <[email protected]> * Fix test race conditions * Add back makefile test targets Co-authored-by: jtieri <[email protected]>

…2.0 (cosmos#534) Bumps [nosborn/github-action-markdown-cli](https://github.com/nosborn/github-action-markdown-cli) from 3.1.0 to 3.2.0. - [Release notes](https://github.com/nosborn/github-action-markdown-cli/releases) - [Commits](nosborn/github-action-markdown-cli@v3.1.0...v3.2.0) --- updated-dependencies: - dependency-name: nosborn/github-action-markdown-cli dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

feat: adding InitChannel util function

48f61cd

seantking requested review from AdityaSripal, colin-axner and damiannolan as code owners November 11, 2021 16:39

seantking commented Nov 11, 2021

View reviewed changes

modules/apps/27-interchain-accounts/keeper/account.go Show resolved Hide resolved

seantking commented Nov 11, 2021

View reviewed changes

seantking requested review from charleenfei and crodriguezvega November 11, 2021 16:50

colin-axner reviewed Nov 12, 2021

View reviewed changes

seantking closed this Jan 10, 2022

faddat pushed a commit to notional-labs/ibc-go that referenced this pull request Feb 23, 2022

Merge pull request cosmos#534 from CosmWasm/response_428

f8e39bf

Cleanup keeper result types

seantking deleted the sean/issue#509-init-channel branch August 3, 2022 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding InitChannel util function #534

feat: adding InitChannel util function #534

seantking commented Nov 11, 2021

seantking Nov 11, 2021

colin-axner Nov 12, 2021

seantking Nov 15, 2021

seantking Nov 11, 2021 •

edited

Loading

seantking Nov 11, 2021

colin-axner Nov 12, 2021

codecov-commenter commented Nov 11, 2021

seantking commented Nov 12, 2021

AdityaSripal commented Nov 12, 2021

colin-axner Nov 12, 2021

colin-axner Nov 12, 2021

colin-axner Nov 12, 2021

colin-axner Nov 12, 2021

colin-axner commented Nov 12, 2021

crodriguezvega commented Nov 16, 2021

colin-axner commented Dec 1, 2021

seantking commented Jan 10, 2022 •

edited

Loading

feat: adding InitChannel util function #534

feat: adding InitChannel util function #534

Conversation

seantking commented Nov 11, 2021

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantking Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 11, 2021

Codecov Report

seantking commented Nov 12, 2021

AdityaSripal commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Nov 12, 2021

crodriguezvega commented Nov 16, 2021

colin-axner commented Dec 1, 2021

seantking commented Jan 10, 2022 • edited Loading

seantking Nov 11, 2021 •

edited

Loading

seantking commented Jan 10, 2022 •

edited

Loading