search

starknet-io / starknet.js

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

Remove 31 char limit in eip-712 style offchain signing #710

Closed FawadHa1der closed 10 months ago

FawadHa1der commented 1 year ago

We are currently have a Sign in with Starknet technical proposal that we will convert into a SNIP. But in order to move forward we need to improve the usability and user expereince of offchain signing. Currently we cannot use more than 31 characters for any non felt field in the signed message and that degrades and limits the user experince considerably. We have a demo deployed, tested with Argentx Wallet. You will be able to see the limitation we are referring to. The 'domain.name' and 'message.statement' fields can eaily get more than 31 characters limit. This feature is very critical for a good offchain signing expereince. The limitation is mentioned in the doc as well.

We suggest to either remove the 31 character limit for strings and/or add a new type for strings of length greater than 31

tabaktoni commented 1 year ago

31 chars is Cairo felt limit, starknetjs limit it to avoid issues. I don't see a reason why not creating a specification that uses felt* (aka. long string) that we already support as so providing unlimited chars.

FawadHa1der commented 1 year ago

Thanks for the reply on this. The problem is that wallet wouldn't know that multiple felts are part of one sentence/message. Bad UX I think not to speak of security issues that might occur if users do not understand them. Also a lot of web domains or complete URI to a resource are longer than 31 characters limit. Showing them on multiple lines does not make sense. Let's say someone is trying to sign in from "https://community.starknet.io/demos/signinwithstarknet" Showing this as 2 different items of an array is bad UX imho. It should be one type otherwise the URI will show up as the screen shot shown below. The above reasoning also applies to a message/statement being displayed in the message.

Screenshot 2023-08-08 at 11 34 39 AM

FawadHa1der commented 1 year ago

Happy to work on this since this is important for the SNIP and is a blocker at the moment. Thanks in advance

JorikSchellekens commented 1 year ago

StarkWare is working on support for strings in Cairo. We could wait for this to land, and the wallets can support this standard. Specifying novel encodings for strings in this proposal is unwise and out of scope.

tabaktoni commented 10 months ago

Closing in favor of https://github.com/starknet-io/starknet.js/issues/816 And opening new one for string support