toArray/toString: add tests #36

dcousens · 2015-02-12T04:27:59Z

This PR adds tests for toArray and toString to enforce consistency.
The tests as of right now are failing because I am not aware of the intended output format of toArray vs toString.

toString() seems to output decimal. Tests pass.
toString(16) seems to output unsigned hex string with a - sign appended to the front.
toArray() seems to output unsigned binary, but not in twos complement (the standard)

@indutny I'm happy to amend these tests if you could shed some light on what is expected :).

dcousens · 2015-02-12T04:29:36Z

test/fixtures/bn.json

+ "hex": "6c4313b03f2e7324d75e642f0ab81b734b724e13fec930f309e222470236d66b"
+ }
+ ],
+ "invalid": [


So far, none of these are throwing. Is that intentional?

Yeah, this is for speed.

What do you mean, for speed?

I mean that checking and throwing takes time, and I want it to be as fast as possible.

Ok, what would the expected behaviour be then? Just so I can put it under these tests :)

Like, does it just skip bad characters?

It is worse than that :) You'll get some garbage. Predictable, but still a garbage.

Why not make the default safe, and for the internal usage a factory method BN.__createFast which doesn't do the checks?
Garbage sounds like a real easy way to have people encounter issues without knowing it. Especially given this library is most often used for crypto.
I think it is likely people will just accept the garbage result as what it should be at face value, because the underlying implementation they have no understanding of.

@dcousens I don't even use .toString()/parse internally, actually. I wonder if the regexp in assert before entering the _parseHex and _parseBase would be enough? Though, it might be a bit more complicated for _parseBase, since the character set may different between different bases.

I think it would most certainly be a start.
I'd have to look at the implementations for other ways to maintain performance and increase safety.

dcousens · 2015-02-12T04:31:44Z

The returned hex from toString also seems to be incomplete, in that hex.length % 2 === 0 doesn't always hold true. In fact, 0x01 will be returned as 0x1. Is it intentional that the hex string is not grouped by bytes?

dcousens · 2015-04-08T01:46:01Z

ping @indutny

indutny · 2015-04-08T08:04:32Z

test/fixtures/bn.json

+ "valid": [
+ {
+ "dec": "0",
+ "hex": ""


wut? Should be 0, I think.

Should it be? That would imply a default padding of 1?

I'd like to choose the path of least surprise. bn.js is used not only for crypto purposes, but for UI input/output too.

Fixed the test @indutny, after some further research and thinking about it. I'm happy with a consistent padding of at least 1.
Conceptually I could understand an empty string for zero, as the absence of any information, but, in this case it wasn't consistent with the decimal result so whatever.

indutny · 2015-04-08T08:07:33Z

I don't think that we need invalid section, at least not now.

indutny · 2015-04-08T08:07:40Z

Otherwise LGTM, thank you! ;)

dcousens · 2015-04-09T03:07:29Z

Rebased.

Ok @indutny I addressed most of the nits.

Waiting on your answer about whether it should have a the default zero padding of 1 (which IMHO I think is odd).

Also curious if after the discussion about regex's whether I keep/remove the invalid section.

indutny · 2015-04-10T18:28:40Z

test/fixtures/bn.json

+ },
+ {
+ "dec": "-1",
+ "hex": "ff"


Why should this happen? Hex could be negative too, right?

It is the twos complement, that is negative.
On 11 Apr 2015 04:28, "Fedor Indutny" [email protected] wrote:

Mmm...? I don't think I support twos complement anywhere in code. Also, it doesn't seem to be a desired feature to me.

I guess it depends how you want this to be used. You block out the ease of use as a data interchange format if you use '-' over a sign bit (twos complement). Otherwise I have to either encode it myself, or just send the string itself. Using twos complement is the standard for this, as that is it's entire purpose. Also, you already would be encoding twos complement for use in elliptic? (DER encoding uses twos complement integers)

@dcousens HEX is nothing special to .toString(), the base-36 or base-3 encoding should work the same way as the HEX does.

I guess the two's complement thing should happen in the code responsible for encoding this. There are various way to encode it, and this doesn't look like a reason to make HEX something special.

Agreed, maybe it's worth having it as separate functionality? In any
case, I'll amend the test vectors.

Sure, makes sense for another issue/PR ;)

indutny · 2015-04-10T18:29:43Z

@dcousens looking very good now, except positive-only hex and a failing tests... Speaking of this, I'm not a big fan of having failing tests in repo's master. Would you mind separating them into an another PR, where we'll figure out the best way to make it work properly?

dcousens · 2015-04-10T21:39:33Z

Sure, did you have a branch I could target?
On 11 Apr 2015 04:29, "Fedor Indutny" [email protected] wrote:

