cosmos / cosmjs

The Swiss Army knife to power JavaScript based client solutions ranging from Web apps/explorers over browser extensions to server-side clients like faucets/scrapers.
https://cosmos.github.io/cosmjs/
Apache License 2.0
655 stars 344 forks source link

Introduction of omitDefault in Cosmjs/Amino 0.31.2 breaks signing with undefined memo field #1555

Open baoskee opened 9 months ago

baoskee commented 9 months ago

In @cosmjs/stargate src/modules/ibc/aminomessages.ts IBC transfer amino messages in 0.31.1 did not include memo field.

Screenshot 2024-02-11 at 2 27 59 PM

IBC transfer amino messages in 0.32.2 does include memo field.

Screenshot 2024-02-11 at 2 29 01 PM

This breaks messages with no memo field in value as omitDefault(undefined) throws an error.

webmaster128 commented 9 months ago

But memo is of type string, not string | undefined. So it feels like the MsgTransfer instance you have there is broken. How do you create it?

omniwired commented 9 months ago

@baoskee you are so right, and you fixed my problem! If no memo is set it fails you are right!

@webmaster128 some examples on the libs where the problem happens:

 "dependencies": {
    "@cosmjs/amino": "^0.32.2",
    "@cosmjs/proto-signing": "0.32.2",
    "@cosmjs/cosmwasm-stargate": "^0.32.2",
    "@cosmjs/ledger-amino": "^0.32.2",
    "@cosmjs/stargate": "^0.32.2",
    "@ledgerhq/devices": "^6.20.0",
    "@ledgerhq/hw-app-cosmos": "^6.29.4",
    "@ledgerhq/hw-transport-node-hid": "^6.28.4",
    "@ledgerhq/hw-transport-webusb": "^6.28.4",
  },
  "peerDependencies": {
    "@ledgerhq/hw-transport": "^6",
    "@ledgerhq/hw-transport-webhid": "^6",
    "@ledgerhq/hw-transport-webusb": "^6"
  }

Example of the MSG:

    const msgTransfer: MsgTransferEncodeObject = {
      typeUrl: "/ibc.applications.transfer.v1.MsgTransfer",
      value: {
        sender: address,
        receiver: osmoAddress,
        sourcePort: "transfer",
        sourceChannel: "channel-0",
        token: {
          denom: "utori",
          amount: amount + "",
        },
        // timeoutTimestamp: Long.fromNumber(11111111),
        timeoutHeight: {
          revisionNumber: BigInt(Long.fromNumber(1).toString()),
          revisionHeight: BigInt(
            Long.fromNumber(timeoutHeightosmoCilent).toString(),
          ),
        },
        timeoutTimestamp: BigInt(
          Long.fromNumber(Date.now() + 600_000)
            .multiply(1_000_000)
            .toString(),
        ),
        memo: "ibc transfer", -> HERE
      },
    };

IF MEMO IS NOT SET, it won't work as @baoskee explained.

webmaster128 commented 9 months ago

I think the type

export interface MsgTransferEncodeObject extends EncodeObject {
  readonly typeUrl: "/ibc.applications.transfer.v1.MsgTransfer";
  readonly value: Partial<MsgTransfer>; // HERE
}

is buggy. The memo field in MsgTransfer is non-toptional but Partial<MsgTransfer> makes fields optional. When you create an instance, always use MsgTransfer.fromPartial({ ... }). Then the memo is an empty string and everything is fine.