From 10d101b5fcfbb77659f5be9add54107cd693db97 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 17 Feb 2022 17:28:04 +0100 Subject: [PATCH] Check uniqueness of Amino type in fromAmino --- CHANGELOG.md | 4 ++ packages/stargate/src/aminotypes.spec.ts | 27 +++++------ packages/stargate/src/aminotypes.ts | 58 ++++++++++++------------ 3 files changed, 45 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15b6551eaf..8a54b32875 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/stargate/src/aminotypes.spec.ts b/packages/stargate/src/aminotypes.spec.ts index f6d1efb2aa..d2fd70e31c 100644 --- a/packages/stargate/src/aminotypes.spec.ts +++ b/packages/stargate/src/aminotypes.spec.ts @@ -82,8 +82,7 @@ describe("AminoTypes", () => { }); }); - // This is a feature we have right now but we don't need - it("can override type by Amino type", () => { + it("can override type with Amino type collision", () => { const types = new AminoTypes({ prefix: "cosmos", additions: { @@ -109,12 +108,9 @@ describe("AminoTypes", () => { foo: "cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6", }, }); - expect(types.fromAmino(aminoMsg)).toEqual({ - typeUrl: "/cosmos.staking.otherVersion456.MsgDelegate", - value: { - bar: 123, - }, - }); + 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.", + ); }); }); @@ -1052,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, @@ -1067,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", () => { diff --git a/packages/stargate/src/aminotypes.ts b/packages/stargate/src/aminotypes.ts index adca645a8e..efc8e1cde2 100644 --- a/packages/stargate/src/aminotypes.ts +++ b/packages/stargate/src/aminotypes.ts @@ -519,31 +519,20 @@ export interface AminoTypesOptions { readonly additions?: Record; } -function sameAminoType(a: AminoConverter, b: AminoConverter): boolean { - return a.aminoType === b.aminoType; -} - /** * A map from Stargate message types as used in the messages's `Any` type * 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; public constructor({ prefix, additions = {} }: AminoTypesOptions) { const defaultTypes = createDefaultTypes(prefix); - const additionalAminoTypes = Object.values(additions); - // `filteredDefaultTypes` contains all types of `defaultTypes` - // that are not included in the `additions`. Not included - // means not having the same Amino type identifier. - const filteredDefaultTypes = Object.entries(defaultTypes).reduce( - (acc, [key, value]) => - additionalAminoTypes.find((addition) => sameAminoType(value, addition)) - ? acc /* Skip this defaultTypes entry */ - : { ...acc, [key]: value } /* Add this defaultTypes entry */, - {}, - ); - this.register = { ...filteredDefaultTypes, ...additions }; + this.register = { ...defaultTypes, ...additions }; } public toAmino({ typeUrl, value }: EncodeObject): AminoMsg { @@ -562,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), - }; } }