Use a single memo field in MsgTransfer

[crodriguezvega]

This issue explores an idea suggested by @damiannolan. All credits go to him!

---

With path forwarding we have added the forwarding field to MsgTransfer to allow users specify the forwarding hops and a memo to execute on the final destination. So that means that MsgTransfer contains now two memo fields. @damiannolan suggested go back to having just one memo field.

I can think of two ways we could change MsgTransfer proto message:

Add hops to MsgTrasfer

We have just the hops field in MsgTransfer:

@@ -51,8 +51,8 @@ message MsgTransfer {
   string memo = 8;
   // tokens to be transferred
   repeated cosmos.base.v1beta1.Coin tokens = 9 [(gogoproto.nullable) = false];
-  // optional forwarding information
-  Forwarding forwarding = 10;
+  // optional forwarding hops
+  repeated Hop hops = 1 [(gogoproto.nullable) = false];
 }

The existing memo field will be used for both single hop (regular) transfers and multi-hop transfers. sendTransfer receives the hops slice:

@@ -64,7 +64,7 @@ func (k Keeper) sendTransfer(
        timeoutHeight clienttypes.Height,
        timeoutTimestamp uint64,
        memo string,
-       forwarding *types.Forwarding,
+       hops []types.Hop,
 ) (uint64, error) {
        channel, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel)
        if !found {

And we modify createPacketDataBytesFromVersion also to take in hops and create a forwarding struct if there are hops and fill in its memo (and setting the regular memo to empty string):

@@ -431,7 +431,16 @@ func (k Keeper) tokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
 }
 
 // createPacketDataBytesFromVersion creates the packet data bytes to be sent based on the application version.
-func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, forwarding *types.Forwarding) []byte {
+func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
+       var forwarding *types.Forwarding
+       if len(hops) > 0 {
+               forwarding = &types.Forwarding{
+                       Hops: hops,
+                       Memo: memo,
+               }
+               memo = ""
+       }
+
        var packetDataBytes []byte
        switch appVersion {
        case types.V1:

Then in ValidateBasic of MsgTransfer we just need to check that if hops is not empty, that the elements contain valid identifiers.

Keep forwarding with only hops

We keep the forwarding field, but we create a different Forwarding (or other name) message that has only hops and no memo. We keep the existing Forwarding message only for FungibleTokenPacketDataV2 (I believe there wouldn't be a way to avoid not having the two memo fields in the packet data, unfortunately).

I think I like the first approach. The only disadvantage (which is an advantage for the second approach) is that in case we add more forwarding-related information to MsgTransfer then it wouldn't be grouped in a forwarding field. What do people think? What approach (if any of these) do you prefer? After writing #6624, I think the second approach makes more sense.

[DimitrisJim]

Decided to go forward with single memo on msg transfer and refactor protos to better separate these:

MsgTransfer now gets a field of type Forwarding containing:

message Forwarding {
    unwind bool
    hops     []Hops
}

where unwind is described in #6624

For the packet data message, we'll now add:

message ForwardingPacketData { 
    destinationMemo string
    hops            []Hops
}

basically a rename of the memo field to better describe its puprose.

If Forwarding is specified (unwind == true || len(hops) > 0) on msg transfer, the memo passed in will be forwarded to the packet's ForwardingPacketData.destinationMemo. This should also simplify some validation which needed to ensure a single memo is present.

[crodriguezvega]

Closed by #6655