Merge pull request #1046 from cosmos/explain-AminoTypes-constructor

Check uniqueness of Amino type in fromAmino

CHANGELOG.md

@@ -30,6 +30,10 @@ and this project adheres to
   - cosmos.authz.v1beta1.MsgRevoke
   - cosmos.feegrant.v1beta1.MsgGrantAllowance
   - cosmos.feegrant.v1beta1.MsgRevokeAllowance
+- @cosmjs/stargate: In `AminoTypes` the uniqueness of the Amino type identifier
+  is checked in `fromAmino` now instead of the constructor. This only affects
+  you if multiple different protobuf type URLs map to the same Amino type
+  identifier which should not be the case anyways.
 
 [#989]: https://github.com/cosmos/cosmjs/issues/989
 [#994]: https://github.com/cosmos/cosmjs/issues/994

packages/stargate/src/aminotypes.spec.ts

@@ -41,6 +41,79 @@ import {
 import { AminoTypes } from "./aminotypes";
 
 describe("AminoTypes", () => {
+  describe("constructor", () => {
+    const msg: MsgDelegate = {
+      delegatorAddress: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6",
+      validatorAddress: "cosmos10dyr9899g6t0pelew4nvf4j5c3jcgv0r73qga5",
+      amount: coin(1234, "ucosm"),
+    };
+
+    it("can override type by type URL", () => {
+      const types = new AminoTypes({
+        prefix: "cosmos",
+        additions: {
+          "/cosmos.staking.v1beta1.MsgDelegate": {
+            aminoType: "my-override/MsgDelegate",
+            toAmino: (m: MsgDelegate): { readonly foo: string } => ({
+              foo: m.delegatorAddress ?? "",
+            }),
+            fromAmino: () => ({
+              bar: 123,
+            }),
+          },
+        },
+      });
+
+      const aminoMsg = types.toAmino({
+        typeUrl: "/cosmos.staking.v1beta1.MsgDelegate",
+        value: msg,
+      });
+      expect(aminoMsg).toEqual({
+        type: "my-override/MsgDelegate",
+        value: {
+          foo: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6",
+        },
+      });
+      expect(types.fromAmino(aminoMsg)).toEqual({
+        typeUrl: "/cosmos.staking.v1beta1.MsgDelegate",
+        value: {
+          bar: 123,
+        },
+      });
+    });
+
+    it("can override type with Amino type collision", () => {
+      const types = new AminoTypes({
+        prefix: "cosmos",
+        additions: {
+          "/cosmos.staking.otherVersion456.MsgDelegate": {
+            aminoType: "cosmos-sdk/MsgDelegate",
+            toAmino: (m: MsgDelegate): { readonly foo: string } => ({
+              foo: m.delegatorAddress ?? "",
+            }),
+            fromAmino: () => ({
+              bar: 123,
+            }),
+          },
+        },
+      });
+
+      const aminoMsg = types.toAmino({
+        typeUrl: "/cosmos.staking.otherVersion456.MsgDelegate",
+        value: msg,
+      });
+      expect(aminoMsg).toEqual({
+        type: "cosmos-sdk/MsgDelegate",
+        value: {
+          foo: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6",
+        },
+      });
+      expect(() => types.fromAmino(aminoMsg)).toThrowError(
+        "Multiple types are registered with Amino type identifier 'cosmos-sdk/MsgDelegate': '/cosmos.staking.otherVersion456.MsgDelegate', '/cosmos.staking.v1beta1.MsgDelegate'. Thus fromAmino cannot be performed.",
+      );
+    });
+  });
+
   describe("toAmino", () => {
     // bank
 
@@ -975,12 +1048,12 @@ describe("AminoTypes", () => {
       });
     });
 
-    it("works with overridden type url", () => {
+    it("works with overridden type URL", () => {
       const msg = new AminoTypes({
         prefix: "cosmos",
         additions: {
-          "/my.OverrideType": {
-            aminoType: "cosmos-sdk/MsgDelegate",
+          "/cosmos.staking.v1beta1.MsgDelegate": {
+            aminoType: "cosmos-sdk/MsgDelegate2",
             toAmino: () => {},
             fromAmino: ({ foo }: { readonly foo: string }): MsgDelegate => ({
               delegatorAddress: foo,
@@ -990,20 +1063,19 @@ describe("AminoTypes", () => {
           },
         },
       }).fromAmino({
-        type: "cosmos-sdk/MsgDelegate",
+        type: "cosmos-sdk/MsgDelegate2",
         value: {
           foo: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6",
         },
       });
-      const expected: { readonly typeUrl: "/my.OverrideType"; readonly value: MsgDelegate } = {
-        typeUrl: "/my.OverrideType",
+      expect(msg).toEqual({
+        typeUrl: "/cosmos.staking.v1beta1.MsgDelegate",
         value: {
           delegatorAddress: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6",
           validatorAddress: "cosmos10dyr9899g6t0pelew4nvf4j5c3jcgv0r73qga5",
           amount: coin(1234, "ucosm"),
         },
-      };
-      expect(msg).toEqual(expected);
+      });
     });
 
     it("throws for unknown type url", () => {

packages/stargate/src/aminotypes.ts

@@ -524,18 +524,15 @@ export interface AminoTypesOptions {
  * to Amino types.
  */
 export class AminoTypes {
+  // The map type here ensures uniqueness of the protobuf type URL in the key.
+  // There is no uniqueness guarantee of the Amino type identifier in the type
+  // system or constructor. Instead it's the user's responsibility to ensure
+  // there is no overlap when fromAmino is called.
   private readonly register: Record<string, AminoConverter>;
 
   public constructor({ prefix, additions = {} }: AminoTypesOptions) {
-    const additionalAminoTypes = Object.values(additions);
-    const filteredDefaultTypes = Object.entries(createDefaultTypes(prefix)).reduce(
-      (acc, [key, value]) =>
-        additionalAminoTypes.find(({ aminoType }) => value.aminoType === aminoType)
-          ? acc
-          : { ...acc, [key]: value },
-      {},
-    );
-    this.register = { ...filteredDefaultTypes, ...additions };
+    const defaultTypes = createDefaultTypes(prefix);
+    this.register = { ...defaultTypes, ...additions };
   }
 
   public toAmino({ typeUrl, value }: EncodeObject): AminoMsg {
@@ -554,18 +551,31 @@ export class AminoTypes {
   }
 
   public fromAmino({ type, value }: AminoMsg): EncodeObject {
-    const result = Object.entries(this.register).find(([_typeUrl, { aminoType }]) => aminoType === type);
-    if (!result) {
-      throw new Error(
-        `Amino type identifier '${type}' does not exist in the Amino message type register. ` +
-          "If you need support for this message type, you can pass in additional entries to the AminoTypes constructor. " +
-          "If you think this message type should be included by default, please open an issue at https://github.com/cosmos/cosmjs/issues.",
-      );
+    const matches = Object.entries(this.register).filter(([_typeUrl, { aminoType }]) => aminoType === type);
+    switch (matches.length) {
+      case 0: {
+        throw new Error(
+          `Amino type identifier '${type}' does not exist in the Amino message type register. ` +
+            "If you need support for this message type, you can pass in additional entries to the AminoTypes constructor. " +
+            "If you think this message type should be included by default, please open an issue at https://github.com/cosmos/cosmjs/issues.",
+        );
+      }
+      case 1: {
+        const [typeUrl, converter] = matches[0];
+        return {
+          typeUrl: typeUrl,
+          value: converter.fromAmino(value),
+        };
+      }
+      default:
+        throw new Error(
+          `Multiple types are registered with Amino type identifier '${type}': '` +
+            matches
+              .map(([key, _value]) => key)
+              .sort()
+              .join("', '") +
+            "'. Thus fromAmino cannot be performed.",
+        );
     }
-    const [typeUrl, converter] = result;
-    return {
-      typeUrl: typeUrl,
-      value: converter.fromAmino(value),
-    };
   }
 }