Improve UX of TypedData usage #1198

PhilippeR26 · 2024-08-08T13:24:54Z

Motivation and Resolution

Solves #529
Several users have reported difficulties to use signature and verification of TypedData (SNIP-12, EIP712), especially with related Account methods.

Account.verifyMessage() & Account.verifyMessageHash() are shadowed.
A Provider.verifyMessageInStarknet() method has been added.
An ec.getFullPublikey() helper has been added.
A typedData.verifyMessage() function has been added.

Usage related changes

Important

No breaking changes in this PR.

As Account.verifyMessage() & Account.verifyMessageHash() needs to instantiate a fake Account with a fake private key, these functions are shadowed (tagged as @ deprecated) and are replaced by Provider.verifyMessageInStarknet().
It clarify also that this verification is working by interacting with Starknet.

const isValid4 = await myProvider.verifyMessageInStarknet(myTypedData, signature2, account0.address);
const isValid5 = await myProvider.verifyMessageInStarknet(msgHash, signature2, account0.address);

the complicated code to get the full public key

const fullPubKey = encode.addHexPrefix(encode.buf2hex(ec.starkCurve.getPublicKey(privateKey, false)));

can now also be made with

const fullPublicKey = ec.getFullPublicKey(privateKey);

It was problematic to verify a message outside of Starknet, with usage of low level functions and problematic types ( Uint8Array, ArraySignature not supported, ...).
We have now a dedicated function, with friendly types (BigNumberish, Signature, ...) :

const isValid = typedData.verifyMessage(myTypedData, signature, fullPublicKey, account0Address);
const isValid2 = typedData.verifyMessage(msgHash, signature, fullPublicKey);

Development related changes

Interfaces: verifyMessage() & verifyMessageHash() have been removed from the Account interface.
verifyMessageInStarknet()has not been added in RpcProvider class, to not impact users implementations.
code of Account.verifyMessage() & Account.verifyMessageHash() has been refactored in Provider.verifyMessageInStarknet().
Some utils functions have been added : num.isBigNumberish(), typedData.verifyMessage() & ec.getFullPublikey()
tests : tests related to new functions have been added.
docs : All JSDoc and guides for Account.verifyMessage() & Account.verifyMessageHash() have been removed.
All new functions includes JSDoc, and have been explained in the Signature guide.

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

ivpavici · 2024-08-20T13:05:59Z

__tests__/rpcProvider.test.ts

+
+describeIfDevnet('EIP712 verification', () => {
+  // currently only in Devnet-rs, because can fail in Sepolia.
+  // to test in all cases once PR#989 implemented.


This comment has been addressed through #1128

So, we can either remove it or add more tests

If the problem is solved, then it can be made in all networks.
Modified.

I didn't verify about devnet vs testnet :D I just saw that PR#989 was merged

Well, my PR#989 has not been merged : Tony closed it, implementing its own PR#1128 on this subject.
So, I suppose that he made the necessary tests to validate that it now works everywhere in all networks.

www/docs/guides/signature.md

penovicp

Made a few suggestions for some minor changes, the test filter and isBigNumberish edge case being most notable.

In general lgtm.

penovicp · 2024-08-22T07:12:40Z

src/utils/typedData.ts

+export function verifyMessage(
+  message: BigNumberish | TypedData,
+  signature: Signature,
+  fullPublicKey: BigNumberish,
+  accountAddress?: BigNumberish
+): boolean {


Suggested change

export function verifyMessage(

message: BigNumberish | TypedData,

signature: Signature,

fullPublicKey: BigNumberish,

accountAddress?: BigNumberish

): boolean {

export function verifyMessage(

message: TypedData,

signature: Signature,

fullPublicKey: BigNumberish,

accountAddress: BigNumberish

): boolean;

export function verifyMessage(

message: BigNumberish,

signature: Signature,

fullPublicKey: BigNumberish

): boolean;

export function verifyMessage(

message: BigNumberish | TypedData,

signature: Signature,

fullPublicKey: BigNumberish,

accountAddress?: BigNumberish

): boolean {

These overrides should make the parameter relationship explicit within TS.

Implemented.

@penovicp
Once modified, the code is not compiling :

src/utils/typedData.ts:632:17 632 export function verifyMessage( ~~~~~~~~~~~~~ The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible.

What's this???

Don't ask me why, but this is working :

export function verifyMessage( message: TypedData, signature: Signature, fullPublicKey: BigNumberish, accountAddress: BigNumberish ): boolean; export function verifyMessage( message: BigNumberish, signature: Signature, fullPublicKey: BigNumberish ): boolean; export function verifyMessage( message: BigNumberish | TypedData, signature: Signature, fullPublicKey: BigNumberish, accountAddress?: BigNumberish ): boolean; export function verifyMessage( message: BigNumberish | TypedData, signature: Signature, fullPublicKey: BigNumberish, accountAddress?: BigNumberish ): boolean {

The error is probably from one of the failure tests where the parameters are deliberately wrong. I'll double check soon and add a tweak if I'm right. Either way I'll approve the PR.

Edit: Yes, the error was from a failure test. Essentially it was complaining that the call will work but that it was using a mix of two overrides, this would be a potential issue in actual code but the test was wrong deliberately. When you added the new override the test call aligned with it so it stopped complaining. I added an any cast in the relevant test instead.

penovicp · 2024-08-22T08:32:15Z

src/utils/num.ts

+  return (
+    isNumber(input) ||
+    isBigInt(input) ||
+    (typeof input === 'string' && (isHex(input) || /^[0-9]*$/i.test(input)))


Suggested change

(typeof input === 'string' && (isHex(input) || /^[0-9]*$/i.test(input)))

(typeof input === 'string' && (isHex(input) || isStringWholeNumber(input)))

Without the change isBigNumberish('') would return true.

Implemented.

penovicp · 2024-08-22T08:49:46Z

__tests__/utils/ec.test.ts

@@ -0,0 +1,10 @@
+import { ec } from '../../src';
+
+describe.only('ec full public key', () => {


Suggested change

describe.only('ec full public key', () => {

describe('ec full public key', () => {

Moved in stark test

penovicp · 2024-08-22T08:50:01Z

__tests__/utils/num.test.ts

@@ -216,3 +216,14 @@ describe('stringToSha256ToArrayBuff4', () => {
    expect(buff).toEqual(new Uint8Array([43, 206, 231, 219]));
  });
 });
+
+describe.only('isBigNumberish', () => {


Suggested change

describe.only('isBigNumberish', () => {

describe('isBigNumberish', () => {

implemented

penovicp · 2024-08-22T09:03:57Z

src/utils/ec.ts

+ * // result = "0x0400b730bd22358612b5a67f8ad52ce80f9e8e893639ade263537e6ef35852e5d3057795f6b090f7c6985ee143f798608a53b3659222c06693c630857a10a92acf"
+ * ```
+ */
+export function getFullPublicKey(privateKey: BigNumberish): string {


I would prefer to avoid expanding utils/ec.ts to keep its identity as a submodule alias. I would suggest moving this method to utils/stark.ts.

penovicp · 2024-08-22T09:16:51Z

src/utils/ec.ts

+ * ```
+ */
+export function getFullPublicKey(privateKey: BigNumberish): string {
+  const privKey = addHexPrefix(BigInt(privateKey).toString(16));


Suggested change

const privKey = addHexPrefix(BigInt(privateKey).toString(16));

const privKey = toHex(privateKey);

Not possible.
Dependency cycle

Once moved in stark, I was able to change to toHex

# [6.13.0](v6.12.1...v6.13.0) (2024-08-27) ### Bug Fixes * repair enum type lookup for typed data hashing ([36f8c3c](36f8c3c)) * sync cryptographic dependencies ([da20310](da20310)) ### Features * implement SNIP-9 outside execution functionality ([#1208](#1208)) ([e3c80c5](e3c80c5)), closes [#1111](#1111) [#1202](#1202) * improve message verification utilities ([#1198](#1198)) ([bdad9a5](bdad9a5))

github-actions · 2024-08-27T06:09:15Z

🎉 This issue has been resolved in version 6.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

chore: improve UX of TypedData usage

548bae4

PhilippeR26 requested a review from ivpavici August 8, 2024 13:26

ivpavici requested review from tabaktoni and penovicp August 13, 2024 08:22

ivpavici reviewed Aug 20, 2024

View reviewed changes

fix: implement requests of Ivan

0d0d2c7

penovicp reviewed Aug 22, 2024

View reviewed changes

fix: implement Petar requests

7dde016

PhilippeR26 requested a review from penovicp August 22, 2024 11:04

refactor: tweak verifyMessage overrides

6c74d81

penovicp approved these changes Aug 22, 2024

View reviewed changes

penovicp merged commit bdad9a5 into starknet-io:develop Aug 26, 2024
3 checks passed

github-actions bot added the released on @next label Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UX of TypedData usage #1198

Improve UX of TypedData usage #1198

PhilippeR26 commented Aug 8, 2024

ivpavici Aug 20, 2024

PhilippeR26 Aug 20, 2024 •

edited

Loading

ivpavici Aug 20, 2024

PhilippeR26 Aug 20, 2024

penovicp left a comment

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024

PhilippeR26 Aug 22, 2024

PhilippeR26 Aug 22, 2024

penovicp Aug 22, 2024 •

edited

Loading

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024

penovicp Aug 22, 2024

PhilippeR26 Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024

	(typeof input === 'string' && (isHex(input) \|\| /^[0-9]*$/i.test(input)))
	(typeof input === 'string' && (isHex(input) \|\| isStringWholeNumber(input)))

		@@ -0,0 +1,10 @@
		import { ec } from '../../src';

		describe.only('ec full public key', () => {

	describe.only('ec full public key', () => {
	describe('ec full public key', () => {

	describe.only('isBigNumberish', () => {
	describe('isBigNumberish', () => {

	const privKey = addHexPrefix(BigInt(privateKey).toString(16));
	const privKey = toHex(privateKey);

Improve UX of TypedData usage #1198

Improve UX of TypedData usage #1198

Conversation

PhilippeR26 commented Aug 8, 2024

Motivation and Resolution

Usage related changes

Development related changes

Checklist:

Choose a reason for hiding this comment

PhilippeR26 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penovicp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penovicp Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeR26 Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Aug 27, 2024

PhilippeR26 Aug 20, 2024 •

edited

Loading

penovicp Aug 22, 2024 •

edited

Loading

PhilippeR26 Aug 22, 2024 •

edited

Loading