@dcousens https://github.com/dcousens looking very good now, except
positive-only hex and a failing tests... Speaking of this, I'm not a big
fan of having failing tests in repo's master. Would you mind separating
them into an another PR, where we'll figure out the best way to make it
work properly?

—
Reply to this email directly or view it on GitHub
#36 (comment).

indutny · 2015-04-10T21:50:50Z

@dcousens oh right! Added you as a collaborator to this repo, I hope you don't mind :)

axic · 2016-02-13T14:48:12Z

@dcousens do you think it would make sense revisiting this PR?

jprichardson · 2016-02-14T03:41:09Z

@axic I sure hope so.

Garbage sounds like a real easy way to have people encounter issues without knowing it. Especially given this library is most often used for crypto.
I think it is likely people will just accept the garbage result as what it should be at face value, because the underlying implementation they have no understanding of.

I couldn't have said it any better @dcousens!

axic · 2016-02-14T17:42:04Z

Some of the things you've mentioned could be addressed/discussed as part of #129:

toString(16) seems to output unsigned hex string with a - sign appended to the front.
toArray() seems to output unsigned binary, but not in twos complement (the standard)

dcousens · 2016-07-08T01:44:59Z

Added this back into my work queue 👍

edit 29/09/2016: soon...

edit 30/11/2017: 😞

dcousens · 2017-11-29T22:43:02Z

re-adding to work queue attempt 3

earonesty · 2018-08-08T18:52:15Z

Note: right now the toString() function doesn't zero pad hex (or anything > base 10), which results in strings like "a" instead of "0a" being output as valid hex strings. Perhaps this is causing tests to fail. If people are relying on the current behavior, might need to make "padding" an option.

dcousens reviewed Feb 12, 2015
View reviewed changes

indutny reviewed Apr 8, 2015
View reviewed changes

dcousens added 7 commits April 9, 2015 12:56

tests: remove unused fixtures variables and move fixtures to fixtures/

11d82d0

tests: remove unused variables

d6d252c

tests: add tests/fixtures for toArray/toString

8b13595

rename dhGroups to dh-groups

7d9198b

utils-test: adhere to 80char limit

1b56590

utils-test: test initialization with hex

6e065cb

utils-test: more 80char limit adherence

dc79116

fixtures: dec of 0 should be hex of 00

59a2122

indutny reviewed Apr 10, 2015
View reviewed changes

dcousens self-assigned this Jul 8, 2016

dcousens added the feature / refactor label Nov 29, 2017

dcousens added the semver-major label Aug 9, 2018

dcousens mentioned this pull request Sep 15, 2018

Padding fix #199

Open

DONALDSULLENDER approved these changes Apr 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toArray/toString: add tests #36

toArray/toString: add tests #36

dcousens commented Feb 12, 2015

dcousens Feb 12, 2015

indutny Apr 8, 2015

dcousens Apr 8, 2015

indutny Apr 8, 2015

dcousens Apr 8, 2015

indutny Apr 8, 2015

dcousens Apr 8, 2015

indutny Apr 8, 2015

dcousens Apr 8, 2015

dcousens commented Feb 12, 2015

dcousens commented Apr 8, 2015

indutny Apr 8, 2015

dcousens Apr 8, 2015

indutny Apr 9, 2015

dcousens Apr 9, 2015

indutny commented Apr 8, 2015

indutny commented Apr 8, 2015

dcousens commented Apr 9, 2015

indutny Apr 10, 2015

dcousens Apr 10, 2015 •

edited

indutny Apr 10, 2015

dcousens Apr 10, 2015 via email

indutny Apr 10, 2015

dcousens Apr 10, 2015 •

edited

indutny Apr 10, 2015

indutny commented Apr 10, 2015

dcousens commented Apr 10, 2015

indutny commented Apr 10, 2015

axic commented Feb 13, 2016

jprichardson commented Feb 14, 2016

axic commented Feb 14, 2016

dcousens commented Jul 8, 2016 •

edited

dcousens commented Nov 29, 2017

earonesty commented Aug 8, 2018

toArray/toString: add tests #36

Are you sure you want to change the base?

toArray/toString: add tests #36

Conversation

dcousens commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens commented Feb 12, 2015

dcousens commented Apr 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 8, 2015

indutny commented Apr 8, 2015

dcousens commented Apr 9, 2015

Choose a reason for hiding this comment

dcousens Apr 10, 2015 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens Apr 10, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens Apr 10, 2015 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 10, 2015

dcousens commented Apr 10, 2015

indutny commented Apr 10, 2015

axic commented Feb 13, 2016

jprichardson commented Feb 14, 2016

axic commented Feb 14, 2016

dcousens commented Jul 8, 2016 • edited

dcousens commented Nov 29, 2017

earonesty commented Aug 8, 2018

dcousens Apr 10, 2015 •

edited

dcousens Apr 10, 2015 •

edited

dcousens commented Jul 8, 2016 •

edited