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 interchain accounts to connect with middleware #1356

Closed
crodriguezvega opened this issue May 16, 2022 · 4 comments · Fixed by #1432
Closed

Allow interchain accounts to connect with middleware #1356

crodriguezvega opened this issue May 16, 2022 · 4 comments · Fixed by #1432
Assignees
Labels
27-interchain-accounts 29-fee needs discussion Issues that need discussion before they can be worked on

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented May 16, 2022

Currently, the OnChanOpenInit callback stack is started by a module calling RegisterInterchainAccount. RegisterInterchainAccount will construct a valid ics27 version. It does not take in any information about potential middleware versions above the ics27 version

@colin-axner colin-axner changed the title Allowing interchain accounts to be connected to any middleware Allow interchain accounts to specify middleware versions in RegisterInterchainAccount May 16, 2022
@colin-axner colin-axner changed the title Allow interchain accounts to specify middleware versions in RegisterInterchainAccount Allow interchain accounts to connect with middleware May 16, 2022
@crodriguezvega crodriguezvega moved this to Todo in ibc-go May 17, 2022
@crodriguezvega crodriguezvega added the needs discussion Issues that need discussion before they can be worked on label May 17, 2022
@crodriguezvega
Copy link
Contributor Author

crodriguezvega commented May 17, 2022

Trying to write down what's needed for this issue to start the discussion...

As discussed in the call today, we need to change the signature of the controller's RegisterInterchainAccount function to something like:

RegisterInterchainAccount(ctx sdk.Context, connectionID, owner, version string) error

Questions:

  • Should the version string be a JSON-encoded string that un-marshalls to 29-fee's Metadata type?
  • Should we do any kind of validation for the version parameter in RegisterInterchainAccount or just pass it through as it is down to channeltypes.NewMsgChannelOpenInit? Like do we need the AppVersion field to be populated at all with some value (maybe ics27-1?), or can it be just an empty string?
  • Will the current JSON-encoded metadata that we marshall as the version string be used to populate the AppVersion field of 29-fee's Metadata type?

@colin-axner
Copy link
Contributor

Should the version string be a JSON-encoded string that un-marshalls to 29-fee's Metadata type?

The version is the full channel version. We cannot assume what the underlying structure is at this entrypoint. The version should simply be passed to the MsgChanOpenInit

Should we do any kind of validation for the version parameter in RegisterInterchainAccount or just pass it through as it is down to channeltypes.NewMsgChannelOpenInit? Like do we need the AppVersion field to be populated at all with some value (maybe ics27-1?), or can it be just an empty string?

No, we cannot since we cannot unwrap the version yet. Checks will happen within OnChanOpenInit

Will the current JSON-encoded metadata that we marshall as the version string be used to populate the AppVersion field of 29-fee's Metadata type?

yes assuming the middleware stacking is ics29 -> ics27 (but there could be many middlewares in between the two)

@crodriguezvega
Copy link
Contributor Author

Ok, thanks.

So then we need to remove this block of code and pass the version argument directly in here. Correct?

Are there any other changes needed?

@colin-axner
Copy link
Contributor

I think that should work! A test of course should be added to check that this assumption holds

@damiannolan damiannolan self-assigned this May 23, 2022
@damiannolan damiannolan moved this from Todo to In progress in ibc-go May 23, 2022
Repository owner moved this from In progress to Done in ibc-go Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-interchain-accounts 29-fee needs discussion Issues that need discussion before they can be worked on
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants