Add wasm08 client IBC support #1468
Conversation
There was a problem hiding this comment.
Thank you for your PR!
I have quickly skimmed through the changes and I am not sure how this relates to ics-008. I would ask to open an issue with details to discuss your feature request. It is usually a nicer way to collaborate than criticize/ defend code. I consider spikes also very helpful in supporting a conversation but expectations should be clear that a spike is for discussion only and not going to be merged.
I would also like to understand how this relates to mesh-security and the planned Juno testnet.
In this PR I can see that you are using the capability module from ibc-go. That makes sense and is required for the sdk 50 upgrade anyway. I had included this into #1448 already but as it is blocked other projects currently, I would be happy to have this replaced earlier.
So why not to a PR to use the official ibc-go v7.2.0 release instead? It would make sense to me to replace all sdk capability imports. No need to rename the import aliases capabilitykeeper/ capabilitytypes. A mixed setup should be avoided. WDYT?
go.mod
Outdated
| // Uses this fork of the wasm08 client so PFM works | ||
| // Now uses github.com/cosmos/ibc-go/modules/capability/types | ||
| // go get github.com/crodriguezvega/ibc-go/v7@24d07389e19357ff836f59a60b2a5d3897baaa08 | ||
| replace github.com/cosmos/ibc-go/v7 => github.com/crodriguezvega/ibc-go/v7 v7.0.0-20230626145354-24d07389e193 |
There was a problem hiding this comment.
why not use https://github.com/cosmos/ibc-go/releases/tag/v7.2.0 ?
There was a problem hiding this comment.
The wasm-client branch is built off 7.0.0 still right now as it has been in development for a bit. Once their review is done 7.2 will get merged in and bumped to v7.3+. This is a requirement to get the wasm-client logic in for Juno for now for testnet
https://github.com/strangelove-ventures/ibc-go/tree/feat/wasm-clients-main
This PR just pulls in the actual spec (We are using the wasm-client working branch on Juno. Without these changes, wasmd will not even compile) - CosmosContracts/juno#680
I do agree, I just made it a draft here so others could see the work was already done. I am more than happy to close if we want to wait until it is in ibc-go main. Juno's testnet will be upgrading to v47 with the wasm-client which required modifications to: Packet Forward Middleware, ASYNC-ICQ, and this wasmd modules to be compatible. This is a requirement for Composable to connect us with Kusama and Polkadot, as well as needed for mesh security per Ethan's comments. I don't think it is the best idea for us to add the mesh-security-sdk in this testnet (Juno's Uni-6), best for us to make that its own testnet most likely since it is early stages. I am more than happy to take lead on that. That testnet would also have this wasm-client support too.
The wasm-client branch is built off 7.0.0 still right now as it has been in development for a bit. Once their review is done 7.2 will get merged in and bumped
Ahh yes this is a good point. I was thinking backwards compatibility with non migrated scoped keepers, but that would never be the case since you HAVE to migrate because of the new forced types. Thanks for this insight! |
|
Thank you @Reecepbcups for the additional comments!
It is unclear to me when or even if this code will go into IBC-go main. I do not want to risk our main branch to depend on this custom fork. We did long living feature branches in the past for early SDK upgrades or some special features. This comes with quite some extra work to maintain and requires some good motivation and preparation. Usually an issue is started to discuss this with community members.
Not sure about this either. But I can see your problems with the capability module when you update your Juno
Can you point me to the comments? We discussed ADR-8 support but nothing about wasm-08 in context of MS To move this forward, I would add the spike label now and keep the branch for some time. Just to manage expectations: our capacity is very limited, please do not expect reviews on a spike. |
|
fyi: I have asked and the capability module will be shipped with the SDK 50 upgrade in IBC-Go. Well get an early alpha or RC tag to start integration. |
Based off my discussions with Strangelove, they are actively working to get it into IBC go main. Right now it looks like it is targeted for IBC v7.3.X, these commits get it to latest so it works. I will start an issue once we get it up on Juno's testnet here in the next week or 2 (pending reviews from Core-1 right now)
Disregard this comment now sorry. I talked with the Core team and we are going to put the mesh security SDK into its own testnet since this v47 upgrade + wasm client is already quite large. I let them know I will happily manage and spin this up when it comes time
Understood, I am just using it here as a placeholder since it is required for our upgrade. I do not expect any reviews as this is bleeding edge implementations |
|
We will be working with the IBC-go team on wasm-08 client support in the future. |
In draft until this goes to IBC-go official version (to remove replace statement)
We need to Juno's v47 testnet soon (mesh security as well), so needed to get this ready now