MsgTransfer For Eureka #7957
MsgTransfer For Eureka #7957
Conversation
| // ClientKeeper defines the expected IBC client keeper | ||
| type ClientKeeper interface { | ||
| GetClientConsensusState(ctx sdk.Context, clientID string) (connection ibcexported.ConsensusState, found bool) |
There was a problem hiding this comment.
nonsensical method not used
| _, isIBCV2 = k.clientKeeperV2.GetClientCounterparty(ctx, msg.SourceChannel) | ||
| if !isIBCV2 { | ||
| var found bool | ||
| channel, found = k.channelKeeper.GetChannel(ctx, msg.SourcePort, msg.SourceChannel) | ||
| if !found { | ||
| return nil, errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", msg.SourcePort, msg.SourceChannel) |
There was a problem hiding this comment.
i could choose to not have clientKeeperV2 in transfer keeper and just send the channelID from the msg directly to chanKeeperV2 and have it error if counterparty doesn't exist. But then we get a counterparty not found error over the channel not found error
| if isIBCV2 { | ||
| encoding := msg.Encoding | ||
| if encoding == "" { | ||
| encoding = types.EncodingJSON |
There was a problem hiding this comment.
default to JSON since that is expected ics20-v1 behaviour
| sender.String(), payload, | ||
| ) | ||
|
|
||
| res, err := k.channelKeeperV2.SendPacket(ctx, msg) |
There was a problem hiding this comment.
SendPacket will call transfer module's OnSendPacket
| @@ -61,6 +61,27 @@ func NewMsgTransfer( | |||
| } | |||
| } | |||
|
|
|||
| // NewMsgTransferWithEncoding creates a new MsgTransfer instance | |||
| // with the provided encoding | |||
| func NewMsgTransferWithEncoding( | |||
There was a problem hiding this comment.
Allow users to pass in custom encoding
| @@ -289,6 +290,15 @@ func (suite *TransferTestSuite) TestOnAckPacket() { | |||
| suite.Require().True(ok) | |||
| originalCoin := sdk.NewCoin(tc.sourceDenomToTransfer, amount) | |||
|
|
|||
| msg := types.NewMsgTransferWithEncoding( | |||
There was a problem hiding this comment.
Use the transfer endpoint rather than channelKeeper.SendPacket
| ics4Wrapper porttypes.ICS4Wrapper | ||
| channelKeeper types.ChannelKeeper | ||
| channelKeeperV2 types.ChannelKeeperV2 | ||
| clientKeeperV2 types.ClientKeeperV2 |
There was a problem hiding this comment.
I could remove this if desired
There was a problem hiding this comment.
It would be nice to have less stuff to put in here, but what would that mean for the code?
|
I reviewed this yesterday, and came to the conclusion that I want to have a more indepth disucssion on how we do this one, because I am not too comfortable with the flag paths needed to make this work. |
|
refactored to have most of the logic pulled out in 2 different fns. If its still not desirable, we can revisit the approach |
|
Not sure why there's so much proto-gen diffs |
| ics4Wrapper porttypes.ICS4Wrapper | ||
| channelKeeper types.ChannelKeeper | ||
| channelKeeperV2 types.ChannelKeeperV2 | ||
| clientKeeperV2 types.ClientKeeperV2 |
There was a problem hiding this comment.
It would be nice to have less stuff to put in here, but what would that mean for the code?
Co-authored-by: Gjermund Garaba <gjermund@garaba.net>
|
* initial progress * progress * initial build and existing tests pass * testing * documentation for tests * refactor * fix regression * Update modules/apps/transfer/types/packet.go Co-authored-by: Gjermund Garaba <gjermund@garaba.net> * remove clientKeeperv2 necessity in code * remove clientkeeperv2 in transfer keeper --------- Co-authored-by: Gjermund Garaba <gjermund@garaba.net> (cherry picked from commit 9f677ba)
* initial progress * progress * initial build and existing tests pass * testing * documentation for tests * refactor * fix regression * Update modules/apps/transfer/types/packet.go Co-authored-by: Gjermund Garaba <gjermund@garaba.net> * remove clientKeeperv2 necessity in code * remove clientkeeperv2 in transfer keeper --------- Co-authored-by: Gjermund Garaba <gjermund@garaba.net> (cherry picked from commit 9f677ba) Co-authored-by: Aditya <14364734+AdityaSripal@users.noreply.github.com>
Description
closes: #7958
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.