CashScript / cashscript

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

Allow bigints as args for inputs and outputs #128

Closed rkalis closed 1 year ago

rkalis commented 2 years ago

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.

emergent-reasons commented 2 years ago

LGTM

emergent-reasons commented 2 years ago

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;
rkalis commented 2 years ago

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.

emergent-reasons commented 2 years ago

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.)

rkalis commented 2 years ago

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.

emergent-reasons commented 2 years ago

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 commented 1 year ago

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.

mr-zwets commented 1 year ago

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.