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

feat: add did:webvh controlller service #487

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

3kz2501
Copy link
Contributor

@3kz2501 3kz2501 commented Feb 5, 2025

No description provided.

@3kz2501 3kz2501 self-assigned this Feb 5, 2025
@3kz2501 3kz2501 requested a review from a team as a code owner February 5, 2025 08:03
@3kz2501 3kz2501 requested a review from tzskp1 February 5, 2025 08:03
@3kz2501 3kz2501 marked this pull request as draft February 5, 2025 08:03
<x25519_dalek::PublicKey as Into<Jwk>>::into(keyring.encrypt.get_public_key());

let mut log_entry = DidLogEntry::new(path)?;
let update_keypair = keyring.update;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keyring has not been modified to remain temporarily compatible with sidetree.As soon as we have finished integrating did:webvh into the Agent, we will change it to the correct naming and so on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that incompatible points is not only naming but also key type.
I implement JWK of ed25519. You should use appropriate JWK type.

impl TryFrom<&Jwk> for ed25519_dalek::VerifyingKey {
type Error = JwkToEd25519Error;
fn try_from(value: &Jwk) -> Result<Self, Self::Error> {
if value.crv != "Ed25519" {
return Err(JwkToEd25519Error::DifferentCrv);
}
let pk = BASE64URL_NOPAD
.decode(value.x.as_bytes())
.map_err(|e| JwkToEd25519Error::Decode(Some(e)))?;
let pk: [u8; 32] = pk.try_into().map_err(|_| JwkToEd25519Error::Decode(None))?;
Ok(ed25519_dalek::VerifyingKey::from_bytes(&pk)?)
}
}
impl From<&ed25519_dalek::VerifyingKey> for Jwk {
fn from(value: &ed25519_dalek::VerifyingKey) -> Self {
let x = BASE64URL_NOPAD.encode(value.as_bytes());
let kty = "OKP".to_string();
let crv = "Ed25519".to_string();
Jwk {
kty,
crv,
x,
y: None,
}
}
}

