Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: type conversion, opInc, opDec for bigint and bigdec type #676

Closed
wants to merge 14 commits into from

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Mar 30, 2023

Description

  1. suport type conversion for bigint and bigdec.
  2. Basic OpInc(++), OpDec(--) for bigint and bigdec

can be converted from bigint to

  • bigdec
  • string

can be converted to bigint from

  • int
  • int32
  • int64
  • uint
  • uint32
  • uint64

can be converted from bigdec to

  • string

can be convrted to bigdec from

  • int
  • int32
  • int64
  • uint32
  • uint64
  • bigint
  • string

How has this been tested?

included testcase.gno

Additional

related pr & issue: #306 #650

to support big number => bigint
to support arbitrary precision => bigdec
for better compatibility => type conversion

bigdec <-> float(64) has not been implemented due to determinism

@mconcat
Copy link

mconcat commented Mar 30, 2023

Awesome! This will improve big integer ergonomics for contract side

@r3v4s r3v4s changed the title type conversion for bigint and bigdec feat: type conversion, opInc, opDec for bigint and bigdec type Apr 7, 2023
@r3v4s r3v4s marked this pull request as ready for review April 7, 2023 01:56
@r3v4s r3v4s requested a review from a team as a code owner April 7, 2023 01:56
@r3v4s
Copy link
Contributor Author

r3v4s commented Apr 7, 2023

@moul @thehowl

Bigint Bigdec Primitive Type is ready, I think :D

@moul
Copy link
Member

moul commented Apr 7, 2023

In my opinion, it's imperative to integrate math.Big into Gno at some stage to enable the smooth transfer of Go libraries that rely on it, all while upholding Go's signature legibility.

I strongly support the addition of new simple types, such as bigint and bigdec, to distinguish Gno from Go. This approach does not hinder porting from Go to Gno, and since my intention is not to port Gno to Go, it offers a clear and straightforward syntax. Gno is designed for blockchain applications, and I anticipate that bigint will be a more frequent data type in many contracts than int.

The question that arises is whether we should create bigint as an alias or as a distinct implementation in Gno.


Can you add the unit tests you wrote in the PR body to the PR?


Edit: Assuming our confidence in converting later bigint to math.Big in both directions, I propose that we proceed with this PR, as it offers a streamlined and performant approach for utilizing important data types.
We can then follow up with a separate PR that introduces math.Big with complete Go-compatibility.


// should always panic
// XXX how to catch go panic in gno ??
// shouldPanic(t, func() {bigdec("abc")} )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this, but can we catch go's panic in gno?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it comes from a machine.Panic instead of a panic inside go code, yes, however need to think whether it makes sense here...

@thehowl
Copy link
Member

thehowl commented Apr 14, 2023

