Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace usage of nimbusds Algorithm and Curve with our own enums #110

Closed
mistermoe opened this issue Nov 1, 2023 · 6 comments · Fixed by #239
Closed

replace usage of nimbusds Algorithm and Curve with our own enums #110

mistermoe opened this issue Nov 1, 2023 · 6 comments · Fixed by #239
Assignees
Labels
good first issue Good for newcomers

Comments

@mistermoe
Copy link
Member

Currently, Crypto, KeyManager, and DidKey expect nimbusds JWSAlgorithm and Curve arguments for various methods. we should implement our own enums and use those instead. surfacing a dependency's enums as arguments to our libs causes confusion. Confusion == kaka DevEX

@andresuribe87
Copy link
Contributor

surfacing a dependency's enums as arguments to our libs causes confusion

@mistermoe can you elaborate on what confusion this is causing?

The way I think about it is:

@mistermoe
Copy link
Member Author

yep,

  • JWSAlgorithm is confusing from a naming perspective
  • we're asking callers to pass in types from third party libs
    • walking away from nimbusds will lead to breaking changes for our public api surface
    • if nimbusds changes these types in subsequent versions, we can get stuck on older versions or sort of be at the mercy of their public api surface

@bradleydwyer
Copy link

we're asking callers to pass in types from third party libs
walking away from nimbusds will lead to breaking changes for our public api surface
if nimbusds changes these types in subsequent versions, we can get stuck on older versions or sort of be at the mercy of their public api surface

@andresuribe87 This is the core issue for me. I believe if we are to provide tools for decentralised identity that we should commit to providing the types necessary to support that. I can also see a lot of benefit in a single narrative/style for how we present our SDKs, of which types are a significant part.

@decentralgabe
Copy link
Member

@mistermoe can you provide an example of our own enum to illustrate how it'd simplify things?

@tomdaffurn
Copy link
Contributor

tomdaffurn commented Dec 5, 2023

We're exposing other Nimbus objects, such as JWK. Thoughts on making our own?

@andresuribe87
Copy link
Contributor

Thoughts on making our own?

Yes, I think we should make our own. In general, I think we should follow https://jlbp.dev/JLBP-2. In particular, "Avoid exposing types from your dependencies".

So this issue can be split into two categories of tasks:

  1. Avoid exposing types from your dependencies by replacing them with our own types. This includes Curve, Algorithm, JWK, and some other types from the com.danubetech package.
  2. Decide what the best API surface for the types that's being replaced.

For 2, related to Curve and Algorithm, there has been discussion in TBD54566975/web5-rs#38 and in TBD54566975/web5-js#271. The proposal is to one of: KeyType and Algorithm + Optional(Curve) and AlgorithmIdentifier. I think it makes sense to focus this issue on the decision of which of the three different abstractions we'll go for, and then make the appropriate changes.

@jiyoonie9 jiyoonie9 removed the status in SDK Development Feb 12, 2024
@jiyoonie9 jiyoonie9 assigned jiyoonie9 and unassigned andresuribe87 Feb 12, 2024
@jiyoonie9 jiyoonie9 moved this to In Progress in SDK Development Feb 13, 2024
@jiyoonie9 jiyoonie9 linked a pull request Feb 14, 2024 that will close this issue
4 tasks
@github-project-automation github-project-automation bot moved this from In Progress to Done in SDK Development Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants