Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
baoskee opened this issue Feb 11, 2024 · 3 comments

Comments

@baoskee
Copy link

baoskee commented Feb 11, 2024

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
Copy link
Member

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
Copy link

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants