From f45c5b50732f175fd4569a69cdc1b41eb17a0989 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 17 Feb 2022 13:50:06 +0100 Subject: [PATCH 1/3] Rewrite code to better explain AminoTypes constructor --- packages/stargate/src/aminotypes.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/stargate/src/aminotypes.ts b/packages/stargate/src/aminotypes.ts index cf7a5451..adca645a 100644 --- a/packages/stargate/src/aminotypes.ts +++ b/packages/stargate/src/aminotypes.ts @@ -519,6 +519,10 @@ 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. @@ -527,12 +531,16 @@ export class AminoTypes { private readonly register: Record; public constructor({ prefix, additions = {} }: AminoTypesOptions) { + const defaultTypes = createDefaultTypes(prefix); const additionalAminoTypes = Object.values(additions); - const filteredDefaultTypes = Object.entries(createDefaultTypes(prefix)).reduce( + // `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(({ aminoType }) => value.aminoType === aminoType) - ? acc - : { ...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 }; From f356ae45dd3fd57e2ebb9fa1a00a589a4b57d263 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 17 Feb 2022 17:08:36 +0100 Subject: [PATCH 2/3] Add AminoTypes constructor tests --- packages/stargate/src/aminotypes.spec.ts | 77 ++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/packages/stargate/src/aminotypes.spec.ts b/packages/stargate/src/aminotypes.spec.ts index 158f31e4..f6d1efb2 100644 --- a/packages/stargate/src/aminotypes.spec.ts +++ b/packages/stargate/src/aminotypes.spec.ts @@ -41,6 +41,83 @@ 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, + }, + }); + }); + + // This is a feature we have right now but we don't need + it("can override type by Amino type", () => { + 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)).toEqual({ + typeUrl: "/cosmos.staking.otherVersion456.MsgDelegate", + value: { + bar: 123, + }, + }); + }); + }); + describe("toAmino", () => { // bank From 10d101b5fcfbb77659f5be9add54107cd693db97 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 17 Feb 2022 17:28:04 +0100 Subject: [PATCH 3/3] 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 15b6551e..8a54b328 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 f6d1efb2..d2fd70e3 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 adca645a..efc8e1cd 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), - }; } }