Encoding for "shortstring" typed data (SNIP-12)

[penovicp]

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.

Added context: This issue is a copy of the issue noted in this PR comment.

[xJonathanLEI]

Hi, I'm trying to implement SNIP-12 in starknet-rs. Given the lack of test vectors from the spec I decided to use starknet.js for generating test data, since it's pretty much the de facto impl in the wild. This is when I noticed a discrepancy between the spec and the starknet.js impl, which is exactly what this issue is about.

Based on the current impl:

starknet.js/src/utils/typedData.ts

Lines 424 to 429 in 8c65fa8

	case 'felt':
	case 'shortstring': {
	// TODO: should 'shortstring' diverge into directly using encodeShortString()?
	if (revision === Revision.ACTIVE) {
	assertRange(getHex(data as string), type, RANGE_FELT);
	} // else fall through to default

a type of shortstring gets properly encoded as shortstring iif it's a non-numerical string. This violates the spec and can be highly problematic especially in the context of a contract where it can be very expensive to tell if a certain string is numerical. I would definitely call this a bug.

However, fixing this bug has some pretty devastating implications.

First of all, the bug conveniently accommodates an exception outlined in the spec:

Revision 1: Will be the initial version of the specification. Note that for this revision the value in this field should be the integer 1 and not the shortstring "1" despite being defined as shortstring. This exception is made to support an inconsistency in the Braavos wallet implementation.

with this bug the revision field correctly uses 0x1 for revision. Naively fixing this bug would cause this field to use 0x31 instead, which would violate the exception noted from the quote above.

This is a minor issue though. Some refactoring can be done to actually implement the exception specifically for that domain field.

The bigger issue is just the fact that fixing this bug itself is a breaking change. Many applications might already depend on the current behaviour. Making matters worse, this bug affects the domain hash, which is usually hard-coded in contracts for verification purposes. Upgrading them would either need to be a huge coordinated effort or simply impossible.

So I guess whichever direction we take, at least one of these need to be done:

1. Obviously just fix this and ask apps/contracts to upgrade if they're affected (they most probably are). Here I'm assuming that wallets & apps use starknet.js for SNIP-12.
2. Update SNIP-12 to officially specify that shortstring only uses short string encoding if the string in question is not ^\d+$ (good luck implementing this in a contract tho).