Create a secp256r1 resolution method #1261

bshambaugh · 2021-04-14T18:49:05Z

I used JsonWebKey2020 which uses publicJwk to map to an object? This is the first use of publicJwk. secp256k1 and ed25519 uses publicKeyBase58 which uses a string.

oed

Not sure this is completely right. From the other examples we always just include the X value in the public key part of the did string. Seems like you are including both X and Y.

package.json

packages/key-did-resolver/src/secp256r1.ts

oed · 2021-04-16T07:57:12Z

packages/key-did-resolver/src/__tests__/secp256r1.test.ts

+describe('Secp256r1 mapper', () => {
+
+    it('successfully resolves the document from did', async () => {
+        const id = "zruuPojWkzGPb8sVc42f2YxcTXKUTpAUbdrzVovaTBmGGNyK6cGFaA4Kp7SSLKecrxYz8Sc9d77Rss7rayYt1oFCaNJ"


Afaik the Key DIDs should only contain the X value (see secp256k1 and ed25519 examples).

does the transmute-industries/did-key.js also use this format?

If so this should be alright.

Transmute industries may be inconsistent with the present state. I believe Orie is updating a part before becoming consistent with the did method key spec and this issue.
w3c-ccg/did-method-key#29 (comment)

However, the did key is the same length as in: https://w3c-ccg.github.io/did-method-key/#p-256 .

edit: Orie is waiting on bls12381 which is not working with node 15. transmute-industries/did-key.js#78 (comment)

Should I include a library, or should I add a function for implementing BigInt to my code?

Which library are you referring to? If it's not too big then I think it's fine to include.

I handle raw (64 byte), compressed (33 byte), and uncompressed (65 byte) keys in my code now. My tests pass, but I do not have full coverage yet. I am currently trying to figure out what to do.

Ahh...line 61 was for the function ECPointDecompress . I added an extra test by taking a compressed public key and flipping the first byte from 03 to 02 and feeding it in to ECPointDecompress. Line 61 looks to be covered by this. The conditional if block that line 61 is in only executes if the parity of the compressed key is opposite (of what is should be?).

Now I need to address the pubKeyBytesToXY function. This is hinted at with the yellow colored 113 and 121 in the uncovered line #s column. I believe the function will fail if the bytesCount is 65 and the 1st byte is not 03, the bytes count is 33 and the 1st bytes is not 03 or 02, or the bytes count is not 64, 65, or 33. I guess I could have the function return null if this is the case. I am not sure how the did:key document will resolve if the function returns null. I could try to report null in place of the key, or give an error message instead of resolving the document will nulls.

I have full coverage. Unfortunately, I found some tests that I can create that can fail. I have not included them yet since they use functions included to enable the tests, not functions that are in the code. I think that the same bug may be in the code too. I am going to try to see if I can fix the bug in the test function and the code functions.

test that fail:

test('test a hex string shorter than 33 bytes', () => {
const inputPublicKeyHex = '36f8964623378bdc068d4bce07ed17c8fa486f9ac0c2613ca3c8c306d7bb6'
const publicKey_u8a = pubKeyHexToUint8Array(inputPublicKeyHex);
const pubKeyBytesToXY = mapper.pubKeyBytesToXY(publicKey_u8a);
expect(pubKeyBytesToXY).toEqual(null);
});

test('test a hex string between 64 and 33 bytes', () => {
const inputPublicKeyHex = '36f8964623378bdc068d4bce07ed17c8fa486f9ac0c2613ca3c8c306d7bb61cd36717b8ac5e4fea8ad23dc8d0783c2318ee4ad7a80db6e0026ad0b072a24f'
const publicKey_u8a = pubKeyHexToUint8Array(inputPublicKeyHex);
const pubKeyBytesToXY = mapper.pubKeyBytesToXY(publicKey_u8a);
expect(pubKeyBytesToXY).toEqual(null);
});

I suspect these both fail since the hex string has (I'm guessing) an odd number of characters.

packages/key-did-resolver/src/secp256r1.ts

oed · 2021-05-14T08:52:13Z

@bshambaugh please let us know when this is ready for a full review again :)

bshambaugh · 2021-05-19T19:56:01Z

@bshambaugh please let us know when this is ready for a full review again :)

I'm dipping my toes into the case where I have nulls coming into the functions:

https://gist.github.com/bshambaugh/333a4d28e3fdb275ed20422e97030e04

oed · 2021-05-20T07:17:35Z

There's something wrong with this PR theres 269 changed files. Looks like you included build outputs in the src folder somehow. Please fix this and I'll do a final review!

bshambaugh · 2021-05-21T14:39:04Z

There is not full test coverage for index.ts

oed

This looks great. Just add the missing dependency bigint-mod-arith to the package.json. Will merge after this is done.

Tests are passing locally when adding the missing dependency.

oed · 2021-05-21T15:38:44Z

packages/key-did-resolver/src/secp256r1.ts

+   }
+ }
+
+ return XYpairObject;


Maybe throw an error if this is null?

oed · 2021-05-24T06:37:37Z

Still seeing issues with this branch. Can you try to do the following:

Merge develop into this branch:

git merge develop

Make sure build and lint passes:

npm run lint
// and
npm run build

That should be it.

…rors for functions that are dependencies

bshambaugh · 2021-05-24T19:55:53Z

Still seeing issues with this branch. Can you try to do the following:

Merge develop into this branch:
git merge develop
Make sure build and lint passes:
npm run lint
// and
npm run build

yes, I still need to work through lint and build. update: they both work without errors or warnings.

That should be it.

…equals undefined

…r1.ts code

bshambaugh · 2021-05-25T02:41:43Z

In addition to keyToDidDoc which is used by
https://github.com/ceramicnetwork/js-ceramic/blob/develop/packages/key-did-resolver/src/index.ts , the following functions are exported for testing purposes:

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L54
ECPointDecompress

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L80
publicKeyToXY

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L87
publicKeyHexToUint8ArrayPointPair

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L95
publicKeyIntToXY

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L102
publicKeyIntToUint8ArrayPointPair

https://github.com/bshambaugh/js-ceramic/blob/develop/packages/key-did-resolver/src/secp256r1.ts#L110
pubKeyBytesToXY

With the exception of pubKeyBytesToXY, all of these functions are known to fail for null and undefined.

This might be okay since https://github.com/ceramicnetwork/js-ceramic/blob/develop/packages/key-did-resolver/src/index.ts only exports getResolver which only uses keyToDidDoc which only uses pubKeyBytesToXY which has error checking.

… additional tests to null. Assume this covers undefined.

bshambaugh · 2021-05-25T04:36:37Z

To my knowledge, everything looks good.

…ile. Fix functions to allow undefined cases to pass.

oed

Looks great 👍

Btw, I made a PR to the multicodecs table to clarify that the compressed format should be used: multiformats/multicodec#214

oed · 2021-05-25T13:11:58Z

@bshambaugh looks like the docs are not building. My bad for not mentioning this to you. Could you make another PR to make sure that npm run docs passes?

bshambaugh · 2021-05-25T13:24:54Z

key-did-resolver$ npm run docs
npm ERR! Missing script: "docs"
npm ERR!
npm ERR! To see a list of scripts, run:
npm ERR! npm run

npm ERR! A complete log of this run can be found in:
npm ERR! /home/ubuntu/.npm/_logs/2021-05-25T13_23_41_860Z-debug.lo

oed · 2021-05-25T13:27:35Z

@bshambaugh you need to run it in the root directory!

This reverts commit 0df0906.

oed · 2021-05-25T13:29:48Z

@bshambaugh We had to revert the change because the docs script didn't pass. Can you please submit a new PR for this feature which includes working docs? 🙏

oed reviewed Apr 16, 2021

View reviewed changes

bshambaugh added 2 commits May 21, 2021 09:10

commit a temporary file called backup

c969509

rebase to april 9th, and bring in desireable changes to may19th

984120d

bshambaugh force-pushed the develop branch from 8f4923e to 984120d Compare May 21, 2021 14:16

remove tests for null and related functions because they are not needed

f80be1c

oed requested changes May 21, 2021

View reviewed changes

packages/key-did-resolver/src/secp256r1.ts Outdated

}

}

