-
Notifications
You must be signed in to change notification settings - Fork 90
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
Conversation
There was a problem hiding this 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/src/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
identity_credential/src/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
bindings/wasm/src/credential/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this 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
bindings/wasm/src/credential/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eike Haß <[email protected]>
There was a problem hiding this 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.
bindings/wasm/src/credential/revocation/status_list_2021/entry.rs
Outdated
Show resolved
Hide resolved
bindings/wasm/src/credential/revocation/status_list_2021/status_list.rs
Outdated
Show resolved
Hide resolved
bindings/wasm/src/credential/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last minor comments
identity_credential/src/revocation/status_list_2021/credential.rs
Outdated
Show resolved
Hide resolved
bindings/wasm/src/credential/revocation/status_list_2021/entry.rs
Outdated
Show resolved
Hide resolved
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)?; |
There was a problem hiding this comment.
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.
Description of change
Implementation of StatusList2021
Type of change
How the change has been tested
Unit and integration tests
Change checklist