cred status list impl #320
Conversation
nitro-neal
requested review from
KendallWeihe,
kirahsapong,
shamilovtim,
diehuxx,
decentralgabe,
frankhinek,
leordev and
ALRubinger
as code owners
| }) | ||
| } | ||
|
|
||
| /// Checks if a given credential is disabled according to this Status List Credential. |
| /// | ||
| /// # Returns | ||
| /// `true` if the bit at the specified index is 1, `false` if it is 0. | ||
| fn get_bit(compressed_bitstring: &str, bit_index: usize) -> Result<bool> { |
There was a problem hiding this comment.
This looks like it's not StatusListCredential-specific, no? Might be better to put this in a separate helper file and move associated tests as well
There was a problem hiding this comment.
I think in reality it will only be used for status list credential, its getting the bit from a compressed gziped base64 string, which I think nothing else in the universe uses 😆
There was a problem hiding this comment.
since it doesn't mention StatusListCredential, still seems worth putting in a separate module. Could be in a module within mod status_list_credential if you like. Putting it in the impl StatusListCredential disrupts the focus.
| } | ||
|
|
||
| // Check if the status purpose matches | ||
| let status_purpose = self |
There was a problem hiding this comment.
for both status_purpose and encoded_list we've got these similar, super vertical blocks of code that do "get this string attribute from additional_properties". I think it'd be more readable and reusable to put logic in a private function that doesn't use as many and_thens.
There was a problem hiding this comment.
yup you are right, let me do this
| })?; | ||
|
|
||
| if status_list_entry.status_purpose != *status_purpose { | ||
| return Err(Web5Error::Parameter("status purpose mismatch".to_string())); |
There was a problem hiding this comment.
nit: do you mind adding the expected and actual status purposes to this error message?
Co-authored-by: Diane Huxley <[email protected]>
| /// | ||
| /// # Returns | ||
| /// A `Result` containing the compressed, base64-encoded bitstring, or an error if an index is out of range. | ||
| fn bitstring_generation(status_list_indexes: Vec<usize>) -> Result<String> { |
There was a problem hiding this comment.
nit: don't need to take ownership. might avoid unnecessary cloning
| fn bitstring_generation(status_list_indexes: Vec<usize>) -> Result<String> { | |
| fn bitstring_generation(status_list_indexes: &[usize]) -> Result<String> { |
| let additional_properties = { | ||
| let mut properties = HashMap::new(); | ||
| properties.insert( | ||
| "statusPurpose".to_string(), | ||
| JsonValue::String(status_purpose.clone()), | ||
| ); | ||
| properties.insert( | ||
| "type".to_string(), | ||
| JsonValue::String(STATUS_LIST_2021.to_string()), | ||
| ); | ||
| properties.insert( | ||
| "encodedList".to_string(), | ||
| JsonValue::String(base64_bitstring.clone()), | ||
| ); | ||
| JsonObject { properties } | ||
| }; |
There was a problem hiding this comment.
Easier to see that we're just initting a hashmap with a few values. You can also use HashMap::from() if you prefer, instead of into_iter().collect().
| let additional_properties = { | |
| let mut properties = HashMap::new(); | |
| properties.insert( | |
| "statusPurpose".to_string(), | |
| JsonValue::String(status_purpose.clone()), | |
| ); | |
| properties.insert( | |
| "type".to_string(), | |
| JsonValue::String(STATUS_LIST_2021.to_string()), | |
| ); | |
| properties.insert( | |
| "encodedList".to_string(), | |
| JsonValue::String(base64_bitstring.clone()), | |
| ); | |
| JsonObject { properties } | |
| }; | |
| let additional_properties = JsonObject { | |
| properties: [ | |
| ("statusPurpose".to_string(), JsonValue::String(status_purpose)), | |
| ("type".to_string(), JsonValue::String(STATUS_LIST_2021.to_string())), | |
| ("encodedList".to_string(), JsonValue::String(base64_bitstring)), | |
| ].into_iter().collect(), | |
| }; |
| issuance_date: None, | ||
| expiration_date: None, | ||
| credential_status: None, | ||
| credential_schema: None, | ||
| evidence: None, |
There was a problem hiding this comment.
| issuance_date: None, | |
| expiration_date: None, | |
| credential_status: None, | |
| credential_schema: None, | |
| evidence: None, | |
| ..Default::default() |
| }; | ||
|
|
||
| let vc_options = VerifiableCredentialCreateOptions { | ||
| id: Some(format!("urn:uuid:{}", uuid::Uuid::new_v4())), |
There was a problem hiding this comment.
you don't necessarily have to pass this in
There was a problem hiding this comment.
I need to actually update ids and introduce a status list credential options (or something like this) for custom ids. Still hashing that out, but will add in next pr
No description provided.