Refactor Storage Signature #738

HenriqueNogara · 2022-03-21T14:37:25Z

Description of change

Change account-core Signature to new type struct, removing unused PublicKey. Remove Serialize/Deserialize for Key and bindings-derive feature

Links to any relevant issues

#669

fixes #722

Type of change

Bug fix (a non-breaking change which fixes an issue)
Enhancement (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Fix

How the change has been tested

All existing tests and examples have been run

Change checklist

I have followed the contribution guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

PhilippGackstatter

It looks like you have a commit related to the release, that is not on dev somehow? Maybe you can check with @eike-hass what's going on.

Maybe I missed the conversation, but why is Signature an enum? I think we can just make the signature a wrapper around bytes.

Edit: Looks like @eike-hass force-pushed dev (?), so you should get rid of 38069e8ba0eb5427eabfff3e100ecd7ab88be7b7 here.

PhilippGackstatter · 2022-03-21T15:24:55Z

identity-account-storage/src/types/signature.rs

-  pub fn data(&self) -> &[u8] {
-    &self.data
+  /// Consumes the signature and returns it as bytes
+  pub fn as_bytes(self) -> Vec<u8> {


Not sure why clippy's wrong_self_convention isn't triggered here, this should be called into_bytes. Not sure about the JS method you're binding to, but I would probably go with toBytes.

PhilippGackstatter · 2022-03-21T15:25:31Z

identity-account-storage/src/types/signature.rs

+/// A digital signature.
+#[derive(Clone, Deserialize, Serialize)]
+pub enum Signature {
+  Ed25519(Vec<u8>),


Do we need this to be an enum? I think we can just use pub struct Signature(Vec<u8>).

We can, but if we want to add support to something else latter on, we would have to change it.

We could also use Vec<u8>/UInt8Array (or a newtype wrapper) for now since we only support those types of signatures then refactor it when we need to support something more exotic?

Edit: or use SignatureValue. Really not sure on which design is best, just that the previous design with public key was wrong.

I assume any signature can be represented as bytes, and what type of signature it is, is clear from context, e.g. when calling key_sign with a KeyLocation that contains a certain MethodType. So having to do the additional match seems unnecessary, unless the Storage implementation is simply wrong and returns something else. It's maybe slightly safer, but I don't really see a major advantage.
At least judging by RemoteSign, I think it's better not to use SignatureValue, so we don't accidentally return that signature value, but have to first encode it as base58 and then return an instance of SignatureValue.

identity.rs/identity-account-storage/src/crypto/remote.rs

Lines 53 to 56 in 1271d95

let message: Vec<u8> = data.to_jcs()?;

let signature: Vec<u8> = RemoteSign::sign(&message, remote_key).await?;

let signature: String = encode_b58(&signature);

Ok(SignatureValue::Signature(signature))

I assume any signature can be represented as bytes

I'm not sure that's a strong assumption. For instance the BBS+ Signatures 2020 spec requires a nonce value used to derive the proof as well, so it's probable that we will need more than just the signature/proof bytes in the future. One could argue that the nonce could be predetermined and passed to the sign function but that just moves the problem to the signing interface not being general enough to handle it. I think I misunderstood nonce as something passed to the signature algorithm: I think it's just something similar to a random challenge added to the data to show it's not replayed. This would strengthen the argument for just using Vec<u8> rather than an enum for now.

At least judging by RemoteSign, I think it's better not to use SignatureValue,

I agree, SignatureValue does not seem fully appropriate for this as it would require Storage implementers to also implement the signature schemes such as JCSEd25519Signature2020 and handle encoding rather than just the raw key signing.

I'm fine using just Vec<u8> or a similar new-type wrapper, until we need to refactor it to handle a new method type, if we cannot come to a consensus on what a future-proof design looks like now.

cycraig

~~This has a bunch of release changes, this may have to be rebased to remove them since I think dev was force-pushed.~~
Edit: merge from dev should solve this now, since the commits will be squashed anyway.

~~Should we consider using SignatureValue from identity-core to avoid the new type altogether or does the SignatureValue::None variant make it unusable in this context?~~
Edit: SignatureValue was decided to be inappropriate for this.

Edit: missed @PhilippGackstatter previous review, sorry for the duplication.

cycraig · 2022-03-21T15:27:42Z

identity-account-storage/src/types/signature.rs

+/// A digital signature.
+#[derive(Clone, Deserialize, Serialize)]
+pub enum Signature {
+  Ed25519(Vec<u8>),


I think this design is good for forwards-compatibility. Should we mark the enum as non_exhaustive so we can add variants without causing a breaking change?

cycraig · 2022-03-21T15:30:18Z

identity-account-storage/src/types/signature.rs

+  /// Consumes the signature and returns it as bytes
+  pub fn as_bytes(self) -> Vec<u8> {
+    match self {
+      Signature::Ed25519(bytes) => bytes,
+    }


Not too sure about the name, as_* usually doesn't consume self. Maybe into_bytes would be more appropriate? It's unclear whether all variants will be able to satisfy this though...

cycraig

Looks good to me!

cycraig · 2022-03-23T16:40:08Z

bindings/wasm/src/account/types/signature.rs

+  // Returns a copy of the signature as a vec of bytes.
+  #[wasm_bindgen(js_name = asBytes)]
+  pub fn as_bytes(&self) -> Vec<u8> {
+    self.0.as_bytes().to_vec()


Suggested change

// Returns a copy of the signature as a vec of bytes.

#[wasm_bindgen(js_name = asBytes)]

pub fn as_bytes(&self) -> Vec<u8> {

self.0.as_bytes().to_vec()

// Returns a copy of the signature as a `UInt8Array`.

#[wasm_bindgen(js_name = asBytes)]

pub fn as_bytes(&self) -> Vec<u8> {

self.0.as_bytes().to_vec()

cycraig

Oh, maybe change the PR title to fit the changelog better. Something like "Refactor Storage Signature".

Edit: and add the Wasm label, it affects the bindings too.

PhilippGackstatter

Looks good to me, thanks for the changes!

PhilippGackstatter · 2022-03-24T08:59:05Z

bindings/wasm/src/account/types/signature.rs

+  /// Returns a copy of the signature as a `UInt8Array`.
+  #[wasm_bindgen(js_name = asBytes)]
+  pub fn as_bytes(&self) -> Vec<u8> {
+    self.0.as_bytes().to_vec()


Nit: I still think this should be toBytes in JS, mostly due to the Rust naming convention reasoning of "to" representing a conversion. But since I'm not familiar with JS naming conventions in that detail, I'll leave it up to you.

HenriqueNogara added Enhancement New feature or improvement to an existing feature Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Mar 21, 2022

HenriqueNogara marked this pull request as ready for review March 21, 2022 14:47

eike-hass force-pushed the dev branch from 38069e8 to 360cc3f Compare March 21, 2022 14:55

HenriqueNogara requested review from cycraig and PhilippGackstatter March 21, 2022 14:59

PhilippGackstatter suggested changes Mar 21, 2022

View reviewed changes

cycraig suggested changes Mar 21, 2022

View reviewed changes

eike-hass force-pushed the dev branch from bf75d35 to 1271d95 Compare March 21, 2022 20:20

HenriqueNogara force-pushed the chore/refactor-signature branch from 73d29a9 to 297f6e0 Compare March 23, 2022 14:30

Refactor account signature - remove public_key

24de838

HenriqueNogara force-pushed the chore/refactor-signature branch from 297f6e0 to 24de838 Compare March 23, 2022 16:01

HenriqueNogara requested review from PhilippGackstatter and cycraig March 23, 2022 16:11

cycraig approved these changes Mar 23, 2022

View reviewed changes

cycraig reviewed Mar 23, 2022

View reviewed changes

HenriqueNogara changed the title ~~Chore/refactor signature~~ Refactor Storage Signature Mar 23, 2022

HenriqueNogara added the Wasm Related to Wasm bindings. Becomes part of the Wasm changelog label Mar 23, 2022

Solve merge conflicts - docs

9e5107c

PhilippGackstatter approved these changes Mar 24, 2022

View reviewed changes

HenriqueNogara merged commit c484b26 into dev Mar 24, 2022

HenriqueNogara deleted the chore/refactor-signature branch March 24, 2022 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Storage Signature #738

Refactor Storage Signature #738

HenriqueNogara commented Mar 21, 2022 •

edited

Loading

PhilippGackstatter left a comment •

edited

Loading

PhilippGackstatter Mar 21, 2022

PhilippGackstatter Mar 21, 2022

HenriqueNogara Mar 21, 2022

cycraig Mar 21, 2022 •

edited

Loading

PhilippGackstatter Mar 21, 2022

cycraig Mar 21, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

cycraig Mar 21, 2022

cycraig Mar 21, 2022

cycraig left a comment

cycraig Mar 23, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

PhilippGackstatter left a comment

PhilippGackstatter Mar 24, 2022

	let message: Vec<u8> = data.to_jcs()?;
	let signature: Vec<u8> = RemoteSign::sign(&message, remote_key).await?;
	let signature: String = encode_b58(&signature);
	Ok(SignatureValue::Signature(signature))

Refactor Storage Signature #738

Refactor Storage Signature #738

Conversation

HenriqueNogara commented Mar 21, 2022 • edited Loading

Description of change

Links to any relevant issues

Type of change

How the change has been tested

Change checklist

PhilippGackstatter left a comment • edited Loading

Choose a reason for hiding this comment

PhilippGackstatter Mar 21, 2022

Choose a reason for hiding this comment

PhilippGackstatter Mar 21, 2022

Choose a reason for hiding this comment

HenriqueNogara Mar 21, 2022

Choose a reason for hiding this comment

cycraig Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

PhilippGackstatter Mar 21, 2022

Choose a reason for hiding this comment

cycraig Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

cycraig left a comment • edited Loading

Choose a reason for hiding this comment

cycraig Mar 21, 2022

Choose a reason for hiding this comment

cycraig Mar 21, 2022

Choose a reason for hiding this comment

cycraig left a comment

Choose a reason for hiding this comment

cycraig Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

cycraig left a comment • edited Loading

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

PhilippGackstatter Mar 24, 2022

Choose a reason for hiding this comment

HenriqueNogara commented Mar 21, 2022 •

edited

Loading

PhilippGackstatter left a comment •

edited

Loading

cycraig Mar 21, 2022 •

edited

Loading

cycraig Mar 21, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

cycraig Mar 23, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading