cashscript icon indicating copy to clipboard operation
cashscript copied to clipboard

Allow bigints as args for inputs and outputs

Open rkalis opened this issue 3 years ago • 6 comments

Initially the request was to use bigint everywhere. But there's a few issues:

  1. It's hard to do this in a non-breaking way, and I don't know if it's worth it to do a breaking release for something like this.
  2. Fee calculation is a delicate part of the code that would need to be fully refactored to work with bigints, which I doubt would be worth the benefit.

So I ended up with only a very small update that allows users to provide inputs and outputs with bigints rather than numbers.

rkalis avatar Aug 28 '22 08:08 rkalis

LGTM

emergent-reasons avatar Aug 28 '22 12:08 emergent-reasons

These appear in the installed file for data.d.ts. When we use this to encode or decode large values, I think that will cause false state right?

export declare function encodeInt(int: number | bigint): Uint8Array;
export declare function decodeInt(encodedInt: Uint8Array, maxLength?: number): number;

emergent-reasons avatar Aug 31 '22 02:08 emergent-reasons

Good catch @emergent-reasons! I guess with that in mind we may need to actually update all interfaces to use only bigint and then just bump a new breaking version, even though the breaking changes are quite small.

rkalis avatar Sep 07 '22 15:09 rkalis

After looking at everything, since sats are by definition within safe range, and assuming competent coding by the user... the only thing I see that has real potential to give false results is the encode/decode. Everything else could be left as numbers if it doesn't have other reason to be bigint. What do you think?

(Sorry for going in almost a circle.)

emergent-reasons avatar Sep 07 '22 15:09 emergent-reasons

Right, I was thinking that if I'm breaking decodeInt I might as well break everything, but I just realised that decodeInt is a function from the @cashscript/utils package, for which the docs specifically mention that it doesn't actually follow semver as it's mostly an internal API. Are you using encodeInt/decodeInt for things in AnyHedge.

rkalis avatar Sep 07 '22 15:09 rkalis

Yes here.

Also wouldn't be mad at all about a full bigint upgrade that forces us and everyone to be more careful. That's a big move though.

emergent-reasons avatar Sep 07 '22 15:09 emergent-reasons

Somebody asked me to clarify about the issue with bigints. Just to make sure it's all in one place:

BCH inputs/outputs can be left as numbers because they cannot be bigger than the 53 bit integer limit for JS. Probably safer as bigint but as long as there is strict integer testing (there should be a hard error on any decimal places), it's fine.

The minimum problem is that encode/decode need to work for script integers in general, not only satoshi amounts. In that case, js ints can't handle the full 63/64 bit range. That means that currently encode/decode are significant error/attack vectors for the library that could cause people to lose money.

emergent-reasons avatar Jan 04 '23 06:01 emergent-reasons

I think this MR was somewhat confused so i created a new issue #140 " Allow bigints as contract constructor arguments & in spending functions arguments"

This MR misses the mark because it doesn't add bigint in the necessary places and instead adds them where they are unnecessary and in a way that adds no value...

I suggest we close this MR and decide on the exact scope of the change in the new issue.

mr-zwets avatar Jan 04 '23 06:01 mr-zwets