Update web5/dids, web5/credentials, web5/crypto, web5/common to latest #177
Conversation
kirahsapong
requested review from
michaelneale,
mistermoe,
jiyoontbd,
phoebe-lew,
KendallWeihe and
diehuxx
as code owners
🦋 Changeset detectedLatest commit: fad1deb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TBDocs Report 🛑 Errors: 0 @tbdex/protocol
@tbdex/http-client
@tbdex/http-server
TBDocs Report Updated at 2024-02-16T17:11:28Z |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
==========================================
- Coverage 89.13% 88.99% -0.15%
==========================================
Files 36 36
Lines 2871 2825 -46
Branches 268 262 -6
==========================================
- Hits 2559 2514 -45
+ Misses 312 311 -1
|
i'm fixing these in #176 fyi |
…sts, address pr comments
|
opened an PR in tbdex test vectors which will need to merge to get passing CI - looking for reviews there |
stamped @kirahsapong ! |
|
merged TBD54566975/tbdex#236 |
| @@ -41,44 +39,11 @@ export type VerifyOptions = { | |||
| signature: string | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
nice, this whole file looks a lot better using the new sdk calls
| @@ -50,7 +50,7 @@ describe('POST /exchanges/:exchangeId/rfq', () => { | |||
|
|
|||
| it('returns a 400 if create exchange request contains a replyTo which is not a valid URL', async () => { | |||
| const aliceDid = await DevTools.createDid() | |||
| const pfiDid = await DevTools.createDid() | |||
| const pfiDid = await DevTools.createDid('dht') | |||
There was a problem hiding this comment.
is DevTools createDid needed at this point? Can we just do DidDht.create()
There was a problem hiding this comment.
we certainly could! though DevTools is just calling DidDht.create() - prob worth deciding what convention we should stick with for consistency! I will track to follow up with
There was a problem hiding this comment.
IMO we should we should only use Devtools when it is significantly more concise to use Devtools AND it is appropriate to use dummy data without needing to modify the output of Devtools very much. I.e. in this case, I would switch to DidDht.create()
There was a problem hiding this comment.
Issue here for tracking!
There was a problem hiding this comment.
At least a few breaking changes here, may be worth documenting them in some capacity (even if it's just in the PR description?), for example I think some of the function signatures expect a BearerDID instead of a PortableDID
That may be unreasonable at this moment, so I'll defer to best judgement
Great job! ✅
|
|
||
| /** | ||
| * Can be used to resolve did:ion and did:key DIDs | ||
| * | ||
| * @beta | ||
| */ | ||
| export const DidResolver = new Web5DidResolver({ | ||
| didResolvers: [DidIonMethod, DidKeyMethod, DidDhtMethod] | ||
| didResolvers: [DidDht, DidJwk, DidWeb] |
There was a problem hiding this comment.
are we sure we want to gut out ion? Our current pfi uses ion right?
There was a problem hiding this comment.
seems we're going full deprecation mode! def will be a big breaking change. we will update our pfi-exemplar right afterward to use DHT! alternatively, any strong opinion against and i can definitely move it back in so we still have resolution available
| @@ -6,7 +6,7 @@ import { Crypto, DevTools } from '../src/main.js' | |||
| describe('Crypto', () => { | |||
| describe('sign / verify', () => { | |||
| it('works with did:ion', async () => { | |||
| const alice = await DevTools.createDid('ion') | |||
| const alice = await DevTools.createDid() | |||
There was a problem hiding this comment.
is it possible to not use devtools anymore here?
There was a problem hiding this comment.
is the plan to move away from DevTools?
cc: @KendallWeihe
There was a problem hiding this comment.
well... maybe, but at least not right now
maybe we should open a ticket on the backlog to discuss?
There was a problem hiding this comment.
@KendallWeihe @nitro-neal looking through the various tests we have DevTools abstracting away the underlying DidDht and DidJwk calls for almost all, except in http-client, which uses DevTools other utility functions, and one test in http-server.
For the sake of consistency, should I change these two isolated instances to use DevTools as well, and then keep the larger discussion open for future change?
There was a problem hiding this comment.
nah I vote keep the changes specific to the intent of the PR here (upgrade) and then we'll follow up with a design discussion on le backlog
There was a problem hiding this comment.
ok yeah word! good call, thanks!
There was a problem hiding this comment.
mentioned up above but: Issue #178 for tracking!
|
might be good to create a test or two that use a portable did as well |
I don't think the compiler will accept anything that isn't a |
closes #160