Migrate from JavaScript to TypeScript #74

KiChjang · 2020-01-08T11:30:31Z

Fixes #59. Fixes #70.

As I feared initially, there are some difficulties when trying to type some parameters and values correctly. msgParams.data in particular is quite problematic -- I've come to understand it as a union between an object and a string according to usage, but the function body does not seem to indicate that it properly handles the cases where it is a string type, and instead assumes that it's always an object.

I'm creating a draft pull request now just to have more eyes looking at what I've managed to convert. Currently it cannot be compiled by the TypeScript compiler due to the aforementioned problems I pointed out above.

KiChjang · 2020-01-08T11:39:20Z

I just found out from #68 that there used to be a typings declaration file for this project, but was removed in that PR. I think I'd be able to get some ideas from that file.

KiChjang · 2020-01-08T23:42:00Z

@danfinlay I'm pinging you now because the msgParams typing is completely out of whack. According to the function bodies of recoverTypedSignature{,Legacy,_v4}, we should expect that msgParams.data to be of the string type. However, in the test files, recoverTypedSignatureLegacy is called with a msgParams.data field with an object type.

I am not sure how to proceed from here -- arguably the test file should stringize the object type first (by way of hashing I suppose?), but I can see that it would also make sense for recoverTypedSignature* functions to hash msgParams.data should they detect that the data field is not of the string type.

Secondly, I am not sure what the purpose of the TypedDataUtils.sign function is, because it seems to me that it wants to construct a properly typed object that conforms to the message schema, and so it would make sense for the typedData parameter to be of an object type that somewhat follows the message schema, perhaps with some fields missing. However, according to the call sites for this function, it often accepts a string type for the typedData parameter. This makes me highly suspect that there has been subtle bugs on the implementation for this function, since there is a disagreement about the inferred types from usage when looking at the function body vs the inferred types at the call site.

I will push a commit on my current work on attempting to type msgParams and typedData correctly. I'm sure there are other places and functions where type disagreements happen. What do you think is the best course of action here?

danfinlay · 2020-01-08T23:50:02Z

Thanks for the quick investigation, @KiChjang.

I think it's very likely that the typefile is just bad, and we may need to disregard a lot of it.

I definitely think using an object as params is plainly superior, and we should refer to the testfile and the sig-examples repo as more realistic reference points instead when possible.

As for TypedUtils.sign, it seems clear it does no signing, it is clearly actually a hashing method. I believe it was broken out so that the hashing functionality could be tested independently.

As some bug fixes show up along the way, I am happy to add to the bounty, but it is tricky to price fixes along the way fairly, so feel free to suggest what would keep you feeling motivated, and if we agree to continue, we will pay out, and if not, we will simply pay you the bounty for identifying additional issues and complexity that need to be addressed before the conversion can be easily completed.

KiChjang · 2020-01-09T00:47:38Z

@danfinlay I forgot to mention -- the type mismatches that I'm seeing right now is preventing the testfile to pass. I think I will change the behavior of certain functions a slight bit (e.g. asserting that msgParams.data must be of the string type instead of simply checking for undefinedness) and push a new commit up that makes the testfile pass, then it'll be ready for review.

With this PR, I believe behaviorally, nothing will change in this library (aside from the slight modifications I mentioned earlier, which I think is inconsequential because other functions would have thrown an exception if there is a type mismatch), so we could simply merge it and file follow-up bug issues and place a bounty on them accordingly.

KiChjang · 2020-01-09T00:59:00Z

The testfile has an interesting testcase where it references a variable called alice that doesn't exist. Should I remove that testcase altogether?

danfinlay · 2020-01-09T01:00:06Z

Yes, removing broken tests like that is fine, I'll open an issue for adding a correct test for that case.

KiChjang · 2020-01-09T12:04:28Z

@danfinlay Let me know whether the current state on this PR is good to go.

KiChjang · 2020-01-10T04:41:33Z

Rebased to master.

whymarrh

@KiChjang this is a great start! I've left a few comments, I think we might need to refine a few of these types to avoid using never and some of the casts in the test file.

whymarrh · 2020-01-10T15:38:01Z

package.json