// if prerotation is enabled, add the prerotation key to the next_key_hashes
if enable_prerotation {
let prerotation_pub_key =
multibase_encode(&keyring.recovery.get_public_key().to_sec1_bytes());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.

Keyring has not been modified to remain temporarily compatible with sidetree.As soon as we have finished integrating did:webvh into the Agent, we will change it to the correct naming and so on.

@3kz2501 3kz2501 marked this pull request as ready for review February 6, 2025 04:35
Copy link
Contributor

@tzskp1 tzskp1 left a comment

Choose a reason for hiding this comment

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

Strictly a comment on the previous PR, but I think Option<Vec<_>> is sufficient for Vec<_>>

diff --git a/protocol/src/did_webvh/domain/did_document.rs b/protocol/src/did_webvh/domain/did_document.rs
index d52ad76..96e953a 100644
--- a/protocol/src/did_webvh/domain/did_document.rs
+++ b/protocol/src/did_webvh/domain/did_document.rs
@@ -20,10 +20,10 @@ pub struct DidDocument {
     pub id: Did,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub deactivated: Option<bool>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub verification_method: Option<Vec<VerificationMethod>>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub authentication: Option<Vec<String>>,
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    pub verification_method: Vec<VerificationMethod>,
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    pub authentication: Vec<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub assertion_method: Option<Vec<String>>,
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -71,8 +71,8 @@ impl DidDocument {
         Self {
             context: vec!["https://www.w3.org/ns/did/v1".to_string()],
             id,
-            verification_method: None,
-            authentication: None,
+            verification_method: vec![],
+            authentication: vec![],
             assertion_method: None,
             key_agreement: None,
             capability_invocation: None,
@@ -86,22 +86,16 @@ impl DidDocument {
     }
 
     pub fn add_verification_method(&mut self, verification_method: VerificationMethod) {
-        if self.verification_method.is_none() {
-            self.verification_method = Some(vec![]);
-        }
-        self.verification_method
-            .as_mut()
-            .unwrap()
-            .push(verification_method);
+        self.verification_method.push(verification_method);
     }
 
     pub fn get_key(&self, key_type: &str) -> Result<Jwk, GetPublicKeyError> {
         let did = &self.id;
-        let public_key = self.verification_method.as_ref().and_then(|v| {
-            v.iter()
-                .find(|vm| vm.id.ends_with(key_type))
-                .and_then(|vm| vm.public_key_jwk.as_ref())
-        });
+        let public_key = self
+            .verification_method
+            .iter()
+            .find(|vm| vm.id.ends_with(key_type))
+            .and_then(|vm| vm.public_key_jwk.as_ref());
         public_key
             .cloned()
             .ok_or_else(|| GetPublicKeyError::PublicKeyNotFound(did.to_string()))
diff --git a/protocol/src/did_webvh/domain/did_log_entry.rs b/protocol/src/did_webvh/domain/did_log_entry.rs
index bb5401f..8bca4c1 100644
--- a/protocol/src/did_webvh/domain/did_log_entry.rs
+++ b/protocol/src/did_webvh/domain/did_log_entry.rs
@@ -256,12 +256,10 @@ impl DidLogEntry {
             .map_err(|_| DidLogEntryError::InvalidState)?
             .replace_scid(scid);
         self.state.id = did.get_did().clone();
-        if let Some(verification_methods) = self.state.verification_method.as_mut() {
-            for verification_method in verification_methods.iter_mut() {
-                // id is did#key format, so only need to replace the did part
-                verification_method.id = verification_method.id.replace(DIDWEBVH_PLACEHOLDER, scid);
-                verification_method.controller = did.get_did().clone();
-            }
+        for verification_method in self.state.verification_method.iter_mut() {
+            // id is did#key format, so only need to replace the did part
+            verification_method.id = verification_method.id.replace(DIDWEBVH_PLACEHOLDER, scid);
+            verification_method.controller = did.get_did().clone();
         }
 
         Ok(())
@@ -321,15 +319,13 @@ impl DidLogEntry {
 
     // calculate the next key hashes by the Update Keys from the previous entry.
     pub fn calc_next_key_hash(&self, keys: &[String]) -> Result<Vec<String>, DidLogEntryError> {
-        let generate_hashed_keys = |keys: &[String]| {
-            keys.iter()
-                .map(|key| {
-                    generate_multihash_with_base58_encode(key.as_bytes())
-                        .map_err(|_| DidLogEntryError::FaildMultihash)
-                })
-                .collect::<Result<Vec<String>, DidLogEntryError>>()
-        };
-        let next_key_hashes = generate_hashed_keys(keys)?;
+        let next_key_hashes = keys
+            .iter()
+            .map(|key| {
+                generate_multihash_with_base58_encode(key.as_bytes())
+                    .map_err(|_| DidLogEntryError::FaildMultihash)
+            })
+            .collect::<Result<Vec<String>, DidLogEntryError>>()?;
         Ok(next_key_hashes)
     }
 }
diff --git a/protocol/src/did_webvh/service/controller/did_webvh_controller_service.rs b/protocol/src/did_webvh/service/controller/did_webvh_controller_service.rs
index bec82fe..a9d9605 100644
--- a/protocol/src/did_webvh/service/controller/did_webvh_controller_service.rs
+++ b/protocol/src/did_webvh/service/controller/did_webvh_controller_service.rs
@@ -90,7 +90,7 @@ where
         if enable_prerotation {
             let prerotation_pub_key =
                 multibase_encode(&keyring.recovery.get_public_key().to_sec1_bytes());
-            let prerotation_keys = vec![prerotation_pub_key.clone()];
+            let prerotation_keys = vec![prerotation_pub_key];
             let next_key_hases = log_entry.calc_next_key_hash(&prerotation_keys)?;
             log_entry.parameters.next_key_hashes = Some(next_key_hases);
         }

}

#[trait_variant::make(Send)]
pub trait DidWebvhService: Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that such naming like *Service is not good.
In this case, something like DidWebvhController would be better.

<x25519_dalek::PublicKey as Into<Jwk>>::into(keyring.encrypt.get_public_key());

let mut log_entry = DidLogEntry::new(path)?;
let update_keypair = keyring.update;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that incompatible points is not only naming but also key type.
I implement JWK of ed25519. You should use appropriate JWK type.

impl TryFrom<&Jwk> for ed25519_dalek::VerifyingKey {
type Error = JwkToEd25519Error;
fn try_from(value: &Jwk) -> Result<Self, Self::Error> {
if value.crv != "Ed25519" {
return Err(JwkToEd25519Error::DifferentCrv);
}
let pk = BASE64URL_NOPAD
.decode(value.x.as_bytes())
.map_err(|e| JwkToEd25519Error::Decode(Some(e)))?;
let pk: [u8; 32] = pk.try_into().map_err(|_| JwkToEd25519Error::Decode(None))?;
Ok(ed25519_dalek::VerifyingKey::from_bytes(&pk)?)
}
}
impl From<&ed25519_dalek::VerifyingKey> for Jwk {
fn from(value: &ed25519_dalek::VerifyingKey) -> Self {
let x = BASE64URL_NOPAD.encode(value.as_bytes());
let kty = "OKP".to_string();
let crv = "Ed25519".to_string();
Jwk {
kty,
crv,
x,
y: None,
}
}
}

pub fn calc_next_key_hash(&self, keys: &Vec<String>) -> Result<Self, DidLogEntryError> {
let generate_hashed_keys = |keys: &Vec<String>| {
pub fn calc_next_key_hash(&self, keys: &[String]) -> Result<Vec<String>, DidLogEntryError> {
let generate_hashed_keys = |keys: &[String]| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this temporary closure is not required.
How about this one?

        let next_key_hashes = keys
            .iter()
            .map(|key| {
                generate_multihash_with_base58_encode(key.as_bytes())
                    .map_err(|_| DidLogEntryError::FaildMultihash)
            })
            .collect::<Result<Vec<String>, DidLogEntryError>>()?;
        Ok(next_key_hashes)

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.

2 participants