hildjj / node-cbor

Encode and decode CBOR documents, with both easy mode, streaming mode, and SAX-style evented mode.
MIT License
357 stars 73 forks source link

decode BigNumber error #123

Closed GeekBerry closed 3 years ago

GeekBerry commented 3 years ago
const value = BigNumber(Math.PI).pow(3);
console.log(value.toFixed()); // 31.006276680299813114880451174049119330924860257

const buffer = cbor.encode(value);
console.log(buffer.toString('hex')); // c482382cc254056e5e99b1be81b6eefa3964490ac18c69399361

const result = cbor.decode(buffer);
console.log(result.toFixed()); // 0
GeekBerry commented 3 years ago

impletment like this might be ok, but still lose precision

    return exponent >= 0
      ? BigNumber(bigInt).times(BigNumber(10).pow(exponent))
      : BigNumber(bigInt).div(BigNumber(10).pow(-exponent));
hildjj commented 3 years ago

https://mikemcl.github.io/bignumber.js/#shift was the answer.

GeekBerry commented 3 years ago

it's no a answer, BigNumber could handle it, but your implement do not solve well. For example: https://github.com/GeekBerry/cbor/blob/8f6fd5efec1181c011327d49e0e954156807fb2f/test/encode.test.js#L19 encode/decode it right

hildjj commented 3 years ago

Shoot. I added a test and everything. Working, should be a quick fix.

hildjj commented 3 years ago

I'm not sure what's going on, unless you are using the cbor-cli package, which I forgot to update, but just did (make sure you've got version 5.0.3). This code works here:

const cbor = require('./')
const BigNumber = require('bignumber.js')

;(async () => {
  const pi3 = BigNumber(-Math.PI).pow(3)
  console.log(pi3.toFixed())
  // -31.006276680299813114880451174049119330924860257

  const buffer = cbor.encode(pi3)
  console.log(await cbor.comment(buffer))
  // c4                -- Tag #4
  //   82              -- Array, 2 items
  //     38            -- Negative number, next 1 byte
  //       2c          -- [0], -45
  //     c3            -- [1], Tag #3
  //       54          -- Bytes, length: 20
  //         056e5e99b1be81b6eefa3964490ac18c69399360 -- 056e5e99b1be81b6eefa3964490ac18c69399360

  const pi3decode = await cbor.decode(buffer)
  console.log(pi3decode.toFixed())
  // -31.006276680299813114880451174049119330924860257

  console.log(pi3.isEqualTo(pi3decode))
  // true
})().catch(console.error)
GeekBerry commented 3 years ago

i'm using cbor@^5.1.0 (latest 5.1.1) and bignumber.js@^9.0.0 (latest 9.0.1) under nodejs 14.3.0

hildjj commented 3 years ago

what happens with my code above in your environment?

hildjj commented 3 years ago

I just tried Node 14.3 and the code above still works for me.

GeekBerry commented 3 years ago

i upgrade to "cbor": "5.1.1", but decoded result is a pure object but not bignumber.js instance.

test code:

const value = BigNumber(-Math.PI).pow(3);
  console.log({
    valueIsBigNumber: BigNumber.isBigNumber(value),
    valueFixed: value.toFixed(),
  });

  const buffer = cbor.encode(value);
  console.log(buffer.toString('hex'));

  const result = cbor.decode(buffer);
  console.log({
    resultIsBigNumber: BigNumber.isBigNumber(result),
    result,
    // resultFixed: result.toFixed(), Error !!!
  });

output:

1.3333238555587148
{
  valueIsBigNumber: true,
  valueFixed: '-31.006276680299813114880451174049119330924860257'
}
a3617320616501616385181f1b0000009223ee121d1b00001c54656d97fc1b00002cac7c45793c1b0000175fbf5ee800
{
  resultIsBigNumber: false,
  result: {
    s: -1,
    e: 1,
    c: [
      31,
      627668029981,
      31148804511740,
      49119330924860,
      25700000000000
    ]
  }
}
hildjj commented 3 years ago

Ah! You're using a different instance of the bignumber.js library than node-cbor is, so instanceof isn't working. I can at least expose the version I'm using as cbor.BigNumber if you'd be willing to use that. Otherwise, this is going to need a bigger fix.

GeekBerry commented 3 years ago

what happens with my code above in your environment?

i am using Window OS, is there any thing difference?

hildjj commented 3 years ago

To prove my hypothesis, try this code:

'use strict'

const cbor = require('./')
const BigNumber = require('bignumber.js')

;(async () => {
  const pi3decode = await cbor.decode('c482382cc354056e5e99b1be81b6eefa3964490ac18c69399360')
  console.log(pi3decode.toFixed())
  const pi3 = new pi3decode.constructor(-Math.PI).pow(3)
  console.log(pi3.isEqualTo(pi3decode))
})().catch(console.error)
GeekBerry commented 3 years ago

em, above code worked under 5.1.1 but not 5.1.0. so 5.1.1 fixed this issue

GeekBerry commented 3 years ago

Ah! You're using a different instance of the bignumber.js library than node-cbor is, so instanceof isn't working. I can at least expose the version I'm using as cbor.BigNumber if you'd be willing to use that. Otherwise, this is going to need a bigger fix.

fixed by export BigNumber by cbor, or force use same bigNumber version lib?

hildjj commented 3 years ago

If you use the exact same version of BigNumber as me, I think the node module loader will cache it correctly. Here is a RunKit demonstrating the problem: https://runkit.com/hildjj/different-instances-of-bignumber

GeekBerry commented 3 years ago

If you use the exact same version of BigNumber as me, I think the node module loader will cache it correctly. Here is a RunKit demonstrating the problem: https://runkit.com/hildjj/different-instances-of-bignumber

i upgrade my bignumber.js to 9.0.1, but i think not the problem, cause returned decode value is a pure object, as you see, console format is different. result not start with BigNumber

1.3333238555587148
{
  valueIsBigNumber: true,
  value: BigNumber {
    s: -1,
    e: 1,
    c: [
      31,
      627668029981,
      31148804511740,
      49119330924860,
      25700000000000
    ]
  },
  valueFixed: '-31.006276680299813114880451174049119330924860257'
}
a3617320616501616385181f1b0000009223ee121d1b00001c54656d97fc1b00002cac7c45793c1b0000175fbf5ee800
{
  resultIsBigNumber: false,
  result: {
    s: -1,
    e: 1,
    c: [
      31,
      627668029981,
      31148804511740,
      49119330924860,
      25700000000000
    ]
  }
}
hildjj commented 3 years ago

Please try upgrading to cbor v5.1.2. Then, remove bignumber.js as a dependency of your project. Then, everywhere you do require('bignumber.js'), instead use cbor.BigNumber.

hildjj commented 3 years ago

And here is a RunKit showing it working correctly: https://runkit.com/hildjj/same-version-of-bignumber

hildjj commented 3 years ago

OK, I checked in a more-comprehensive fix, that should work with your old code. Please upgrade to 5.2.0 or higher.

hildjj commented 3 years ago

Proof: https://runkit.com/hildjj/two-versions-of-bignumber-working