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

Revocation registry cleanups #233

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

andrewwhitehead
Copy link
Member

Updates to anoncreds-clsignatures 0.2.1 and simplifies revocation registry handling.

Signed-off-by: Andrew Whitehead <[email protected]>
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

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

I think RevocationStatusList de/serialization could be more straightforward.

#[serde(
rename = "currentAccumulator",
skip_serializing_if = "Option::is_none",
with = "serde_opt_accumulator"
Copy link
Contributor

@bobozaur bobozaur Aug 17, 2023

Choose a reason for hiding this comment

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

This seems like an overkill. Serialization doesn't seem to require anything custom, so the attribute could instead be deserialize_with. But then the custom deserialization seems to only want to achieve having multiple names for the field, which can also be done with #[serde(alias = "currentAccumulator"].

Depending on which should be the serialization name for the field, you might want to use #[serde(rename = "currentAccumulator", alias = "accum"] instead.

Copy link
Contributor

@berendsliedrecht berendsliedrecht Aug 17, 2023

Choose a reason for hiding this comment

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

The test json_rev_list_can_be_deserialized might also need to be expanded to make sure that the test vector, which includes currentAccumulator checks that des.accum.is_some(). as skip_serializing_if="Option::is_none" is set, it will silently fail if it can not interpret currentAccumulator correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't trying to change the current deserialization, but it's pretty strange. It accepts values like:

{ ...
  "currentAccumulator": {
    "currentAccumulator": "1 000..."
  }
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because the visitor implements the visit_map method.

Copy link
Member Author

Choose a reason for hiding this comment

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

So do we think that's a mistake and it's just meant to support both currentAccumulator and accum as the field name? In that case it wouldn't need any custom deserialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fairly sure that it is a mistake. It was a custom implementation because we could not change it as it was a private field within Ursa (it has been a while so there might be more to it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... The changes are introduced in this PR, so I was honestly expecting you to know the answer to that 😅 . To me it just seemed weird so I pointed it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved the deserialization code around and changed it to an Accumulator value, the old implementation had the visit_map method.

Copy link
Member Author

@andrewwhitehead andrewwhitehead Aug 17, 2023

Choose a reason for hiding this comment

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

Updated to just deserialize from a string, which is the format the tests use. I'm a little surprised the bitvec serialization is a list instead of hex bytes or something.

Signed-off-by: Andrew Whitehead <[email protected]>
@andrewwhitehead andrewwhitehead merged commit ddcc9be into hyperledger:main Aug 18, 2023
25 checks passed
@andrewwhitehead andrewwhitehead deleted the upd/ac-021 branch August 18, 2023 19:22
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.

3 participants