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

feat: changes for ics #3496

Closed
wants to merge 1 commit into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Apr 20, 2023

Description

This PR is part of an effort to support sdk 47 and ibc7 without forks. We
would love review and feedback, but it shouldn't be considered for merging
at this time.

It relates to this PR on ICS:

cosmos/interchain-security#817

Commit Message / Changelog Entry

pending. We don't even know if this should be merged, and the pr is being
made so that we can get advice.


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@crodriguezvega
Copy link
Contributor

Thanks, @faddat. Could you please give us some more context/information of why this change would be needed?

If I look at cosmos/interchain-security#817 and I see the item Switch to forked vesion for ibc-go to bypass the passing of pointer to empty struct in IBCKeeper.NewKeeper, does this mean that for ICS they need to be able to set a pointer to empty struct for the staking keeper? It sounds a bit strange to me that they would want that, so could you elaborate why that is needed?

@faddat
Copy link
Contributor Author

faddat commented Apr 24, 2023

I cannot, sorry. @vuong177 can. This needs to be linked to this:

cosmos/interchain-security#852

and

cosmos/cosmos-sdk#15917

@sontrinh16
Copy link
Member

sontrinh16 commented Apr 24, 2023

@crodriguezvega

hey yeah it weird please see the code lines in the link https://github.com/cosmos/interchain-security/blob/main/app/consumer/app.go#L317-L342. So we need the consumerkeeper to act as stakingkeeper in the consumer chain to pass in the IBCKeeper, while the consumerkeeper also need to pass in some properties of the IBCKeeper in order to do that IBCkeeper need to be initialize first. so to make this work they will pass the pointer to a empty struct first (consumerkeeper) then assign the value to where the pointer is pointed to. This use to work for ibc-go v4 since IBCKeeper only check for empty struct for stakingkeeper so in order for ics to work with ibc-go v7 we need to let the pointer to empty struct go throu. I hope that make sense

@faddat faddat marked this pull request as ready for review April 25, 2023 04:36
@faddat
Copy link
Contributor Author

faddat commented Apr 25, 2023

@crodriguezvega would this be better if it targeted main?

Also do you happen to see another way around it?

thank you.

@faddat faddat changed the base branch from release/v7.0.x to main April 25, 2023 04:37
@faddat faddat changed the base branch from main to release/v7.0.x April 25, 2023 04:37
@faddat
Copy link
Contributor Author

faddat commented Apr 25, 2023

@sontrinh16 I am going to make a pr that targets main, I think that the ibc-go team is preparing v8.0.0 right now. I'll also leave this one open. @crodriguezvega thanks very much for your review.

@mergify mergify bot mentioned this pull request Apr 30, 2023
9 tasks
@crodriguezvega
Copy link
Contributor

Thanks for the explanations and the extra context, @sontrinh16 and @faddat. I see in the interchain-security repo that an alternative approach is being followed for the upgrade to SDK 0.47 and ibc-go v7. Does that mean that we could close this PR?

@sontrinh16
Copy link
Member

sontrinh16 commented May 15, 2023

Thanks for the explanations and the extra context, @sontrinh16 and @faddat. I see in the interchain-security repo that an alternative approach is being followed for the upgrade to SDK 0.47 and ibc-go v7. Does that mean that we could close this PR?

yes i think we can close this one

@crodriguezvega
Copy link
Contributor

yes i think we can close this one

Thank you for the quick reply, @sontrinh16!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants