fix: rename master secret to link secret #153

berendsliedrecht · 2023-03-20T11:56:20Z

Signed-off-by: blu3beri [email protected]

berendsliedrecht · 2023-03-20T11:59:28Z

src/data_types/cred_request.rs

+    #[serde(alias = "blinded_ms")]
+    pub blinded_ls: ursa::cl::BlindedCredentialSecrets,
+    #[serde(alias = "blinded_ms_correctness_proof")]
+    pub blinded_ls_correctness_proof: ursa::cl::BlindedCredentialSecretsCorrectnessProof,


This adds compatibility for both blinded_ms and blinded_ls if the spec changes this field.

This does however create the output of blinded_ls and not blinded_ms anymore.

Didn't think about the credential request. Not sure if we should change it now, as it's two more fields to update :/ and that we need to support both ways for backwards compatibility

With alias we support both for incoming data models, but we always output with ls.

I mainly changed it because blinded_ms that refers to a link secret feels weird.

Do you think the implications outweigh the naming convention?

Yeah I think so. There's a reason why it wasn't changed in code, but the name was changed to link secret. I just thought it was only an API thing, and forgot about the credential request. I think for the API we can still use link secret, but keep this as ms. Then we link to the spec, which explains master secret was used previously, but it's now link secret?

Oh that's a difficult one, I would really like it to be ls. Changing it later would be a breaking change and it feels like an oversight. If we could do it later without a breaking change, I am 100% to just leave it for now.

Anoncreds v1 is expected to keep compatibility with what has been done, so it's not likely to change this field. At least not in a breaking manner.

I reverted it back to ms as it will likely not change in the spec (sadly :( ).

berendsliedrecht · 2023-03-20T12:00:20Z

src/data_types/link_secret.rs

+
+#[derive(Serialize, Deserialize)]
+pub struct LinkSecret {
+    pub value: MasterSecret,


The structure is still:

{ "value": { "ms": "BIGNUMBER" } }

Is it needed to keep this structure? I didn't see it referenced in the spec and I think it would be a good opportunity to simplify it to avoid confusions (IMO it should be simply a plain string)

It's internal so I don't think it matters. Changing this should be doable with serde remote, but will be done if it's required.

I think it does matter, because when dealing with the library we usually call load or toJson, which in this case is misleading because we don't have any reference regarding how the expected format should be. In mi opinion these methods do make sense for standardized fields (i.e. the ones whose structure is clearly stated in AnonCreds spec).

As a direct user of this library, I can think of treating it as an opaque string and don't care about it's internal structure. However, if I want to use a Link Secret generated by other means or if I want to use one generated here for another purpose I wouldn't expect it to be a JSON object with such particular structure.

It feels more natural and logic to me if this load that receives a string would simply receive the Link Secret value instead of a JSON structure. Likewise, the toJson that outputs a string might become toString.

I think it does matter, because when dealing with the library we usually call load or toJson, which in this case is misleading because we don't have any reference regarding how the expected format should be. In mi opinion these methods do make sense for standardized fields (i.e. the ones whose structure is clearly stated in AnonCreds spec).

As a direct user of this library, I can think of treating it as an opaque string and don't care about it's internal structure. However, if I want to use a Link Secret generated by other means or if I want to use one generated here for another purpose I wouldn't expect it to be a JSON object with such particular structure.

It feels more natural and logic to me if this load that receives a string would simply receive the Link Secret value instead of a JSON structure. Likewise, the toJson that outputs a string might become toString.

I agree with the structure feeling incorrect from a to/from json perspective. But fromJson taking a string as input feels very inconsistent with the API and every other implementation.

I think reworking the structure to this:

{ "link_secret": "BIGNUMBER" }

would be good. I think avoiding the link_secret in the JSON, by using a fromString method, would only cause confusing with regards to the rest of the API.

Can't we just pass the value as string and ditch the whole MasterSecret object?

We could, but we would need some logic in the wrapper which I would like to avoid. I will try to make something that would work for that, but it will take some time.

To me that structure is certainly better (although I'd prefer to keep the value field naming) but still a problem if not documented and, as the rest of the JSON structures used in the library are stated at the spec, it is as inconsistent as my solution for loading from strings.

I still don't see why do we need to input and output a JSON if the Link Secret itself is not a JSON. In the spec is referred to it as a sufficiently random unique identifier, which gives me the idea that it is a string. I would say that is more like a nonce than other objects using in the library.

Actually I don't know why Nonce and MasterSecret were treated differently, since both structures are calling Ursa much in a similar way. If you look at data_types/nonce.rs you'll see that they are doing pretty much the same thing as I'm proposing here about using strings. Ok, I admit it: maybe that's for another PR or further discussion in #138 😄

Since the link secret is not an anoncreds object, I will deal with it like that. Which means basically treating it like a string.

Latest changes now include dealing with the link secret as a sting in decimal layout (like in the datamodels).

src/data_types/cred_request.rs

TimoGlastra · 2023-03-23T16:25:06Z

@blu3beri is this ready-ish to merge or is there any outstanding disucssions here?

berendsliedrecht · 2023-03-23T16:40:20Z

@blu3beri is this ready-ish to merge or is there any outstanding disucssions here?

Code is good, but yarn is acting very weird in the javascript tests, not sure what is going on there.

TimoGlastra

See comment from ariel also

wrappers/python/anoncreds/__init__.py

wrappers/python/demo/test.py

Signed-off-by: blu3beri <[email protected]>

berendsliedrecht force-pushed the link-secret branch 2 times, most recently from 0d7cfa2 to fb91d13 Compare March 20, 2023 11:58

berendsliedrecht commented Mar 20, 2023

View reviewed changes

genaris reviewed Mar 20, 2023

View reviewed changes

src/data_types/cred_request.rs Outdated Show resolved Hide resolved

berendsliedrecht force-pushed the link-secret branch 6 times, most recently from 0936b2c to cfae932 Compare March 22, 2023 09:31

berendsliedrecht force-pushed the link-secret branch from cfae932 to 47fcb22 Compare March 23, 2023 16:43

berendsliedrecht requested a review from TimoGlastra March 23, 2023 21:55

TimoGlastra approved these changes Mar 24, 2023

View reviewed changes

wrappers/python/anoncreds/__init__.py Show resolved Hide resolved

wrappers/python/demo/test.py Outdated Show resolved Hide resolved

berendsliedrecht force-pushed the link-secret branch 2 times, most recently from bb21142 to 5a31014 Compare March 24, 2023 08:57

berendsliedrecht added 2 commits March 24, 2023 11:40

fix: rename master secret to link secret

1f69ab4

Signed-off-by: blu3beri <[email protected]>

fix: rename master secret to link secret

2f8c39e

Signed-off-by: blu3beri <[email protected]>

berendsliedrecht force-pushed the link-secret branch from 1f653fa to 2f8c39e Compare March 24, 2023 10:43

berendsliedrecht merged commit 32009f6 into hyperledger:main Mar 24, 2023

berendsliedrecht deleted the link-secret branch March 24, 2023 11:05

genaris mentioned this pull request Mar 27, 2023

Master Secret / Link Secret structure #138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rename master secret to link secret #153

fix: rename master secret to link secret #153

berendsliedrecht commented Mar 20, 2023

berendsliedrecht Mar 20, 2023

berendsliedrecht Mar 20, 2023

TimoGlastra Mar 20, 2023

berendsliedrecht Mar 20, 2023

TimoGlastra Mar 21, 2023

berendsliedrecht Mar 21, 2023

genaris Mar 21, 2023

berendsliedrecht Mar 21, 2023

berendsliedrecht Mar 20, 2023 •

edited

Loading

genaris Mar 20, 2023

berendsliedrecht Mar 20, 2023

genaris Mar 20, 2023

berendsliedrecht Mar 20, 2023

TimoGlastra Mar 20, 2023

berendsliedrecht Mar 20, 2023

genaris Mar 20, 2023

berendsliedrecht Mar 20, 2023

berendsliedrecht Mar 20, 2023

TimoGlastra commented Mar 23, 2023

berendsliedrecht commented Mar 23, 2023

TimoGlastra left a comment

fix: rename master secret to link secret #153

fix: rename master secret to link secret #153

Conversation

berendsliedrecht commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berendsliedrecht Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented Mar 23, 2023

berendsliedrecht commented Mar 23, 2023

TimoGlastra left a comment

Choose a reason for hiding this comment

berendsliedrecht Mar 20, 2023 •

edited

Loading