Padding fix

[macor161]

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

[fanatid]

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]

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]

I really should get cracking on #36

[therightstuff]

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]

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

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

[therightstuff]

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

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