Create and resolve did:key #25
Conversation
amika-sq
force-pushed
the
did-key
branch
2 times, most recently
from
f1575cd
to
72d7900
Compare
amika-sq
requested review from
andresuribe87,
KendallWeihe,
mistermoe,
decentralgabe and
diehuxx
|
Created a task in our backlog to implement this ourselves: #43 |
crates/dids/src/method/key.rs
Outdated
| let key_manager = Arc::new(LocalKeyManager::new_in_memory()); | ||
|
|
||
| DidKey::create(key_manager, DidKeyCreateOptions { key_type }) | ||
| .expect("DidKey Ed25519 creation failed") |
There was a problem hiding this comment.
Error message is hardcoded to Ed25519 as the key type rather than using the key_type parameter
There was a problem hiding this comment.
This actually begs the question... should we define default key types? (apologies if this convo has already been had)
There was a problem hiding this comment.
Fixed it as I rebased to main!
Rust doesn't have the concept of "default parameter values" like a lot of other languages have. We could define a default value for ANY KeyType if it's not present, but that didn't seem right to me. What may be a good default for one did method may not be a good default for another.
The other approach we could take is the builder pattern, which would allow us to define default values if the builder doesn't have a value set for a required field.
There was a problem hiding this comment.
Another options is we could implement the Default trait (documentation) for DidKeyCreateOptions (and the others like DidJwkCreateOptions).
Then we could define whatever default options we think are good for creating did:key with.
The only downside, the user still needs to provide a value when calling this function. It would just change to the following:
DidKey::create(key_manager, DidKeyCreateOptions::default())
There was a problem hiding this comment.
Just briefly perusing web5-js and web5-kt, we're offering at least some amount of default-value DX. I think this warrants an issue on the backlog, if only for the sake of discussion to rule it out. I don't love simplicity insofar as it robs the developer of valuable knowledge, but intuitively, I think it would be wise to offer a simple create() DX where the developer can easily get-off-zero. Which further begs the question of if the key manager should also have a default value.
There was a problem hiding this comment.
A compelling counter argument, against supporting default values, is that Rust is not a junior dev friendly language. Which is to say, not a lot of newbie developers are using Rust. In which case, default values only serve to add unnecessary overhead and confusion.
Idk just trying to steel man the counter case, I still think intuitively, default values are a good DX.
There was a problem hiding this comment.
Late in the day for me so I'm rambling, but anyways this isn't a priority whatsoever IMO
| impl DidMethod<DidKey, DidKeyCreateOptions> for DidKey { | ||
| const NAME: &'static str = "key"; | ||
|
|
||
| fn create( |
There was a problem hiding this comment.
Does it make sense to add documentation to public facing functions?
There was a problem hiding this comment.
This isn't public. DidMethod is public, and it is documented here: https://github.com/TBD54566975/web5-rs/blob/main/crates/dids/src/method/mod.rs#L21-L42
There was a problem hiding this comment.
This isn't public.
The implementation of DidMethod::create is being done via DidKey::create. Since the former is public, I thought that meant this method is public as well....
Anyways, I don't think it's important. My main concern is that there is no place in documentation that describes to consumers of the crate how to use DidKeyCreateOptions when creating a did of type key. While this is obvious for DidKey, it will become not so obvious for DidDht in the future.
There was a problem hiding this comment.
We can address this when we implement DidDht
Resolve
did:keyDIDs using Spruce's did-method-key crate. Uses our DidResolutionResult implementation so that the resolved DIDDocument & all respective metadata is in the expected format according to the community draft resolution spec here.Resolves #22