Minimal ibcv3 upgrade #824
Conversation
|
@faddat @vuongnguyen17 I have pulled out the minimal portion of the ibcv3 upgrade here. It does not yet wire up ICA in order to keep this truly minimal. I used as many of your commits as I could and then fixed up a few still-missing things (or cherry-picking conflicts) I would love your review of this. Also, if you wish to make a PR on top adding ICA, I would be happy to merge (or I will do so next week). Please do not change imports or rename variables as part of this PR. Those can be separate and make it harder to review and merge. |
Codecov Report
@@ Coverage Diff @@
## main #824 +/- ##
==========================================
+ Coverage 59.60% 59.63% +0.02%
==========================================
Files 52 52
Lines 5944 5948 +4
==========================================
+ Hits 3543 3547 +4
Misses 2139 2139
Partials 262 262
|
| } | ||
| } | ||
| return nil | ||
| return counterpartyVersion, nil |
There was a problem hiding this comment.
This as a default makes sense. The contract can return an error when incompatible
| @@ -133,6 +136,8 @@ func (i IBCHandler) OnChanOpenAck( | |||
| if !ok { | |||
| return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | |||
| } | |||
| channelInfo.Counterparty.ChannelId = counterpartyChannelID | |||
| i.channelKeeper.SetChannel(ctx, portID, channelID, channelInfo) | |||
There was a problem hiding this comment.
I stumbled upon this line as the IBC module is responsible for managing the channel data and lifecycle IMHO. When I remove this line, the TestIBCReflectContract contract fails though. After some debugging, I found out that this is due to changes in v3:
Channel state will not be set before application callback
This is quite unfortunate as we need the counterparty channel ID in the handler_plugin
Storing it here is quite pragmatic but has some smell as the wasm module does not own the data. Not sure about other side effects.
Please add some code doc why we need this. 🙏
ethanfrey
force-pushed
the
minimal-ibcv3-upgrade
branch
from
0b90ef6 to
7704459
Compare
First part of #806
Original commits pulled from #793
I just try the upgrade to ibcv3, without renaming, or even the addition of ICA.
I would consider both of those follow up PRs