Add authz and feegrant messages to default types

[webmaster128]

authz and feegrant were added to the code generator already. They add the new message types:

• cosmos.authz.v1beta1.MsgGrant
• cosmos.authz.v1beta1.MsgExec
• cosmos.authz.v1beta1.MsgRevoke
• cosmos.feegrant.v1beta1.MsgGrantAllowance
• cosmos.feegrant.v1beta1.MsgRevokeAllowance

They should be added to the Stargate registry.

For the AminoTypes it seems like no Amino JSON signing is implemented on-chain. So we should create a specific error message for that, explaining why it does not work.

[webmaster128]

This should make https://github.com/cosmos/composer/blob/master/react/src/redux/action-creator/wallet.ts#L59-L60 unnecessary.

[webmaster128]

Reopening because this part is missing:

we should create a specific error message for that, explaining why it does not work.

We should add an implementation to https://github.com/cosmos/cosmjs/blob/main/packages/stargate/src/aminotypes.ts#L67-L511 for the new message types where both toAmino and fromAmino are implemented by throwing this error: "This message type cannot be signed using the Amino JSON sign mode because this is not implemented on-chain."

[msteiner96]

I added the error in #1039 , but wonder how I should do the fromAmino() , what's the aminoType of those new added protobuf urls? @webmaster128

[crypzoh]

I have a question around the not implemented aminoTypes. That also means, that signing those kind of messages won't work with a ledger device, since it seems that when using a ledger it requires the amino signer?

[nooomski]

Just FYI, discussing right now if we should add Amino signing for Authz and will ping here if there's an issue on the SDK. Imo there's quite a few use cases for it.

[RiccardoM]

Amino for x/authz is actually implemented, it just works differently for all the other messages (e.g. the type is empty). I'm now testing implementing a custom registry for those types on our library. Will get back here if I have any news

[RiccardoM]

All right, I've tested some things and it seems possible to do it, as long as CosmJS allows for truly custom encoding/decoding when using Amino.

Context

Currently x/authz messages do support Amino encoding, but in a very custom way. They don't have the classic JSON message structure (with the type and value field), and instead the value is used as the entire JSON object.

Instead of having something like this:

{
  "type": "cosmos/MsgGrant",
  "value": {
    "grant":{
      "authorization":{},
      "expiration":"2023-02-17T15:37:29Z"
    },
    "grantee":"desmos1gedewhj0qpm55yc359e6qd36wkl9jvnp3zk90h",
    "granter":"desmos1vxe0qadrgpg52zk03k5r29wd9jt59ej664t2yl"
  }
}

We have something like this:

{
  "grant":{
    "authorization":{},
    "expiration":"2023-02-17T15:37:29Z"
  },
  "grantee":"desmos1gedewhj0qpm55yc359e6qd36wkl9jvnp3zk90h",
  "granter":"desmos1vxe0qadrgpg52zk03k5r29wd9jt59ej664t2yl"
}

Possible solution

Once possible solution is to define the following converter:

export const cosmosTypes: Record<string, AminoConverter> = {
  // Authz types
  "/cosmos.authz.v1beta1.MsgGrant": {
    aminoType: "",
    toAmino: (value: MsgGrant): AminoMsgGrant => {
      return {
        grant: value.grant ? convertGrant(value.grant) : undefined,
        granter: value.granter,
        grantee: value.grantee,
      };
    },
    fromAmino: (msg: AminoMsgGrant): MsgGrant => {
      return MsgGrant.fromPartial({
        grant: msg.grant ? convertAminoGrant(msg.grant) : undefined,
        granter: msg.granter,
        grantee: msg.grantee,
      });
    },
  },
};

The problem with this is that (after testing it), I've found out that CosmJS automatically puts everything that is returned by the toAmino method inside the value object, and then uses the specified aminoType as the type field value (thus building the standard Amino message JSON).

I think that by applying the following changes to CosmJS everything could be implemented properly:

1. allow aminoType to be undefined (or a custom type explicitly telling that this is a custom encoding - maybe something like Exclude or else)
2. during the encoding, if the aminoType value is saying the encoding should be custom, do not build the default { "type": "", "value: {} } object. Instead, use the value returned by the toAmino as the entire JSON object to be used.

[webmaster128]

Urg, that sounds like the same bug that Stargaze had in their airdrop claim message. The value was lifted up one level because the Amino Interface wrapper was not implemented properly. Isn't that a bug in the SDK?

[amaury1093]

If I'm not mistaken, the reason we have weird amino encoding for authz/feegrant is because we forgot to add the RegisterLegacyAminoCodec function for authz/feegrant. See cosmos/cosmos-sdk#11190.

We can definitely add RegisterLegacyAminoCodec for authz/feegrant in v0.46. But CosmJS must detect the SDK version, and throw an error for <0.46.

[nooomski]

I spoke with @aaronc about this yesterday, and looks like it wasn't implemented properly in the SDK yeah. I'll confirm with him and create an issue on the SDK if that's the case.

[nooomski]

Ah, there you go. Great timing. Are you okay to create an issue on the SDK @AmauryM ?

[webmaster128]

I would appreciate a solution that is consistent with the current Amino JSON framework. The lack of a type identifier makes it impossibe to do the reverse conversion (Amino JSON -> proto), would requires us to change a lot of architecture and probably break caller code.

[amaury1093]

We can track this issue on the SDK here: cosmos/cosmos-sdk#11190. There's also a PR cosmos/cosmos-sdk#11224.

Note that this will be only available starting on v0.46.

[webmaster128]

Note that this will be only available starting on v0.46.

That's fine with me. CosmJS users can change the configuration of the AminoTypes class in their application to adapt it to their backend. And then at some point the authz messages just get integrated by default assuming a 0.46+ backend. Then they can still be overriden by 0.45 projects.

For now we'll just change the term "not implemented on chain" to "not supported on chain" in #1039 and consider a non-compliant implementation "not supported".

[webmaster128]

By the way, @AmauryM, thank you for implementing Amino JSON sign mode for the new types. I have the impression this will make many people very happy and help the adotion of the new functionality.

[webmaster128]

This discussion has been summarized in https://medium.com/confio/authz-and-ledger-signing-an-executive-summary-8754a0dc2a88 in order to educate the various stakeholder. The follow-up ticket is #1092.