nativeToScVal did not handle numbers correctly.

[overcat]

Describe the bug

The following code should output false, but we got true, 2 ** 127 - 1 was not converted correctly.

const sorobanClient = require("soroban-client")

const a = sorobanClient.nativeToScVal(-(2 ** 127), {
    type: "i128",
}).toXDR("base64")

const b = sorobanClient.nativeToScVal(2 ** 127 - 1, {
    type: "i128",
}).toXDR("base64")

console.log(a === b)

What version are you on? Check yarn.lock or package-lock.json to find out precisely what version of stellar-base you're running.

soroban-client: 0.11.0
stellar-base 10.0.0-soroban.7

To Reproduce Check the code above.

Expected behavior Check the code above.

Additional context i256, u128, and u256 also have this issue. I speculate that it is due to the incorrect handling of the sign bit?

[willemneal]

This is a JS issue. You went outside the bounds of the number type in JS, 2**53 - 1 So you need bigint, e.g. 2n ** 127 - 1n. However, I do think that it's a bug that the method doesn't throw if you are trying to use a number instead of a bigint.

[Shaptic]

Allowing number is intentional, but @willemneal is right that this is because JavaScript can't represent values that large using its Number type. You should either use Strings or BigInts for these. Perhaps you're right - maybe we shouldn't allow number?

[willemneal]

Yeah for ints larger than u/i32, I think it should only allow bigints as it will prevent issues like this and draw attention to the issue. The bindings currently type them as bigints, but also doesn't ensure that the JS uses bigint arguments.

[overcat]

You should either use Strings or BigInts for these.

String is not supported, right?

const b = sorobanClient.nativeToScVal("170141183460469231731687303715884105727", {
    type: "i128",
}).toXDR("base64")

          throw new TypeError("invalid type (".concat(opts.type, ") specified for string value"));
          ^

TypeError: invalid type (i128) specified for string value

Furthermore, personally I tend not to support the use of number here as it may introduce implicit bugs.

[Shaptic]

Ah sure, that's right. If you know you have an integer, you should use ScInt rather than nativeToScVal which has more flexibility. But we can add support for that in this converter, too! I'll look into this.

[Shaptic]

Closed by #690!