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

toArray/toString: add tests #36

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

toArray/toString: add tests #36

wants to merge 8 commits into from

Conversation

dcousens
Copy link
Contributor

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 :).

"hex": "6c4313b03f2e7324d75e642f0ab81b734b724e13fec930f309e222470236d66b"
}
],
"invalid": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is for speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean, for speed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Like, does it just skip bad characters?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

dcousens commented Apr 8, 2015

ping @indutny

"valid": [
{
"dec": "0",
"hex": ""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wut? Should be 0, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

indutny commented Apr 8, 2015

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

@indutny
Copy link
Owner

indutny commented Apr 8, 2015

Otherwise LGTM, thank you! ;)

@dcousens
Copy link
Contributor Author

dcousens commented Apr 9, 2015

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.

},
{
"dec": "-1",
"hex": "ff"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dcousens dcousens Apr 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the twos complement, that is negative.
On 11 Apr 2015 04:28, "Fedor Indutny" notifications@github.com wrote:

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@dcousens dcousens Apr 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense for another issue/PR ;)

@indutny
Copy link
Owner

indutny commented Apr 10, 2015

@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
Copy link
Contributor Author

Sure, did you have a branch I could target?
On 11 Apr 2015 04:29, "Fedor Indutny" notifications@github.com 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
Copy link
Owner

indutny commented Apr 10, 2015

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

@axic
Copy link
Contributor

axic commented Feb 13, 2016

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

@jprichardson
Copy link
Collaborator

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

axic commented Feb 14, 2016

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 dcousens self-assigned this Jul 8, 2016
@dcousens
Copy link
Contributor Author

dcousens commented Jul 8, 2016

Added this back into my work queue 👍

edit 29/09/2016: soon...

edit 30/11/2017: 😞

@dcousens
Copy link
Contributor Author

re-adding to work queue attempt 3

@earonesty
Copy link

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 dcousens mentioned this pull request Sep 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants