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

refactor: improve didcore VerificationMethod in-memory representation #1170

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Mar 28, 2024

I followed the approach outlined in #1163 (comment)

  • removed PublicKeyField in favour of bytes
  • introduced a new flag that determines the serialization format
  • implemented custom serialization

I do not think that this is really an improvement since it has its own downsides:

  • we now have 2 separate attributes to determine the format of a single value
  • VerificationMethod is now responsible for implementing how to serialize other type - key should know how to serialize itself
  • while bytes can represent various key types, having just raw bytes us not that useful for working with the key. Usually, those bytes are used to construct a type that has methods for key operations

So I think we might go from:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
#[serde(untagged)]
#[serde(deny_unknown_fields)]
pub enum PublicKeyField {
    #[serde(rename_all = "camelCase")]
    Multibase { public_key_multibase: String },
    #[serde(rename_all = "camelCase")]
    Jwk { public_key_jwk: JsonWebKey },
    #[serde(rename_all = "camelCase")]
    Base58 { public_key_base58: String },
    #[serde(rename_all = "camelCase")]
    Base64 { public_key_base64: String },
    #[serde(rename_all = "camelCase")]
    Hex { public_key_hex: String },
    #[serde(rename_all = "camelCase")]
    Pem { public_key_pem: String },
    #[serde(rename_all = "camelCase")]
    Pgp { public_key_pgp: String },
}

to something like:

#[derive(Clone, Debug, PartialEq)]
pub enum PublicKeyField {
    Multibase(public_key::Key),
    Jwk (JsonWebKey),
    Base58(public_key::Key),
    Base64(public_key::Key),
    Hex(public_key::Key), 
    Pem(pem::Pem),
    Pgp(pgp::Pgp),
}

impl Serialize for PublicKeyField {...}

The key type is responsible for holding the key bytes and PublicKeyField knows how to de/serialize itself. Your thoughts?

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>
@@ -162,8 +164,9 @@ fn construct_new_did_document(
id,
did.clone(),
VerificationMethodType::Ed25519VerificationKey2018,
PublicKeyFormat::Multibase(multibase::Base::Base58Btc),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaking the implementation details to the upper layer. Can we even reliably determine the key format at this point?

@Patrik-Stas
Copy link
Contributor

@xprazak2 thanks for the insights! You have good points.

The key type is responsible for holding the key bytes and PublicKeyField knows how to de/serialize itself. Your thoughts?

I think this is good idea, lets do it!

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