search

starknet-io / starknet.js

JavaScript library for StarkNet
https://www.starknetjs.com
MIT License
1.21k stars 706 forks source link

feat: CairoFelt252 class #1146

Open b0rza opened 1 month ago

b0rza commented 1 month ago

Draft PR info!

This is a draft PR to check the validity of the approach. Will update the description with proper info after confirmation, when the PR is ready.

Solves the Felt part of https://github.com/starknet-io/starknet.js/issues/1116

Motivation and Resolution

...

RPC version (if applicable)

...

Usage related changes

Development related changes

Checklist:

b0rza commented 1 month ago

@ivpavici please take a look if this is what you had in mind when the Cairo data types issue was created. I understand that this class differs from what was done for CairoUint256 and CairoUint512, but this one feels like a utility class leveraged by those types and others.

Please confirm if that's correct, or something else needs to be done.

tabaktoni commented 1 month ago

@ivpavici please take a look if this is what you had in mind when the Cairo data types issue was created. I understand that this class differs from what was done for CairoUint256 and CairoUint512, but this one feels like a utility class leveraged by those types and others.

Please confirm if that's correct, or something else needs to be done.

Yep, it seems good. WP! 🎸 Please add also in https://github.com/starknet-io/starknet.js/tree/next-version/src/utils/calldata on es. request/response parsers, We would leave felt() helper for external users but internally would switch all to new Class Felt.

penovicp commented 1 month ago

@tabaktoni @ivpavici @PhilippeR26 There are some good points about potential behaviour changes mentioned in the test comments that we should probably decide on before the next major release such as hex validation and value bound checks. Another that might also make sense is number handling, currently positive and negative numbers are treated differently: 1 is stored as '1', '1' is stored as '1', -1 is stored as '-1', and '-1' is stored as '11569'.

b0rza commented 1 month ago

@penovicp the tests are pretty much a verbatim copy of the existing ones for felt(). So I guess at least some of those comments can be directed to the original author, but I'm more than willing to make changes.

I wanted to make sure they are passing with this new class approach.

I will obviously tweak and do some more passes over everything now that I know this is the direction that we want to take.

PhilippeR26 commented 1 month ago

@tabaktoni @ivpavici @PhilippeR26 There are some good points about potential behaviour changes mentioned in the test comments that we should probably decide on before the next major release such as hex validation and value bound checks. Another that might also make sense is number handling, currently positive and negative numbers are treated differently: 1 is stored as '1', '1' is stored as '1', -1 is stored as '-1', and '-1' is stored as '11569'.

-1 ? Is now Starknet.js handling negative numbers? In validate.ts, only u are handled. i are not accepted. So why test negative cases?

penovicp commented 1 month ago

-1 ? Is now Starknet.js handling negative numbers? In validate.ts, only u are handled. i are not accepted. So why test negative cases?

The linked issue #1116 lists the signed integer types as planned for future support after CairoFelt252 is done.

PhilippeR26 commented 1 month ago

So, does it mean that currently only TypedData will use negative numbers, and all the rest of snjs is not use them (validate, requestParser, responseParser) ? Right?

Also, I don't understand why to accept negative input on felt252. By definition, a felt252 do not accept negative numbers ; only i8,i16, i32, i64 and i128 are authorized in Cairo to handle negative numbers.

penovicp commented 1 month ago

For negative numbers the class could throw a validation error, or they could be serialised as PRIME - value, or something else.

I'm currently not advocating for a specific approach, just wanted to point out some of the class behaviour we might want to change now that it's being reworked. For a different example: how should the class behave for numbers with values larger than PRIME.

tabaktoni commented 1 month ago

So, does it mean that currently only TypedData will use negative numbers, and all the rest of snjs is not use them (validate, requestParser, responseParser) ? Right?

Also, I don't understand why to accept negative input on felt252. By definition, a felt252 do not accept negative numbers ; only i8,i16, i32, i64 and i128 are authorized in Cairo to handle negative numbers.

Felt support storing negative as P-value. https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/serialization_of_Cairo_types/ But: The felt252 type is a fundamental type that serves as the basis for creating all types in the core library. However, it is highly recommended that programmers to use the integer types instead of the felt252 type whenever possible, as the integer types come with added security features that provide extra protection against potential vulnerabilities in the code, such as overflow and underflow checks. By using these integer types, programmers can ensure that their programs are more secure and less susceptible to attacks or other security threats.

So we can throw a warning on negative with the recommendation of using iN

PhilippeR26 commented 1 month ago

I remember when felt where renamed felt252 in Cairo 1. Abdel asked to the community how to rename felt. One of the popular proposal was u252...