Adds tests cases for issue #5702

[peetzweg]

See #5702 for further details.

[jacogr]

Always appreciate this. Obviously it cannot be merged but useful for somebody that wishes to look into the logged issue - if CI fails the PR is not merge-able, but somebody can certainly copy this/parts of it into their effort.

One comment:

If anything fails you need to delve through an it statement to find the place - so each it should have 1 expectation (maybe more, if they are very closely related and follow-ups of each other and easy to find)

In some cases it is possible, in others it would be impossible to find which one creates issues, for instance

// Zero
    expect(
      new Int(registry, new Uint8Array([0]), 8).toNumber()
    ).toEqual(0);

    expect(
      new Int(registry, new Uint8Array([0, 0]), 16).toNumber()
    ).toEqual(0);

TL;DR Rather use a describe block with specific it + (one) expect for each.

EDIT: If there are u8 -> number issues, the underlying decoding problems would be in https://github.com/polkadot-js/common/blob/master/packages/util/src/u8a/toNumber.ts & https://github.com/polkadot-js/common/blob/master/packages/util/src/u8a/toBn.ts (which also has relevant tests, worth expanding)

[peetzweg]

Thanks @jacogr for pointing out where these errors might root from. Tried looking into it a bit already, will continue from there.

In some cases it is possible, in others it would be impossible to find which one creates issues, for instance

Yes, agree it's not as transparent which one failed, however the stacktrace reveals which one it actually failed i.e.

  at TestContext.<anonymous> (/Users/xxx/git/pjs-api/packages/types-codec/src/base/Int.spec.ts:113:7)

Copy pasting almost identical its is quite cumbersome. Unfortunately we can't create parameterized tests with @polkadot/dev-test right?

There also seems to be an issue with decoding just the value 0 to a Number, which results in

i8: Input too large. Found input with 9 bits, expected 8

https://github.com/peetzweg/api/blob/f33bbc8340d66f46b77da041bfc1467b3d33c450/packages/types-codec/src/base/Int.spec.ts#L111-L114

Which is a quite odd. 🤔

[peetzweg]

I think I have found the cause for these inconsistencies, haven't been able resolve the problem yet. I have to hunches which look odd to me.

isNegative flag is always true

As we are using the Int class, this uses the toNumber() function, which expects a ToNumberOptions isNegative, this is always set to true here, as the AbstractInt class passes the isSigned flag along, which is always true. Which is probably not correct as the value needed should indicate if the number is negative or not.

The conversion in toNumber.ts is not fully transparent to me, only that a different branch is used for isNegative. When adding tests I can make some tests fail for value 0 if this value is set to true, with isNegative=false they work. Same for other values.
From what I read about two's complement, the sign of a the given number in bits needs to be inferred by the least significant bit, however in SCALES LE encoding this would be the "first" bit (left to right).

Interpretation for i8 and i16 is different to i32

From glancing over the different conversion from different byte amounts, the working version for 32bit/4bytes looks oddly different than the conversion i8/i16 in question.

Excerpt, only case 1,2 and 4 are relevant. i24 and other are not even support by pjs and vanilla rust. Could be broken as well but would have never bubbled up.

case 1: // 1 Byte => i8 ❌
        return (((value[0] ^ 0x0000_00ff) * -1) - 1);

case 2: // 2 Bytes => i16 ❌
  return ((((value[0] + (value[1] << 8)) ^ 0x0000_ffff) * -1) - 1);

case 3: // 1 Byte => i24 ❔
  return ((((value[0] + (value[1] << 8) + (value[2] << 16)) ^ 0x00ff_ffff) * -1) - 1);

case 4: // 4 Byte => i32 ✅
  // for the 3rd byte, we don't << 24 - since JS converts all bitwise operators to
  // 32-bit, in the case where the top-most bit is set this yields a negative value
  return ((((value[0] + (value[1] << 8) + (value[2] << 16) + (value[3] * 0x1_00_00_00)) ^ 0xffff_ffff) * -1) - 1);

https://github.com/polkadot-js/common/blob/adca13bc8c9c6c24ddb02c788b9c869bd050f4a8/packages/util/src/u8a/toNumber.ts#L23-L35

Maybe a fix for this is now obvious for someone. I have some tests locally and tried but out of focus for today. Could probably pick this up next week again.

[jacogr]

Could you merge in the master branch (it contains the latest common stuff). Hopefully it means the test cases here would start passing…

[peetzweg]

@jacogr done! They succeed locally now. Great job! 🙌

Stale review.

[jacogr]

Thank you.

[polkadot-js-bot]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.