stellar / js-xdr

Read/write XDR encoded data structures (RFC 4506)
Apache License 2.0
25 stars 31 forks source link

Add XDR support for 128/256-bit ints + BigInt helpers for numbers with arbitrary precision #96

Closed orbitlens closed 1 year ago

orbitlens commented 1 year ago

Added basic XDR types i128, u128, i256, u256 for operations with 128/256-bit integers. They can be used as basic types for ScVal wrappers.

Examples of encoding, slicing to parts, and casting to js BigInt:

// i256 from i32 parts
i256 = new I256([1, -2, 3, -4, 5, -6, 7, -8])
// same i256 from i64 parts represented as BigInts
i256 = new I256([-8589934591n, -17179869181n, -25769803771n, -34359738361n])
// same i256 from i128 parts represented as BigInts
i256 = new I256([-316912649983270374087927529471n, -633825299966540748184444993531n])
// convert BigInt value represented in HEX notation to i256
i256 = new I256(-0x7FFFFFFF800000005FFFFFFFA00000003FFFFFFFC00000001FFFFFFFFn)
// convert string value to i256
i256 = new I256('-215679573286988304483011684407414453311449214917006457140549519409151')

// get underlying BigInt value
i256.valueOf()  // returns -215679573286988304483011684407414453311449214917006457140549519409151n
// cast to string
i256.toString() // returns '-215679573286988304483011684407414453311449214917006457140549519409151'

//slice to integer parts of smaller size (32/64/128 bits each)
i256.slice(32)  // returns [1n, -2n, 3n, -4n, 5n, -6n, 7n, -8n]
i256.slice(64)  // returns [-8589934591n, -17179869181n, -25769803771n, -34359738361n]
i256.slice(128) // returns [-316912649983270374087927529471n, -633825299966540748184444993531n]

TODO: test coverage for u128, i128, u256

paulbellamy commented 1 year ago

This looks awesome. Thanks. My thinking is that this js-xdr library should only really deal with stuff core to the xdr protocol, whereas stellar's usage of xdr would be in the js-stellar-base repo. Since U256/I256/etc are a stellar-specific concept (soroban-specific really) that doesn't exist in the core xdr protocol, I'd say this code should live over there.

I'm happy to take this code and port it to that library, or you're welcome to and I'll approve the PR over there. Whichever you prefer, @orbitlens .

orbitlens commented 1 year ago

@paulbellamy My motivation to implement these new types in the js-xdr:

paulbellamy commented 1 year ago

In this pull request updated Hyper and UnsignedHyper to use the same BigInt conversion primitives. I really like the idea of "put any number of arguments of any type into the constructor and it will take care of it" approach.

I do like that as well.

  • If we split i64/u64 and i128/u128/i256/u256 into separate repositories, there will be no code reuse between them. As an alternative, we can leave source/bigint-encoder.js in the js-xdr repository, and export it. This way the stellar-base package could import the encoding primitives and build i128/u128/i256/u256 on top of it. In this case the codebase overlap will be minimal.

I'd be in favor your suggestion to leave the bigint-encoder here and importing it in js-stellar-base. I like your Hyper/UnsignedHyper refactor, and agree the code re-use would be great.

The fact that classic StellarCore doesn't use 128/256-bit integers right now doesn't mean that they won't be utilized in the future. For example, DEX offers could really benefit from i128 to overcome the existing price rounding errors when trading between assets with a huge difference in real-world price (e.g. even BTC/XLM pair trades sometimes results in significant rounding errors).

It's not about whether core uses these or not. This library tries to stick to only the xdr spec, and i128/256 are not in the xdr spec. Stellar-specific stuff belongs in js-stellar-base.

There's no harm in adding a few non-standard extension in the base serialization library. Webpack and other bundlers will be able to skip them entirely during tree-shake if those types are not used in the code. If we are talking about Stellar-related apps, the situation is even more simple. Since the XDR type definitions generated from .x files will always contain 128/256 bindings (at least for the sake of supporting ScVals), these types will be always present in the generated XDR type bindings. Therefore, there's no difference in which repository they will be defined.

I agree about the webpack/tree-shaking/bundle-size aspect. It is more about code organization into our existing framework.

I'm personally not a huge fan of the split between js-xdr and js-stellar-base, but it is what we have right now, so we should try to stick to it.

orbitlens commented 1 year ago

@paulbellamy As soon as source/bigint-encoder.js remains in the js-xdr package and other packages can reuse it, I have no strong objections against moving 128/256-bit integers to stellar-base. The only thing I'd like to stress out, I'd really like to have methods like .valueOf(), .toJson(), .toString() and .slice(), as well as universal constructor (the constructor that can build a large integer from string, BigInt, or several Int32 parts) in ScVal or some nested ScVal type.

In fact, exporting abstract LargeInt class from js-xdr might do the trick even better than BigInt helpers.

paulbellamy commented 1 year ago

Sounds like a good plan to me.

Shaptic commented 1 year ago

Agreed with the discussion here: src/{i,u}{128,256}.js probably belong in stellar-base alongside a higher-level construct to easily move between them all.

Another component of this work, unfortunately, is going to be modifying the relevant parts of stellar/dts-xdr to generate types that match the new method signatures for Hyper.

Shaptic commented 1 year ago

Closing as this was incorporated into #100