I think bignum makes sense as a native type, and in general agree with Manfred's argument. While I think this is probably the first instance in which Gno deviates from Go at a language level, it is for good reason in the context of application. I have primarily two concerns:

  • Bignums have by default no memory-size constraint - while in general this is not a problem in Gno (as any extra memory used, the user has to pay with their gas -- though we do need to make the allocator aware of this and adjust the allocated memory on how much size the bignum's actually taking up), I do wonder if maybe from a programmer's point of view this is not obvious enough. It should be clear that bignums do not come cheap, that giving "arbitrary" access to them to an end user is equivalent to allowing them to keep adding characters to a []byte. I don't necessarily have a solution here; I'm just wondering if this is something you or manfred thought and/or have opinions or counterarguments about.
  • Performance - bignums obviously perform worse than integers - some extra context about existing benchmarks, especially comparing the behaviours against gmp (which I don't recommend using). I'd like you to consider doing a few benchmarks to see how much bignums would perform on common operations compared to u/int64s, and perhaps also to see if a hybrid approach whereby numbers in the int64 range as stored internally as integers (until an operation pushes them beyond the limits) would perform better.

@jaekwon
Copy link
Contributor

jaekwon commented Apr 18, 2023

We should definitely discuss this in depth before pulling the trigger on it.
(Meanwhile it's also possible to merge much of this in as long as we don't yet expose bigint/bigdec as keywords).

While BigInt I see as being more straightforward and beneficial, also acknowledging the variable size blowup issue; I think BigDec is much worse. The way BigDec works across different libraries (I chose cockroach's impl after comparing many libraries), there is no universal spec for it, especially the way precision is handled across multiplication/division. I don't understand BigDec's size properties very well and I don't need to thus far, because BigInt and BigDec are only used to compute constant values in a limited way. That is, when we declare constant expressions in the form of decimals or floating point constant expressions, we don't manipulate the value (say) in a for-loop. But once bigdec becomes available in Gno code as a type, this will start happening. And it will probably cause much confusion.

IMO supporting BigInt is more straightforward. But we would want the Gno->Go precompiler to do the right thing to ensure that type-safety is ensured. I guess we can have a shim.BigInt type for that.

The main question is are we breaking away from Go now officially? If we could transpile Gno code to Go code that uses big.Int, (and surely we can do this), I don't see why not. Maybe people might even end up using this for Go.

@r3v4s
Copy link
Contributor Author

r3v4s commented Apr 18, 2023

@jaekwon thx for your opinion.

I think we don't have much problem making bigint as native type(I'll commit some work for better transpile). One thing that @thehowl and I concerning was about default memory size, which we can hard limit the size value of 512 bits or somehow make gas-fee related(proportional) to bigint size.

OTOH, for bigdec now I see there will be some problems making it as native type. So how about we make it as none keyword type like (var std.dec) ??

even if, bigdec is going to be none keyword type, we can still doesn't have to implement native methods and having it as native will have better performance

@thehowl
Copy link
Member

thehowl commented Apr 18, 2023

If we're putting a hard limit on 512 bits, we should not call it "bignum" but a "uint512".

If we put a limit we can also use an ad-hoc library such as uint256 (could be adapted to 512 bits without too much trouble I'd think, though 10^77 possible values are probably enough for most use cases), which has much better cpu/memory benchmarks than using big.Int.

I didn't comment on this in my last comment, though I do agree that we should probably leave bigdec out. To me there's not really a compelling case to having it as a builtin type, as to me any quantity that is discrete (ie. money and tokens) should be in integers and any which should try to get close to the mathematical Real/Rational numbers should be a float, with very very few exceptions.

@piux2
Copy link
Contributor

piux2 commented Apr 19, 2023

The maximum memory size for GVM execution is determined in the allocator. This brings up another question: how do we set max memory size. I opened a new issue for the discussion #761.

@piux2
Copy link
Contributor

piux2 commented Apr 19, 2023

In Solidity, uint256 and int256 utilize a maximum memory allocation of 32 Bytes in the Ethereum Virtual Machine (EVM), which is comparable to an unrestricted bigint in GNO when the maximum allocation value is set.

However, GNO's memory allocation tracking requires knowledge of the type size in order to monitor allocations. To emulate Go's bigint behavior, we need to develop a Bigint structure type in gno and port the big.Int implementation from Go to GNO, allowing the GNO allocator to be aware of the number of Words (unit types) used to store the Bigint.

Otherwise, as in current implementation, we would need to assign a fixed size for bigint in the allocator, verify this size, and trigger a panic if an overflow occurs. In the current implementation, the GNO bigint type value is set at 200 bytes in the GNO allocator.

@piux2
Copy link
Contributor

piux2 commented Apr 19, 2023

In Go, big.dec is an internal data type within the big package, serving solely as a decimal representation for big.Float for display purposes and not being exposed for arithmetic calculations to users.

In Solidity, Decimal is not a type but rather a literal (value format) of the fixed-point number type, as Solidity uses fixed-point numbers instead of floating-point numbers. There are no floating-point numbers in Solidity. Users can create custom decimal struct types and arithmetic calculation functions to wrap int256 operations and simulate decimal number arithmetic.

CockroachDB is an RDBMS that adheres to SQL specifications and supports both fixed-point types (decimal type) and floating-point numbers (float type). In SQL, Decimal is a distinct number type rather than a representation of floating-point numbers.

In GNO, we implement bigdec for fixed-point numbers to achieve deterministic big floating-point numbers. This seems to be the original intention (adopting the fixed-point number concept from CockroachDB).
So far, It works fine.

Moving forward, it might not be necessary to connect Go's big dec to GNO's bigdec, as they represent different point number types. Go's big.dec is for floating-point numbers, while Gno's bigdec is for fixed-point numbers since it is borrowed from CockroachDB

A potential optimization involves creating the Bigdec structure in GNO, allowing the allocator to utilize the internal []Word size in order to calculate the Bigdec size, therefore leveraging the full memory of GNO for a dynamically-sized big number. Presently, the allocator has a fixed bigdec size of 200 bytes.

For the time being, if we think adjusting the fixed type size and verifying overflow suffices, then this approach would be a relatively simple and satisfactory solution.

@jaekwon
Copy link
Contributor

jaekwon commented Apr 20, 2023

I think we don't have much problem making bigint as native type(I'll commit some work for better transpile). One thing that @thehowl and I concerning was about default memory size, which we can hard limit the size value of 512 bits or somehow make gas-fee related(proportional) to bigint size.

We can't limit bigint size without breaking Go spec for constant expressions so we can't do that. Which means we will have to account for memory allocations.

@r3v4s
Copy link
Contributor Author

r3v4s commented Apr 20, 2023

Since this pr includes both bigint and bigdec, I thought it would be better if we separated PR and have further discussion. Starting with bigint #764

@r3v4s r3v4s closed this Apr 20, 2023
@moul
Copy link
Member

moul commented Apr 28, 2023

How about supporting just (u)int{128,256} for now and reserving bigint for Gnolang-1.1+?

If gno precompile can generate valid Go code for these new simple types, then we can consider adding bigint or other types later with more ease.

@peter7891
Copy link
Contributor

How about supporting just (u)int{128,256} for now and reserving bigint for Gnolang-1.1+?

If gno precompile can generate valid Go code for these new simple types, then we can consider adding bigint or other types later with more ease.

(u)int{128,256} is supported both in Rust and Zig.
I gravitate towards this more.

@moul moul requested a review from mvertes June 22, 2023 09:10
@r3v4s r3v4s deleted the temp/bigint-bigdec branch April 3, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants