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: CairoFelt252 class #1146

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

b0rza
Copy link
Contributor

@b0rza b0rza commented Jun 5, 2024

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 #1116

Motivation and Resolution

...

RPC version (if applicable)

...

Usage related changes

  • Change 1.
  • ...

Development related changes

  • Change 1.
  • ...

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@b0rza
Copy link
Contributor Author

b0rza commented Jun 5, 2024

@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
Copy link
Collaborator

tabaktoni commented Jun 6, 2024

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

Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

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

General approach looks good to me.

Tests could be tweaked a bit. Noticed that some are almost equivalents, e.g. non-ascii check and exceeding max safe integer. Values from constants.ts could be used for some tests, such as using PRIME where uint256 is mentioned.

@penovicp
Copy link
Collaborator

penovicp commented Jun 6, 2024

@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
Copy link
Contributor Author

b0rza commented Jun 6, 2024

@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
Copy link
Collaborator

@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
Copy link
Collaborator

penovicp commented Jun 7, 2024

-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
Copy link
Collaborator

PhilippeR26 commented Jun 7, 2024

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
Copy link
Collaborator

penovicp commented Jun 7, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

import { BigNumberish, isBigInt, isBoolean, isHex, isStringWholeNumber } from '../num';
import { encodeShortString, isLongText, isShortText, isString } from '../shortString';

type ParsableTypes = BigNumberish | Boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be like this? instead of boolean constructor

Suggested change
type ParsableTypes = BigNumberish | Boolean;
type ParsableTypes = BigNumberish | boolean;

@b0rza b0rza force-pushed the feature/introduce-felt252-class branch from fbce638 to e7595d5 Compare July 8, 2024 20:10
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.

5 participants