@@ -29,6 +31,8 @@
    "tweetnacl-util": "^0.15.0"
  },
  "devDependencies": {
-    "tape": "^4.9.1"
+    "tape": "^4.9.1",
+    "ts-node": "^8.5.4",


I believe we can avoid this dependency by compiling the test file as well and executing the generated JS file directly. (Related to my comments on the tsconfig below.)

We can add the test directory to .include in the tscconfig file.

whymarrh · 2020-01-10T15:39:50Z

tsconfig.json

@@ -0,0 +1,10 @@
+{
+  "compilerOptions": {
+    "outDir": "./build",


Let's leave this property out and output the JS files alongside the TS files—that produces the exact same package structure as before, allowing us to not have to modify the publish process.

whymarrh · 2020-01-10T15:41:25Z

package.json

@@ -4,7 +4,8 @@
  "description": "A few useful functions for signing ethereum data",
  "main": "index.js",
  "scripts": {
-    "test": "node test/index.js"
+    "build": "tsc -d",
+    "test": "npm run build && ts-node test/index.ts"


Suggested change

"test": "npm run build && ts-node test/index.ts"

"test": "npm run build && node test/index.js"

whymarrh · 2020-01-10T15:41:41Z

package.json

-    "tape": "^4.9.1"
+    "tape": "^4.9.1",
+    "ts-node": "^8.5.4",
+    "typescript": "3.4.3"


Can we use the latest version of TypeScript here?

whymarrh · 2020-01-10T15:49:02Z

package.json

@@ -4,7 +4,8 @@
  "description": "A few useful functions for signing ethereum data",
  "main": "index.js",
  "scripts": {
-    "test": "node test/index.js"
+    "build": "tsc -d",


Suggested change

"build": "tsc -d",

"build": "tsc --project .",

Let's use the --project flag here and keep all the compiler options in the tsconfig file. (We'll need to add "declaration": true to .compilerOptions there.)

whymarrh · 2020-01-10T15:55:56Z

test/index.ts

@@ -41,11 +41,11 @@ test('personalSign and recover', function (t) {
  const privKeyHex = '4af1bceebf7f3634ec3cff8a2c38e51178d5d4ce585c52d6043e5e2cc3418bb0'
  const privKey = Buffer.from(privKeyHex, 'hex')
  const message = 'Hello, world!'
-  const msgParams = { data: message }
+  const msgParams: sigUtil.MsgParams<never> = { data: message }


Why are we using the never type here? The never type doesn't make sense to me, semantically. We might have some of these types wrong if we need this?

Since TypedData<T> is a union type of string | EIP712TypedData | EIP712TypedData[] | TypedMessage<T>, the never type here signifies that we know for sure that the data field will never be of the TypedMessage<T> type, so it's safe to use never.

I don't particularly like this syntax either, so I'm open to suggestions on improving this.

so it's safe to use never

I don't contest that it's safe to use never here, I guess I'm more wondering if we can avoid using it in favour of something more semantic? I'll spend a bit more time thinking of an alternative.

I have an idea -- instead of trying to parameterize on the TypedData type, we can instead parameterize on the API functions themselves, i.e. make TypedData parameter-less, and then on the functions, require MsgParams<TypedData | TypedMessage<T>>.

whymarrh · 2020-01-10T15:57:54Z

test/index.ts

@@ -453,7 +447,7 @@ test('signedTypeData', (t) => {
            { name: 'contents', type: 'string' }
        ],
    },
-    primaryType: 'Mail',
+    primaryType: 'Mail' as 'Mail',


Why do we need to cast this? primaryType should be a string, no? There are a few instances of @param {string} primaryType in the JSDoc comments and TYPED_MESSAGE_SCHEMA defines it as a string?

Similarly for all of the below casts?

primaryType should be a string, no?

Not exactly. This is the type definition of TypedMessage:

export interface TypedMessage<T extends MessageTypes> { types: T; primaryType: keyof T; // ... }

In other words, the type of primaryType should only consist of the keys that appear in types. I could have made this string instead, but I figured out that we'd probably want stronger typing guarantees, and what's the point of having a type system if we can't leverage it? ;)

Yes, fair, I'm definitely in favour of stronger type guarantees. That said, this cast seems unnecessary—how can we best remove the need for it?

As I recall, this is currently a limitation of TypeScript, which doesn't allow object fields to be narrowed down to their literal types; perhaps this is the tracking issue? microsoft/TypeScript#16896

A marginally better way would be to use as const instead of as 'Mail' here, but it's purely cosmetic.

whymarrh · 2020-01-10T16:02:37Z

package.json

@@ -21,6 +22,7 @@
  },
  "homepage": "https://github.com/MetaMask/eth-sig-util#readme",
  "dependencies": {
+    "@types/node": "^13.1.2",


Can we downgrade this to ^10 (refs #77)

KiChjang · 2020-01-10T23:01:23Z

Hmm... should I .gitignore the javascript files? I don't quite want future contributors to edit those files.

EDIT: Nevermind, I think I will do just that.

- Create necessary project configuration files to support TypeScript - Annotate every function parameter and return value with its proper type

KiChjang · 2020-01-14T01:18:12Z

@whymarrh Please have a look, I believe all your review comments are adequately addressed.

whymarrh · 2020-01-17T05:17:38Z

@KiChjang thanks I'll have some time later today to look at this

whymarrh

This LGTM, the types definitely make a bit more sense now

KiChjang · 2020-01-18T12:49:06Z

@danfinlay Looks like we have an OK from a reviewer, is there anything else left in this PR that you'd like me to address, or is it good to go?

whymarrh · 2020-01-18T17:03:58Z

@rekmarks want to double-check this as well

rekmarks · 2020-01-18T18:11:56Z

@whymarrh I'll review over the course of the weekend. One initial thought is that we should get some linting up in here. I think there's an internal discussion to be had to determine our TypeScript lint rules (e.g. gaba is doing stuff I don't care for), so let's continue to not worry about style in this PR?

whymarrh · 2020-01-18T20:55:46Z

One initial thought is that we should get some linting up in here. I think there's an internal discussion to be had to determine our TypeScript lint rules (e.g. gaba is doing stuff I don't care for), so let's continue to not worry about style in this PR?

Yeah let's ignore styling for this PR, once this is merged we can iterate on the project (linting, docs, etc.)

whymarrh

Thanks @KiChjang!

whymarrh

Fixed the build 😅 thanks again

KiChjang marked this pull request as ready for review January 9, 2020 00:48

danfinlay mentioned this pull request Jan 9, 2020

Add working decryption failure test #75

Open

whymarrh requested review from whymarrh and rekmarks January 9, 2020 14:43

KiChjang force-pushed the typescript branch from 77cbba3 to f7ef465 Compare January 10, 2020 04:41

whymarrh suggested changes Jan 10, 2020

View reviewed changes

KiChjang added 8 commits January 10, 2020 23:27

Migrate from JavaScript to TypeScript

03f2503

- Create necessary project configuration files to support TypeScript - Annotate every function parameter and return value with its proper type

Rewrite module.exports in index.ts to use an export statement instead

3190c9c

Attempt to type TypedData and TypedMessage correctly

516ef3e

Correctly type the return values of hashType, hashStruct and encodeData

5c24112

Fold TypedMsgParams into MsgParams

4222e2d

Remove buggy test case

09551b2

Ensure typedSignatureHash receives a non-empty array

36c4a01

Reconfigure TypeScript compiler

ee44225

KiChjang force-pushed the typescript branch from 5c3e1a0 to ee44225 Compare January 10, 2020 23:27

KiChjang added 2 commits January 11, 2020 05:04

Change string literal type narrowing syntax

94b5772

Eliminate type parameter on TypedData

e161b2c

whymarrh reviewed Jan 18, 2020

View reviewed changes

whymarrh mentioned this pull request Jan 18, 2020

v4 Release #79

Closed

6 tasks

Merge branch 'master' into typescript

13ffc39

whymarrh previously approved these changes Jan 22, 2020

View reviewed changes

Fix package files

9a7b6df

whymarrh dismissed their stale review via 9a7b6df January 22, 2020 22:29

whymarrh approved these changes Jan 22, 2020

View reviewed changes

whymarrh merged commit ff95e45 into MetaMask:master Jan 22, 2020

whymarrh mentioned this pull request Jan 22, 2020

convert javascript to typescript #73

Closed

KiChjang deleted the typescript branch January 29, 2020 22:24

	"test": "npm run build && ts-node test/index.ts"
	"test": "npm run build && node test/index.js"

Migrate from JavaScript to TypeScript #74

Migrate from JavaScript to TypeScript #74

Conversation

KiChjang commented Jan 8, 2020 • edited Loading

KiChjang commented Jan 8, 2020

KiChjang commented Jan 8, 2020 • edited Loading

danfinlay commented Jan 8, 2020 • edited Loading

KiChjang commented Jan 9, 2020

KiChjang commented Jan 9, 2020

danfinlay commented Jan 9, 2020

KiChjang commented Jan 9, 2020

KiChjang commented Jan 10, 2020

whymarrh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiChjang Jan 10, 2020 • 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

KiChjang commented Jan 10, 2020 • edited Loading

KiChjang commented Jan 14, 2020

whymarrh commented Jan 17, 2020

whymarrh left a comment

Choose a reason for hiding this comment

KiChjang commented Jan 18, 2020

whymarrh commented Jan 18, 2020

rekmarks commented Jan 18, 2020

whymarrh commented Jan 18, 2020

whymarrh left a comment

Choose a reason for hiding this comment

whymarrh left a comment

Choose a reason for hiding this comment

KiChjang commented Jan 8, 2020 •

edited

Loading

KiChjang commented Jan 8, 2020 •

edited

Loading

danfinlay commented Jan 8, 2020 •

edited

Loading

KiChjang Jan 10, 2020 •

edited

Loading

KiChjang commented Jan 10, 2020 •

edited

Loading