(Nested) `Any` from/toAmino not symmetrical

[fmorency]

Problem

The call to fromAmino removes the nested message type indicator in a group proposal, causing an issue when multiple message types contain the same fields.

Context

Telescope configuration

The POA module contains MsgRemovePending and MsgRemoveValidator differing only by message name, i.e., the message fields are the same in both messages.

The remove pending validator test creates a Group Proposal containing a Remove Pending Validator message, submits the proposal, vote on it and execute the proposal.

Encoding and signing the message using AMINO works fine

{
  "chain_id": "manifest-ledger-beta",
  "account_number": "2",
  "sequence": "8",
  "fee": {
    "amount": [
      {
        "denom": "umfx",
        "amount": "100000"
      }
    ],
    "gas": "550000"
  },
  "msgs": [
    {
      "type": "cosmos-sdk/group/MsgSubmitProposal",
      "value": {
        "group_policy_address": "manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj",
        "proposers": [
          "manifest1pss7nxeh3f9md2vuxku8q99femnwdjtcwhpj7f"
        ],
        "messages": [
          {
            "type": "poa/MsgRemovePending",
            "value": {
              "sender": "manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj",
              "validator_address": "manifestvaloper1pss7nxeh3f9md2vuxku8q99femnwdjtcjhuxjm"
            }
          }
        ],
        "title": "remove pending",
        "summary": "some remove pending"
      }
    }
  ],
  "memo": ""
}

The nested message is of the right type. However, the signedTxBodyEncodedObject created in the signAmino method and broadcasted to the server is

{
  "typeUrl": "/cosmos.tx.v1beta1.TxBody",
  "value": {
    "messages": [
      {
        "typeUrl": "/cosmos.group.v1.MsgSubmitProposal",
        "value": {
          "groupPolicyAddress": "manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj",
          "proposers": [
            "manifest1pss7nxeh3f9md2vuxku8q99femnwdjtcwhpj7f"
          ],
          "metadata": "",
          "messages": [
            {
              "sender": "manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj",
              "validatorAddress": "manifestvaloper1pss7nxeh3f9md2vuxku8q99femnwdjtcjhuxjm"
            }
          ],
          "exec": 0,
          "title": "remove pending",
          "summary": "some remove pending"
        }
      }
    ],
    "memo": ""
  }
}

Notice that the nested message field has no type indicator. This field is created by the call to fromAmino

const signedTxBody = {
    messages: signed.msgs.map((msg) => this.aminoTypes.fromAmino(msg)),
    memo: signed.memo,
    timeoutHeight: timeoutHeight,
};

The message decoded by the server is of type poa/RemoveValidator instead of poa/RemovePending causing the signature check to fail. I suspect the lack of type indication in the nested message causes the issue. One is unable to match the type only from the instance.

Everything works fine when using the DIRECT signer. Everything also works fine using the AMINO signer from the manifestd CLI.

Running the test

yarn starship start
yarn starship:test starship/__tests__/poa.group.test.ts

[fmorency]

I also tried using the MsgSubmitProposalEncoded as

...
  const submitMsg: MsgSubmitProposalEncoded = {
    groupPolicyAddress: POA_GROUP_ADDRESS,
    title,
    summary,
    proposers,
    exec: Exec.EXEC_UNSPECIFIED,
    messages: messages.map((msg) => Any.toProtoMsg(msg)),
    metadata: "",
  }

  const msg = GroupMessageComposer.fromPartial.submitProposal(submitMsg);
  const result = await client.signAndBroadcast(signer, [msg], fee);
  ...

where messages[0] is, e.g.,

ManifestMessageComposer.encoded.payout({
  authority: POA_GROUP_ADDRESS,
  payoutPairs: [
    {
      address: test2Address,
      coin: { denom, amount: "1000" },
    },
  ],
});

without luck. I'm getting the following error

TypeError: decoder.toAminoMsg is not a function

      196 |     }
      197 |
    > 198 |     return decoder.toAminoMsg!(data);
          |                    ^
      199 |   }
      200 | }
      201 |

      at Function.toAminoMsg (src/codegen/registry.ts:198:20)
      at src/codegen/cosmos/group/v1/tx.ts:2870:74
          at Array.map (<anonymous>)
      at Object.toAmino (src/codegen/cosmos/group/v1/tx.ts:2870:39)
      at AminoTypes.toAmino (node_modules/@cosmjs/stargate/src/aminotypes.ts:40:24)
      at node_modules/@cosmjs/stargate/src/signingstargateclient.ts:404:56
          at Array.map (<anonymous>)
      at SigningStargateClient.signAmino (node_modules/@cosmjs/stargate/src/signingstargateclient.ts:404:27)
      at SigningStargateClient.signAndBroadcast (node_modules/@cosmjs/stargate/src/signingstargateclient.ts:319:19)
      at submitGroupProposal (starship/src/test_helper.ts:105:18)
      at submitVoteExecGroupProposal (starship/src/test_helper.ts:162:22)
      at Object.<anonymous> (starship/__tests__/manifest.group.test.ts:123:5)

[fmorency]

The issue is a Telescope issue but stems from how CosmJS does things in .signAmino.

// signAmino()
...
const msgs =  messages.map((msg) => this.aminoTypes.toAmino(msg));
...
const signedTxBody = {
        messages: signed.msgs.map((msg) => this.aminoTypes.fromAmino(msg)),
        memo: signed.memo,
        timeoutHeight: timeoutHeight,
    };
...

The toAmino and fromAmino are NOT symmetrical in the case of nested Any, meaning that converting the Any from Proto to Amino and back doesn't result in the original Any.

In MsgSubmitPropoal, the fromAmino function performs the following

message.messages = object.messages?.map(e => GlobalDecoderRegistry.fromAminoMsg(e)) || [];

where messages.messages is an Any[]. The e variable is of type AnyAmino. The call to fromAminoMsg call the generic fromAminoMsg method from the registry where the proper decoder is fetched and decoder.fromAminoMsg is called, effectively calling the proper fromAminoMsg function from the object concrete type.

At this point, the concrete type of the message is lost because fromAminoMsg returns a generic object type which is what gets embedded in the message.messages field.

This is wrong.

Workaround

I modified CosmJS so as not to call fromAmino, i.e.,

const signedTxBody = {
        messages,
        memo: signed.memo,
        timeoutHeight: timeoutHeight,
    };

and things started working properly.

[fmorency]

I was debugging some FE code last Friday and discovered that toSDK/fromSDK are also impacted by this. The fromSDK function is not able to properly decode the nested Any because it is represented as its JSON mapping, e.g.,

messages": [
        {
          "@type": "/cosmos.bank.v1beta1.MsgSend",
          "from_address": "manifest1c799jddmlz7segvg6jrw6w2k6svwafganjdznard3tc74n7td7rqdqe6ks",
          "to_address": "manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct",
          "amount": [
            {
              "denom": "umfx",
              "amount": "12000000"
            }
          ]
        }
      ],

According to the ProtoJSON Format,

If the Any contains a value that has a special JSON mapping, it will be converted as follows: {"@type": xxx, "value": yyy}. Otherwise, the value will be converted into a JSON object, and the "@type" field will be inserted to indicate the actual data type.

I believe the code generated by Telescope only covers the 1st case.