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

BigNumber encoding hex result doesn't match test cases #101

Closed listenaddress closed 4 years ago

listenaddress commented 4 years ago

I haven't been able to recreate this encoding test case. Is there something I'm doing wrong?

const cbor = require('cbor')
const { BigNumber } = require('bignumber.js')

// This doesn't match the test case here: https://github.com/hildjj/node-cbor/blob/466d01b25959ab1270a409e31453f53b5bba082d/test/cases.js#L695
const num = new BigNumber('9223372036854775807')
console.log(cbor.encode(num).toString('hex')) // c2487fffffffffffffff

// This matches the test case here: https://github.com/hildjj/node-cbor/blob/466d01b25959ab1270a409e31453f53b5bba082d/test/cases.js#L80
const num2 = new BigNumber('18446744073709551616')
console.log(cbor.encode(num2).toString('hex')) // c249010000000000000000

Here's a repl.it.

listenaddress commented 4 years ago

Ok, the test case that I'm encoding only gets decoded in the tests. And when I encode it, I don't get the associated hex string.

Screen Shot 2020-03-26 at 8 26 22 AM
hildjj commented 4 years ago

Whoa. I wonder if my encoding tests have been running at all. Great find, I'm checking into it.

hildjj commented 4 years ago

Ah, I think I've got it. 9223372036854775807 is in the decodeGood list of tests, which are tests that decode correctly, but they don't round-trip because the canonical way of encoding is different than a valid way of encoding that number. I think everything is fine, but want to make sure you agree before closing this.

listenaddress commented 4 years ago

Thanks for taking a look!

What are you referencing by canonical and valid?

I am trying to match some cbor encoding written in Go, which produces 1b7fffffffffffffff. Is that possible?

hildjj commented 4 years ago

I guess you're asking for BigNumber integers and ECMAscript bigints to be encoded as normal integers, if they would fit. I'm open to adding an option for that, but since it's a breaking change as well as breaking round-tripping, I don't want to make it the default.

hildjj commented 4 years ago

Fixed in version 5.0.2. Use the collapseBigIntegers option.

console.log(cbor.encodeOne(0x7fffffffffffffffn, {
  collapseBigIntegers: true
}).toString('hex'))

generates: 1b7fffffffffffffff

listenaddress commented 4 years ago

Awesome, thanks a lot Joe!