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

Add support for StatusList2021 #1273

Merged
merged 40 commits into from
Jan 24, 2024
Merged

Add support for StatusList2021 #1273

merged 40 commits into from
Jan 24, 2024

Conversation

UMR1352
Copy link
Contributor

@UMR1352 UMR1352 commented Jan 8, 2024

Description of change

Implementation of StatusList2021

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

Unit and integration tests

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

@UMR1352 UMR1352 added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Jan 16, 2024
@UMR1352 UMR1352 added Documentation Improvements or additions to documentation. Excludes from changelog Added A new feature that requires a minor release. Part of "Added" section in changelog labels Jan 16, 2024
@UMR1352 UMR1352 marked this pull request as ready for review January 16, 2024 10:17
@UMR1352 UMR1352 requested a review from abdulmth January 16, 2024 10:19
Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

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

It looks great, thanks for working on it, some points that I noticed. This is not the full review, I will take a deeper look later.

identity_credential/Cargo.toml Outdated Show resolved Hide resolved
identity_credential/src/credential/status.rs Outdated Show resolved Hide resolved
examples/1_advanced/7_status_list_2021.rs Outdated Show resolved Hide resolved
examples/1_advanced/7_status_list_2021.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

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

Some more comments, I also would suggest moving the files of the old bitmap revocation method to a new directory under revocation and feature gate the status list into its own feature since it's not the same as bitmap, but it's under its feature.

@abdulmth
Copy link
Contributor

Also, please change the test number since the SD-JWT exampled is merged now, also Add the tests to the README files inside the examples directories, also to the node and cypress tests so they run for CI. Check the sd-jwt PR to see where they should be added.

@UMR1352 UMR1352 requested a review from a team as a code owner January 19, 2024 10:16
Copy link
Collaborator

@eike-hass eike-hass left a comment

Choose a reason for hiding this comment

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

Overall looking very good 💪 Left a view comments. Not everything needs to be addressed, but would like to have discussion on those topics

Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

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

The improvements from the last review look great, thanks for working on it! I have some more comments, mainly on the rust example, whatever suggestions you decide to take should be also changed in the Wasm example. Also, some Wasm issues that need to be addressed, since wasm bindgen is weird sometimes.

examples/1_advanced/8_status_list_2021.rs Outdated Show resolved Hide resolved
examples/1_advanced/8_status_list_2021.rs Outdated Show resolved Hide resolved
examples/1_advanced/8_status_list_2021.rs Show resolved Hide resolved
examples/1_advanced/8_status_list_2021.rs Show resolved Hide resolved
examples/1_advanced/8_status_list_2021.rs Show resolved Hide resolved
Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

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

Last minor comments

serde_json::from_str::<StatusList2021Credential>(status_list_credential_json.as_str()).unwrap();

// Set the value of the chosen index entry to true to revoke the credential
status_list_credential.set_credential_status(&mut credential, credential_index, true)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could've combined them to RevokedOrSuspended since that represents the 1 value better and the purpose is not relevant anyways. I added a suggestion to change the value parameter to something more descriptive if you want to keep it the way it is.

@UMR1352 UMR1352 merged commit 8bc3ab7 into main Jan 24, 2024
11 checks passed
@eike-hass eike-hass deleted the feat/status-list-2021 branch January 24, 2024 16:02
@eike-hass eike-hass linked an issue Jan 26, 2024 that may be closed by this pull request
8 tasks
@UMR1352 UMR1352 mentioned this pull request Jan 29, 2024
8 tasks
@eike-hass eike-hass changed the title Feat/status list 2021 Add support for StatusList2021 Feb 1, 2024
@eike-hass eike-hass removed the Documentation Improvements or additions to documentation. Excludes from changelog label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Implement Status List 2021
3 participants