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

Padding fix #199

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

Padding fix #199

wants to merge 2 commits into from

Conversation

macor161
Copy link

Fixes #198
toString padding should only apply if length of number is smaller than requested padding.

@fanatid
Copy link
Collaborator

fanatid commented Sep 14, 2018

I think that this will make things more complex

  • you did not do same for hex base
  • out.length % padding !== 0 still here and still not clear for what this construction is used

@macor161
Copy link
Author

Hey @fanatid thanks for the quick response!

Hmm after inspecting the code a little bit more, it seems that this is indeed the intended use of the padding param.

If you look at the tests at line 16, a hex number is created with a length of 7:

var a = new BN('ffb9602', 16);

The length of hex numbers must always be even so they use a padding of 2 to fix it:

assert.equal(a.toString('hex', 2).length, 8);

So this doesn't seem to be a bug but it is still rather unusual, especially for decimal numbers.

Then again, I think that having two different behaviors for hex numbers and decimal numbers would probably add a lot of confusion for the user. And this would add a breaking change to the library.

So I suggest having a new function explicitly made for "normal" padding. toStringWithPadding or something like that. What do you think?

@dcousens
Copy link
Contributor

I really should get cracking on #36

@therightstuff
Copy link

I'm confused, isn't padding only applied on display? If so, why wouldn't it be applied differently for different representations? If I ask for padding when displaying a decimal, I'm going to expect something different from when I'm asking for padding for a hexadecimal.

@macor161
Copy link
Author

@therightstuff I totally agree with you that the display should behave differently depending on the base of the number. However, I think that the padding should be applied uniformly.

If I call toString() with a padding of 6, I think it should add zeros to the left of that number until the length is 6, whatever the base of the number is.

But right now, toString() uses a modulo padding, which adds zeros until the length is a multiple of the padding. Hence new BN('123').toString(10, 2) === '0123'.

It is totally fine and not a bug, but a little bit unexpected as a user perspective.

So this is why I propose a new function:

  • To prevent any breaking changes for users who are using the current padding.
  • Because the modulo padding is also used internally.

What do you think if I close this pull request and we continue the conversation here?

@dcousens
Copy link
Contributor

Padding is typically dealt with at a byte level, the issue of toString doing any padding at all is confusing.

@therightstuff
Copy link

@dcousens what do you mean by padding? I think it's fair to say that the majority of developers using something like bn.js will understand padding as being for representation, and when we talk about padding we talk about adding leading zeroes or blanks to fill a minimum space. If there's another possible meaning then I would say that that needs to be made clear with different method or parameter names.

If toString() allows a padding parameter that is non-standard (regardless of what's "correct" or "incorrect"), then it should have a different name (not "padding", perhaps "bytePadding" or something) or be documented clearly. If we're just talking about different padding behaviour or different mechanisms between different bases then that should be transparent to the developer calling toString.

@therightstuff
Copy link

@macor161 I agree with not introducing breaking changes, perhaps using a new signature would help?

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 this pull request may close these issues.

4 participants