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.

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

Allow OnChanOpenTry version negotiation to call WriteOpenTryChannel in a future block #732

Closed
3 tasks
michaelfig opened this issue Mar 22, 2022 · 4 comments · Fixed by #1019
Closed
3 tasks
Labels
needs discussion tao Transport, authentication, & ordering layer.

Comments

@michaelfig
Copy link
Contributor

Summary

Allow OnChanOpenTry both to write the new state and publish the event in a future block.

Problem Definition

On the Agoric chain, we're using async messaging to implement smart contracts. This works fine with IBC and async acknowledgements, but the ibc-go version negotiation is unfortunately synchronous. We'd like to allow Agoric users to implement channel version negotiation in a smart contract, but the synchronous nature defeats that goal.

Benefits of sorting this out would be to reduce cross-chain synchronous coupling, which may be an issue for other chains with similar async requirements.

Disadvantage is the need to reassess the IBC standard to lift the requirement for synchronous behaviour.

Proposal

This is a strawman.

Without changing the API presented to synchronous OnChanOpenTry implementations, we can define a constant Error that indicates the need for postponed async operation.

  1. return "", types.MagicalAsyncError from OnChanOpenTry
  2. In modules/core/keeper/msg_server.go ChannelOpenTry, make this modification:
 	// Perform application logic callback
	version, err := cbs.OnChanOpenTry(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.CounterpartyVersion)
        if err == types.MagicalAsyncError {
                // Bail out to have a future block write the state
                return &channeltypes.MsgChannelOpenTryResponse{}, nil
        }
  1. Call k.ChannelKeeper.WriteOpenTryChannel explicitly at a later time.

We would also need some way to write a failed open try into the state as well, to interrupt the handshake if no version negotiation can be successfully completed.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@michaelfig
Copy link
Contributor Author

Tagging @AdityaSripal for comment and any counterproposal.

Tagging @schnetzlerjoe as somebody negatively affected by the current synchronous situation.

@michaelfig
Copy link
Contributor Author

I anticipated something like this when proposing passive relayers. For contrast, here is the solution I used then: https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-025-ibc-passive-channels.md#handling-channel-open-attempts

@AdityaSripal AdityaSripal transferred this issue from cosmos/ibc-go May 3, 2022
@AdityaSripal
Copy link
Member

cc: @mpoke

@schnetzlerjoe could you detail how the synchronous situation is affecting you?

@michaelfig
Copy link
Contributor Author

@schnetzlerjoe could you detail how the synchronous situation is affecting you?

Speaking on behalf of @schnetzlerjoe since I was working with him, lack of async version negotiation makes it impossible to implement a dIBC (async-only) Interchain Accounts host. We just avoided doing that, as implementing an ICA controller only creates outbound connections to hosts, thus the controller doesn't need to handle OnChanOpenTry.

What I'm concerned about is that ICA hosts (or some future protocol) will require version negotiation, and that will make it impossible to implement in dIBC. That will be a blocker for some IBC applications on Agoric, we just aren't feeling the pain yet of being blocked from creating an ICA host.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion tao Transport, authentication, & ordering layer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants