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

More int types #164

Merged
merged 2 commits into from
Oct 22, 2024
Merged

More int types #164

merged 2 commits into from
Oct 22, 2024

Conversation

virgil-serbanuta
Copy link
Member

@virgil-serbanuta virgil-serbanuta commented Oct 17, 2024

Fixes #127

@virgil-serbanuta virgil-serbanuta force-pushed the int-types branch 2 times, most recently from 0a0d542 to 4b7202a Compare October 18, 2024 09:31
Copy link
Collaborator

@ACassimiro ACassimiro left a comment

Choose a reason for hiding this comment

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

Just for clarification, this modification now allows us to use unsupported types in our contracts, right? And the types will be handled by K as normal integers, without any special handling such as representing them using structs or tuples.

It seems clear to me, and I have no problems with it for our demos.

Copy link
Member

@yanliu18 yanliu18 left a comment

Choose a reason for hiding this comment

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

I did some research and confirmed that u256 is not a valid type in Rust, unless you are using some external libraries.
And the documentation in MultiverseX contract uses biguint and bigint to represent large numbers exceeding u64.
The rust builtin implementation of biguint can be referenced here.

Although, we might want to take shortcuts for Devcon demo, we should still maintain rust contract legal. What do you think?

The rest of the implementation looks fine for me.

@virgil-serbanuta
Copy link
Member Author

Just for clarification, this modification now allows us to use unsupported types in our contracts, right? And the types will be handled by K as normal integers, without any special handling such as representing them using structs or tuples.

It seems clear to me, and I have no problems with it for our demos.

Yes. This is not a good solution in general, but it saves time for the demo.

@virgil-serbanuta
Copy link
Member Author

I did some research and confirmed that u256 is not a valid type in Rust, unless you are using some external libraries. And the documentation in MultiverseX contract uses biguint and bigint to represent large numbers exceeding u64. The rust builtin implementation of biguint can be referenced here.

I know. There are two issues here:

  1. ULM works with u256 while BigUint is either u800 or has unlimited size, depending on the context. I don't think we can just replace one with the other.
  2. Implementing BigUint takes time (or have a similar implementation for u256). It's true that we would mostly be copy-pasting things from the original MX semantics, but even that takes time.

Although, we might want to take shortcuts for Devcon demo, we should still maintain rust contract legal. What do you think?

I mostly agree. My original plan was to implement a U256 type as a struct in Rust (for which we would have to fake implementations for operators somehow, and we would need to make encoding and decoding work for structs; we would also have a choice between making impl work for structs or using static functions for most things we need - all of these are easy-ish, but they take time, and we only have 3 days left until the deadline (or 4, depends on how you count)), or as a BigUint-like implementation where we keep the ints in a K map and we access them through hooks, and we would still need to wrap the int ID in a struct so we would still have the same issues as for the full Rust struct implementation.

However, I think that some of the leaders (Ilia or Xiaohong or both) said that adding the u256 type as a fake native type is an acceptable solution, so, while I dislike it, that's what I ended up doing.

The rest of the implementation looks fine for me.

@yanliu18
Copy link
Member

yanliu18 commented Oct 21, 2024

@virgil-serbanuta I agree, we should work on more urgent task then. Thanks for the explanation.
Then I am OK to approve the changes.

@virgil-serbanuta virgil-serbanuta merged commit 91ae4d7 into main Oct 22, 2024
3 checks passed
@virgil-serbanuta virgil-serbanuta deleted the int-types branch October 22, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Int256
3 participants