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

Persist channel state before calling OnChanOpenAck #2962

Closed
3 tasks
alpe opened this issue Dec 21, 2022 · 5 comments · Fixed by #2973
Closed
3 tasks

Persist channel state before calling OnChanOpenAck #2962

alpe opened this issue Dec 21, 2022 · 5 comments · Fixed by #2973
Assignees
Labels
Milestone

Comments

@alpe
Copy link
Contributor

alpe commented Dec 21, 2022

Summary

We have this code smell in x/wasm/ibc.go for backwards compatibility . Can you set the channel state before OnChanOpenAck is called (as it was in the past) ? It probably does not make a big difference for you but we need the channel active for the contract. The ack callback may trigger the first IBC package to be sent.


For Admin Use

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

Thanks for opening this issue, @alpe! If I understand it correctly, your request is to move this line before this one, right?

How much are you blocked on this? Would you require this change for the upgrade of wasmd to SDK 0.47?

@alpe
Copy link
Contributor Author

alpe commented Dec 21, 2022

, your request is to move this line before this one, right?

Correct 👍

How much are you blocked on this?

We are not blocked by this as we persist the channel state ourself. But that is a hack that I would like to see removed in our code as soon as possible.

Would you require this change for the upgrade of wasmd to SDK 0.47?

This is unrelated to the SDK upgrade. A backport to v4.2 would be great to so that we can apply this early

@crodriguezvega
Copy link
Contributor

All right. Thanks for the clarifications!

@damiannolan
Copy link
Contributor

damiannolan commented Dec 22, 2022

afaik we made these changes just for consistency when adding the application version negotiation return values for OnChanOpenInit and OnChanOpenTry.
I think it should be fine to change ChanOpenAck to write the channel before the app cbs are invoked, but we should probably do the same for ChanOpenConfirm also. cc @AdityaSripal

@AdityaSripal
Copy link
Member

afaik we made these changes just for consistency when adding the application version negotiation return values for OnChanOpenInit and OnChanOpenTry.
I think it should be fine to change ChanOpenAck to write the channel before the app cbs are invoked, but we should probably do the same for ChanOpenConfirm also. cc @AdityaSripal

Yes that is correct

@github-project-automation github-project-automation bot moved this from In review to Todo in ibc-go Jan 18, 2023
@crodriguezvega crodriguezvega moved this from Todo to Done in ibc-go Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done 🥳
4 participants