return XYpairObject;

Copy link

Member

oed May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe throw an error if this is null?

Add "bigint-mod-arith": "^3.0.0" to key-did-resolver package.json

ded589c

bshambaugh added 2 commits May 24, 2021 11:50

add a try and finally blocks for pubKeyBytesToXY add tests to show er…

2eba9a4

…rors for functions that are dependencies

removed try and finally blocks from secp256r1.ts

2d238ca

bshambaugh added 3 commits May 24, 2021 17:37

refactored code to get linter to pass

cc5fb77

simplify TypeError conditional in pubKeyBytesToXY since null loosely …

b43f87a

…equals undefined

add additional tests for malformed did:keys , add comments to secp256…

2b7305a

…r1.ts code

move test for functions in the test file to the end. Clean up and add…

381595d

… additional tests to null. Assume this covers undefined.

add tests for undefined case for functions declared for use in test f…

1e0a343

…ile. Fix functions to allow undefined cases to pass.

oed approved these changes May 25, 2021

View reviewed changes

oed merged commit 0df0906 into ceramicnetwork:develop May 25, 2021

stbrody pushed a commit that referenced this pull request May 25, 2021

Revert "feat(key-did-resolver): Add support for secp256r1 (#1261)"

57e4f03

This reverts commit 0df0906.

bshambaugh mentioned this pull request May 27, 2021

Merge secp256r1 key-did-method with npm run docs working #1418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a secp256r1 resolution method #1261

Create a secp256r1 resolution method #1261

bshambaugh commented Apr 14, 2021 •

edited

Loading

oed left a comment

oed Apr 16, 2021

oed Apr 24, 2021

oed Apr 24, 2021

bshambaugh Apr 26, 2021

bshambaugh Apr 26, 2021

oed May 7, 2021

bshambaugh May 13, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

oed commented May 14, 2021

bshambaugh commented May 19, 2021

oed commented May 20, 2021

bshambaugh commented May 21, 2021

oed left a comment

oed May 21, 2021

oed commented May 24, 2021

bshambaugh commented May 24, 2021 •

edited

Loading

bshambaugh commented May 25, 2021 •

edited

Loading

bshambaugh commented May 25, 2021

oed left a comment

oed commented May 25, 2021

bshambaugh commented May 25, 2021

oed commented May 25, 2021

oed commented May 25, 2021

Create a secp256r1 resolution method #1261

Create a secp256r1 resolution method #1261

Conversation

bshambaugh commented Apr 14, 2021 • edited Loading

oed left a comment

Choose a reason for hiding this comment

oed Apr 16, 2021

Choose a reason for hiding this comment

oed Apr 24, 2021

Choose a reason for hiding this comment

oed Apr 24, 2021

Choose a reason for hiding this comment

bshambaugh Apr 26, 2021

Choose a reason for hiding this comment

bshambaugh Apr 26, 2021

Choose a reason for hiding this comment

oed May 7, 2021

Choose a reason for hiding this comment

bshambaugh May 13, 2021 • edited Loading

Choose a reason for hiding this comment

bshambaugh May 14, 2021 • edited Loading

Choose a reason for hiding this comment

bshambaugh May 14, 2021 • edited Loading

Choose a reason for hiding this comment

bshambaugh May 14, 2021 • edited Loading

Choose a reason for hiding this comment

oed commented May 14, 2021

bshambaugh commented May 19, 2021

oed commented May 20, 2021

bshambaugh commented May 21, 2021

oed left a comment

Choose a reason for hiding this comment

oed May 21, 2021

Choose a reason for hiding this comment

oed commented May 24, 2021

bshambaugh commented May 24, 2021 • edited Loading

bshambaugh commented May 25, 2021 • edited Loading

bshambaugh commented May 25, 2021

oed left a comment

Choose a reason for hiding this comment

oed commented May 25, 2021

bshambaugh commented May 25, 2021

oed commented May 25, 2021

oed commented May 25, 2021

bshambaugh commented Apr 14, 2021 •

edited

Loading

bshambaugh May 13, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

bshambaugh May 14, 2021 •

edited

Loading

bshambaugh commented May 24, 2021 •

edited

Loading

bshambaugh commented May 25, 2021 •

edited

Loading