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

Clarify documentation of KeyType #37

Closed
wants to merge 1 commit into from
Closed

Conversation

andresuribe87
Copy link
Contributor

From #34 and a conversation with @decentralgabe , added documentation to clarify what I think may be confusing.

Feedback is more than welcome.

@amika-sq
Copy link
Contributor

amika-sq commented Dec 5, 2023

Reading these additional comments, in my opinion, make the purpose of it even more confusing 😅

I added a ticket in the backlog to refactor this into something that more resembles what we have in Kotlin: #38

If others agree that this helps clarify, I'm all for adding. But to me, it's bringing in a concept (JWK kty) that consumers of the SDK don't really need to know about.

@@ -2,6 +2,13 @@ use ssi_jwk::JWK;
use ssi_jws::Error as JWSError;

/// Enum defining all supported cryptographic key types.
///
/// Note that KeyType is NOT to be interpreted as the "kty" header defined in https://www.rfc-editor.org/rfc/rfc7517#section-4.1.
Copy link
Member

Choose a reason for hiding this comment

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

this note is not necessary since this enum is not defined in the context of JOSE

/// Instead, "kty" (the JOSE header) should be thought of as the type for a Json Web Key (JWK)
/// object. This enum should be thought of as the type of a cryptographic key.
/// KeyType maps to a JWK type, but not the other way around. For example, the KeyType of "Ed25519"
/// maps to the JWK type (i.e. the "kty" header) of "EC". The converse is not true. "EC" maps to
Copy link
Member

Choose a reason for hiding this comment

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

it's actually OKP 😉

@andresuribe87
Copy link
Contributor Author

Closing this since it's covered in #38

@KendallWeihe KendallWeihe deleted the clarify_key_type branch April 24, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants