Allow `TypedDataUtils` to be called unbound #152

Gudahtt · 2021-07-14T21:06:17Z

Previously the functions exposed as TypedDataUtils could only be called from the TypedDataUtils object. If they were called unbound, they would throw errors because of the use of this.

They have all been updated to no longer rely upon this, so they now work the same way regardless how they are bound when called.

They are still exported as the TypedDataUtils object, so this should not change the API. This was done to simplify the code, specifically to make it easier for functions outside of TypedDataUtils to reuse code inside of TypedDataUtils.

Some types required adjustments, as type mistakes were brought to light that TypeScript for some reason wasn't aware of when these were declared as properties of the TypedDataUtils object. These were fixed by adding two type assertions, making the types parameter to hashStruct and hashType more strict, and by making the typedData parameter to eip712Hash more strict. The type assertions (warranted or not) preserve the types used previously. We can replace them later with validation.

Gudahtt · 2021-07-14T21:13:15Z

src/index.ts

+function hashStruct(
+  primaryType: string,
+  data: Record<string, unknown>,
+  types: Record<string, MessageTypeProperty[]>,


Here is the parameter type adjustment that was required to eliminate type errors. Clearly this is required because it's passed straight to encodeData, which itself required the types parameter to be of this type.

Gudahtt · 2021-07-14T21:14:40Z

src/index.ts

+              field.type,
+              // TODO: Add validation to ensure this is a string-indexed
+              // object, so that this type cast can be removed
+              value as Record<string, unknown>,


Here's the first type assertion that was required. This should be guaranteed to be a Record<string, any> for any properly formatted message, but we don't have sufficient type information at this point to guarantee that. We can add that later.

Gudahtt · 2021-07-14T21:16:53Z

src/index.ts

-        sanitizedData.domain,
+      hashStruct(
+        // TODO: Validate that this is a string, so this type cast can be removed.
+        sanitizedData.primaryType as string,


Here's the second type assertion that was required. At first it looked like this was already guaranteed to be a string, because primaryType is a keyof the MessageTypes object, and that's string-indexed. But apparently TypeScript is screwy when it comes to index types, and it happily allows using a number index as well (I tested it to be sure), despite this attempt at preventing that. So technically this could be a number. But that's clearly against the spec, and it's clearly what we were trying to prevent with that type.

We can shore this up later with runtime validation. Or better types. Or both.

Gudahtt · 2021-07-14T21:17:21Z

test/index.ts

@@ -1389,3 +1389,122 @@ test('signedTypeMessage V4 with recursive types', (t) => {
    '0xf2ec61e636ff7bb3ac8bc2a4cc2c8b8f635dd1b2ec8094c963128b358e79c85c5ca6dd637ed7e80f0436fe8fce39c0e5f2082c9517fe677cc2917dcd6c84ba881c',
  );
 });
+
+test('unbound sign typed data utility functions', (t) => {


I tested that these all failed on the main branch.

will need to be converted to jest style to go along with this PR: #161

Gudahtt · 2021-07-14T21:47:21Z

I'd strongly recommend hiding whitespace when looking at this diff. It's a mess when whitespace is shown because everything was de-indented.

Gudahtt · 2021-07-14T21:48:52Z

src/index.ts

+  useV4 = true,
+): Buffer {
+  const encodedTypes = ['bytes32'];
+  const encodedValues: unknown[] = [hashType(primaryType, types)];


This was another type change that was required. TypeScript assumed that this was supposed to be a Buffer array because the first entry was a Buffer. But it looks like it's just an array of unknown data, not just of buffers.

Gudahtt · 2021-07-14T21:49:25Z

src/index.ts

+ */
+function hashType(
+  primaryType: string,
+  types: Record<string, MessageTypeProperty[]>,


This parameter type changed for the same reason as hashStruct

Gudahtt · 2021-07-14T21:49:54Z

src/index.ts

+ * @returns {Buffer} - keccak hash of the resulting signed message
+ */
+function eip712Hash<T extends MessageTypes>(
+  typedData: TypedData | TypedMessage<T>,


This was never actually passed in Partial<> data. Also sanitizeData requires that the typed data be complete.

Gudahtt · 2021-07-14T21:52:08Z

Just rebased this onto main and updated the commit message. No other changes made.

shanejonas · 2021-07-22T21:28:34Z

Havent touched this repo yet, but the changes to typings look good, +1 for removal of this.

Gudahtt · 2021-07-26T19:39:58Z

Thanks for the review @shanejonas! Just had to rebase this to resolve conflicts. The diff should be identical now except for the tests, which now need "V4" specified explicitly in a few places due to the consolidation PR that was merged.

Previously the functions exposed as `TypedDataUtils` could only be called from the `TypedDataUtils` object. If they were called unbound, they would throw errors because of the use of `this`. They have all been updated to no longer rely upon `this`, so they now work the same way regardless how they are bound when called. They are still exported as the `TypedDataUtils` object, so this should not change the API. This was done to simplify the code, specifically to make it easier for functions outside of `TypedDataUtils` to reuse code inside of `TypedDataUtils`. Some types required adjustments, as type mistakes were brought to light that TypeScript for some reason wasn't aware of when these were declared as properties of the `TypedDataUtils` object. These were fixed by adding two type assertions, making the `types` parameter to `hashStruct` and `hashType` more strict, and by making the `typedData` parameter to `eip712Hash` more strict. The type assertions (warranted or not) preserve the types used previously. We can replace them later with validation.

Gudahtt requested a review from a team as a code owner July 14, 2021 21:06

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch 2 times, most recently from 9ede8b9 to 910deeb Compare July 14, 2021 21:11

Gudahtt commented Jul 14, 2021

View reviewed changes

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch from 910deeb to 982c020 Compare July 14, 2021 21:51

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch from 982c020 to 3343934 Compare July 15, 2021 16:21

shanejonas previously approved these changes Jul 22, 2021

View reviewed changes

Gudahtt dismissed shanejonas’s stale review via c8ee6a8 July 26, 2021 19:32

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch 2 times, most recently from c8ee6a8 to d1b3605 Compare July 26, 2021 19:38

Gudahtt mentioned this pull request Jul 27, 2021

Add encode data tests #164

Merged

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch from d1b3605 to cf0663c Compare August 2, 2021 15:14

Gudahtt force-pushed the allow-sign-typed-data-utilties-to-be-called-unbound branch from 00a876d to 807aeae Compare August 2, 2021 15:18

shanejonas approved these changes Aug 3, 2021

View reviewed changes

Gudahtt merged commit 0c7e95a into main Aug 3, 2021

Gudahtt deleted the allow-sign-typed-data-utilties-to-be-called-unbound branch August 3, 2021 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `TypedDataUtils` to be called unbound #152

Allow `TypedDataUtils` to be called unbound #152

Gudahtt commented Jul 14, 2021 •

edited

Loading

Gudahtt Jul 14, 2021

Gudahtt Jul 14, 2021

Gudahtt Jul 14, 2021

Gudahtt Jul 14, 2021

shanejonas Jul 30, 2021

Gudahtt Aug 2, 2021

Gudahtt commented Jul 14, 2021

Gudahtt Jul 14, 2021

Gudahtt Jul 14, 2021

Gudahtt Jul 14, 2021

Gudahtt commented Jul 14, 2021

shanejonas commented Jul 22, 2021

Gudahtt commented Jul 26, 2021

Allow TypedDataUtils to be called unbound #152

Allow TypedDataUtils to be called unbound #152

Conversation

Gudahtt commented Jul 14, 2021 • 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

Gudahtt commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt commented Jul 14, 2021

shanejonas commented Jul 22, 2021

Gudahtt commented Jul 26, 2021

Allow `TypedDataUtils` to be called unbound #152

Allow `TypedDataUtils` to be called unbound #152

Gudahtt commented Jul 14, 2021 •

edited

Loading