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

parseCoins has to work with Big Int arithmetics. Uint64 is not enough #1268

Closed
AKorpusenko opened this issue Sep 14, 2022 · 5 comments · Fixed by #1270
Closed

parseCoins has to work with Big Int arithmetics. Uint64 is not enough #1268

AKorpusenko opened this issue Sep 14, 2022 · 5 comments · Fixed by #1270
Milestone

Comments

@AKorpusenko
Copy link

parseCoins can work only with uint64 type which is too small for tokens with decimals like 12 or 18
https://github.com/cosmos/cosmjs/blob/main/packages/proto-signing/src/coins.ts#L20
has to be changed to work with BigInt and serialized to a string type after

@webmaster128
Copy link
Member

Actually the only thing this line does is a format check on the string and strip leading zeros. So we can even implement it without using BigInt, which is still an issue for some environments.

@AKorpusenko
Copy link
Author

AKorpusenko commented Sep 14, 2022

@webmaster128, thanks for reply
But wtf you already use the big number arithmetics for Uint64 😄.
Here there is a case of usage of bn.js. Moreover you do use the BigInt here

The easiest case is just to use either the BigInt, or bn.js if you want so (or this is a backward compatibility issue). Like this:

/* eslint-disable no-bitwise */
import BN from "bn.js";
import { Coin } from "@cosmjs/amino";
/**
 * Takes a coins list like "819966000ucosm,700000000ustake" and parses it.
 *
 * This is a Stargate ready version of parseCoins from @cosmjs/amino.
 * It supports more denoms.
 */
export function parseCoins(input: string): Coin[] {
  return input
    .replace(/\s/g, "")
    .split(",")
    .filter(Boolean)
    .map((part) => {
      // Denom regex from Stargate (https://github.com/cosmos/cosmos-sdk/blob/v0.42.7/types/coin.go#L599-L601)
      const match = part.match(/^([0-9]+)([a-zA-Z][a-zA-Z0-9/]{2,127})$/);
      if (!match) throw new Error("Got an invalid coin string");
      return {
        amount: new BN((match[1]), 10, "be").toString(),
        denom: match[2],
      };
    });
}

@AKorpusenko AKorpusenko changed the title parseCoins have to work with BigInt. Uint64 is not enough parseCoins has to work with Big Int arithmetics. Uint64 is not enough Sep 14, 2022
@webmaster128
Copy link
Member

webmaster128 commented Sep 14, 2022

The point I am trying to make is that parseCoins does not do any arithmetics. It splits and checks a string and converts a string number to a string number after this has been checked to be [0-9]+. So we can get rid of the UintXYZ limitation entirely.

We use bn.js for historic reasons and I'd like to avoid creating more usages of it. I'd like to use native BigInt but the React Native folks make me wonder if we can use it in critical places.

Anyways, I'll fix this one way or another.

@webmaster128 webmaster128 added this to the 0.29.0 milestone Sep 14, 2022
@AKorpusenko
Copy link
Author

Thanks for reply

@webmaster128
Copy link
Member

Shipped as part of CosmJS 0.29.0

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

Successfully merging a pull request may close this issue.

2 participants