Skip to content

Conversation

penovicp
Copy link
Collaborator

Motivation and Resolution

Resolves #1002 (i128 typed data encoding). Also adds typed data range checks for primitive types and a "StarkNet" casing update for typed data types.

Usage related changes

The encoding changes should not affect users that use valid data, if the data is outside the permitted range it will generate errors with the code from this PR. If users are using some of the updated types they will need to update them to their new name.

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

case 'i128':
case 'ContractAddress':
case 'shortstring': {
// TODO: should 'shortstring' diverge into directly using encodeShortString()?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to draw attention to the approach taken with the shortstring encoding. It was implemented by reusing the existing felt encoding.

As a basic example, a provided value "hello" will be encoded as 0x68656c6c6f, this I believe to not be contentious. The part where I believe there might be issues with is the encoding of numerical strings, the existing felt encoding treats both numbers and numerical strings as the same value and such behaviour might be undesirable/unexpected for the shortstring type. As an example "2" will be encoded as 0x2 while someone coming from a Cairo background will probably assume it to be encoded as 0x32.

Copy link
Member

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

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

IGWT!

@tabaktoni tabaktoni changed the base branch from develop to next-version March 25, 2024 13:26
@penovicp penovicp merged commit 87147d5 into next-version Mar 25, 2024
@penovicp penovicp deleted the fix/typed-i128 branch March 25, 2024 13:32
@tabaktoni tabaktoni mentioned this pull request Mar 25, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

encodeType of typed data does not encode preset types correctly

2 participants