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

Add full DidJwk & bind to kt #305

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Conversation

KendallWeihe
Copy link
Contributor

No description provided.

Comment on lines 80 to 93
let did = match Did::parse(uri) {
Ok(d) => d,
Err(_) => return ResolutionResult::from_error(ResolutionMetadataError::InvalidDid),
};

let decoded_jwk = match general_purpose::URL_SAFE_NO_PAD.decode(did.id) {
Ok(dj) => dj,
Err(_) => return ResolutionResult::from_error(ResolutionMetadataError::InvalidDid),
};

let public_jwk = match serde_json::from_slice::<Jwk>(&decoded_jwk) {
Ok(pj) => pj,
Err(_) => return ResolutionResult::from_error(ResolutionMetadataError::InvalidDid),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

.map_err()? might be more concise than match statements here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type is ResolutionResult, not Result. Might be a better option than I have right now tho

Comment on lines +610 to 611
STATIC METHOD create(options: DidJwkCreateOptions): BearerDid
STATIC METHOD resolve(uri: string): ResolutionResult
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to also change did:dht and did:dht to just have static create and remove constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, as well as did:web

@@ -9,7 +9,7 @@ use super::{
use crate::{
crypto::{
dsa::Signer,
key_managers::{in_memory_key_manager::InMemoryKeyManager, key_manager::KeyManager},
key_managers::{in_memory_key_manager::InMemoryKeyManager, KeyManager},
Copy link
Contributor

Choose a reason for hiding this comment

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

we can address export hierarchy/API in a separate PR, but mentioning just to put the idea out there: key_managers::in_memory_key_manager::InMemoryKeyManager is a lot of times to say "key manager" before I get to the thing I need. Luckily rust makes it easy to export things under the exact namespace we want while still keeping or underlying file hierarchy the same.

Some musings about namespaces in this talk about python may also apply in rust. Namely, namespaces are best for preventing collisions, and can become cumbersome when used to create taxonomies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree and it would be awesome if you want to spearhead @diehuxx

IMO we should go as far as to define namespace in the APID such that all projects have the same namespacing

@KendallWeihe KendallWeihe merged commit 2ebfde2 into kendall/key-managers Aug 22, 2024
15 checks passed
@KendallWeihe KendallWeihe deleted the kendall/did-jwk branch August 22, 2024 19:11
KendallWeihe added a commit that referenced this pull request Aug 22, 2024
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