Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

bumping web5 versions #149

Merged
merged 9 commits into from
Jan 23, 2024
Merged

bumping web5 versions #149

merged 9 commits into from
Jan 23, 2024

Conversation

jiyoonie9
Copy link
Contributor

@jiyoonie9 jiyoonie9 commented Jan 20, 2024

Summary

Upgrading to latest versions of @web5 packages (dids, common, crypto, credentials)
Removed dependency from Jose library in Crypto.ts
Using DidResolver.dereference() from @web5/dids package, which closes #147
Using VerifiableCredentials.create() from @web5/credentials package instead of DevTools.createCredential()
Part of an effort to break apart #126 into smaller ones. Related to #122

Copy link

changeset-bot bot commented Jan 20, 2024

🦋 Changeset detected

Latest commit: 025b95d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@tbdex/http-client Minor
@tbdex/protocol Minor
@tbdex/http-server Minor

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

@jiyoonie9 jiyoonie9 marked this pull request as ready for review January 22, 2024 19:41
Comment on lines +57 to +59
"ms": "2.1.3",
"query-string": "8.1.0",
"typeid-js": "0.3.0"
Copy link

Choose a reason for hiding this comment

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

Why are we adding ms and typeid-js to http-client in this PR? If ms is necessary in dependencies, why is @types/ms in devDepencencies?

Copy link
Contributor Author

@jiyoonie9 jiyoonie9 Jan 23, 2024

Choose a reason for hiding this comment

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

Why are we adding ms and typeid-js to http-client in this PR?

so this PR is part of a 4 PR series in an effort to break down moe's 122-request-token PR which is a chonky boi.

i just ported over the client/package.json changes from the original PR, which includes ms and typeid-js since they're being used by the client during request token generation.

the PR that will use the ms and typeid-js is here

If ms is necessary in dependencies, why is @types/ms in devDepencencies?

i thought we needed to include @types/xyz in devDependencies that correspond to package xyz in dependencies in order to write with type checking during development. is that incorrect?

Copy link

Choose a reason for hiding this comment

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

i thought we needed to include @types/xyz in devDependencies that correspond to package xyz in dependencies in order to write with type checking during development. is that incorrect?

ah, u right

Copy link
Member

Choose a reason for hiding this comment

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

yee that's correct @jiyoontbd

@@ -78,7 +82,7 @@
"mkdirp": "3.0.1",
"mocha": "10.2.0",
"rimraf": "4.4.0",
"sinon": "15.0.2",
"sinon": "17.0.1",
Copy link

Choose a reason for hiding this comment

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

Should sinon and @types/sinon have matching versions? Only different by patch version so probably not a big deal regardless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, ty!

const algorithmName = privateKeyJwk?.['alg'] || ''
let namedCurve = Crypto.extractNamedCurve(privateKeyJwk)
const algorithmName = privateKeyJwk['alg'] || ''
const namedCurve = privateKeyJwk['crv'] || ''
Copy link

Choose a reason for hiding this comment

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

CI is failing because you're reverting some of the changes from #141. Might need to rebase.

Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

Updating web5 deps unblocks #147, which has a TODO in protocol/src/did-resolver.ts. We can tackle that in this PR or a follow up, up to you @jiyoontbd.

Copy link
Contributor

github-actions bot commented Jan 23, 2024

TBDocs Report

🛑 Errors: 0
⚠️ Warnings: 2

@tbdex/protocol

  • Project entry file: packages/protocol/src/main.ts
📄 File: packages/protocol/src/dev-tools.ts
⚠️ extractor:ae-unresolved-link: The @link reference could not be resolved: No member was found with name "createCredential" #L37
⚠️ extractor:ae-unresolved-link: The @link reference could not be resolved: No member was found with name "createJwt" #L48

@tbdex/http-client

  • Project entry file: packages/http-client/src/main.ts

@tbdex/http-server

  • Project entry file: packages/http-server/src/main.ts

TBDocs Report Updated at 2024-01-23T22:33:33Z 025b95d

@jiyoonie9
Copy link
Contributor Author

ci is still failing because of test vector tests in packages/protocol, but that will be addressed in this test vector specific PR

@kirahsapong
Copy link
Contributor

🎉 nice!

Can we also clean up this TODO in offering.ts which is just removing the type annotation following the web5/credentials pkg update?

- get requiredClaims(): PresentationDefinitionV2 { ... }
+ get requiredClaims() { ... }

I can push a quick commit for that if you prefer - lmk!

@jiyoonie9
Copy link
Contributor Author

🎉 nice!

Can we also clean up this TODO in offering.ts which is just removing the type annotation following the web5/credentials pkg update?

- get requiredClaims(): PresentationDefinitionV2 { ... }
+ get requiredClaims() { ... }

I can push a quick commit for that if you prefer - lmk!

@kirahsapong removed the return type

Copy link
Contributor

@kirahsapong kirahsapong left a comment

Choose a reason for hiding this comment

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

you're a star ⭐

@jiyoonie9 jiyoonie9 merged commit c471b3d into main Jan 23, 2024
8 of 9 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants