CashScript / cashscript

⚖️ Easily write and interact with Bitcoin Cash smart contracts
https://cashscript.org
MIT License
112 stars 79 forks source link

Disallow JS Numbers, and use bigint everywhere to avoid boundary cases for number inputs #140

Closed mr-zwets closed 1 year ago

mr-zwets commented 1 year ago

Currently CashScript uses Number as type for the contract constructor arguments & in spending functions argument. The number format is only accurate up to 53bit but the BCH VM allows for 63 bit numbers.

This becomes a more pressing issue with CashTokens because the maximum token amount is a 63 bit number so needs to use bigint instead of number as type. The issue is already relevant even without CashTokens for contracts doing multiplication on satoshi amounts, such as AnyHedge.

The MR #128 "Allow bigints as args for inputs and outputs" does not address this. The two issues listed in that MR for full conversion were

1) There needs to be a breaking release for CashTokens anyway 2) if we leave the in and output amounts as Numbers, the fee can stay the same

So a question then is whether to make a breaking change to encodeInt/decodeInt from the @cashscript/utils

Or to simply introduce encodeBigInt/decodeBigInt and only use it where necessary.

emergent-reasons commented 1 year ago

To reiterate the critical point, encode/decode and all other entry points for data into a contract cannot be allowed to introduce numerical errors.

So bigint doesn't have to be enforced, but numerical precision must be enforced. In other words, passing in an "integer" that is outside the safe range must be rejected at a minimum.

rkalis commented 1 year ago

bigint can already be used in contract parameters and function parameters, this was introduced in the bigint update last May. It's also supported for encodeInt. The only place where it hasn't been added is in decodeInt, but that function isn't significantly used in recent CashScript SDK any more, so it doesn't have to get updated.

The issue that @emergent-reasons explained to me earlier is that they would like to be able to use bigint when proivding inputs through .from() and when providing outputs through .to(), which is addressed by #128. I forgot to merge that PR earlier, but I also still fail to grasp why it matters much for inputs/outputs (not taking CashTokens into account).

Now that we have to take CashTokens into account, I definitely see the need for supporting bigint in inputs and outputs, and as you mentioned @mr-zwets CashTokens will be a breaking update any way, so we have more flexibility.

I think the cleanest option here would be to replace every occurrence of number with bigint (i.e. disallow the use of number altogether - only exception might be fee satoshis per byte, since that needs to be a float).

We should probably still close #128 since that attempts to do it in a non-breaking way. I can take on converting the current version of the library to bigint, then @mr-zwets, perhaps you can work on top of that for CashTokens.

emergent-reasons commented 1 year ago

All sounds good. Just two points:

  1. Fine to close #128. Sorry for the original request which wasn't thought through well enough.
  2. Reiterating - from a practical perspective, any combination of number and/or bigint is fine as long as unsafe numbers are strictly blocked/errors. That's in all parts of the interface, including encode/decode. The worst case then is just "can't use big numbers" instead of the much worse "might make invalid contracts or draw invalid data out of contracts".
rkalis commented 1 year ago

After @mr-zwets explained it to me, I realise that I just completely misunderstood your initial request last year @emergent-reasons, which is why I didn't understand the need for it. Now I finally understand, so let's get that fixed in the CashTokens release. Sorry about that! 🙈

emergent-reasons commented 1 year ago

Ok thanks! The original request was a mess. We literally just thought to do bigint exclusively everywhere which might be a pure approach but probably not a practical one.

rkalis commented 1 year ago

This is released in the latest cashscript@next release. It will also be included in the full 0.8.0 release once the May 2023 upgrade is live.