Small Status List Updates #95
Conversation
nitro-neal
requested review from
andresuribe87,
mistermoe,
tomdaffurn and
phoebe-lew
as code owners
Codecov Report
@@ Coverage Diff @@
## main #95 +/- ##
==========================================
+ Coverage 69.92% 70.97% +1.05%
==========================================
Files 21 21
Lines 1197 1199 +2
Branches 135 135
==========================================
+ Hits 837 851 +14
+ Misses 292 281 -11
+ Partials 68 67 -1
|
| if (response.status.isSuccess()) { | ||
| val body = response.bodyAsText() | ||
| return VerifiableCredential.parseJwt(body) | ||
| } else { | ||
| throw ClientRequestException(response, "Failed to retrieve VerifiableCredentialType from $url") | ||
| throw ClientRequestException( |
There was a problem hiding this comment.
Throwing inside a try block seems like a code smell to me. Is that intentional?
| @@ -361,7 +395,7 @@ class StatusListCredentialTest { | |||
| } | |||
|
|
|||
| @Test | |||
| fun `should asynchronously validate if a credential is in the status list using a mock HTTP client`() = runBlocking { | |||
| fun `should asynchronously validate if a credential is in the status list using a mock HTTP client`() { | |||
There was a problem hiding this comment.
can you explain the "asynchronously" here @nitro-neal ?
There was a problem hiding this comment.
has to make an http GET to download the status list credential (vs passing in the status list credential object)
There was a problem hiding this comment.
I was also confused by this. Asynchronously, to me, means that you would need to explicitly wait until the http GET downloads the credential. That doesn't seem to be what the test is doing. Furthermore, that implementation has a runBlocking bit, which actually makes the async call be blocking.
My suggestion is to rename to should validate if a credential is in the status list using a mock HTTP client
| } | ||
|
|
||
| require(response.status.isSuccess()) { | ||
| "Failed to retrieve VerifiableCredentialType from $url with status ${response.status}" |
There was a problem hiding this comment.
This throws an IllegalArgumentException. Is that what you intended?
| "Failed to retrieve VerifiableCredentialType from $url with status ${response.status}" | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
I wonder if this try/catch block could be removed to simplify the code.
|
|
||
| if (!duplicateSet.add(statusListEntry.statusListIndex)) { | ||
| throw IllegalArgumentException("duplicate entry found with index: ${statusListEntry.statusListIndex}") | ||
| throw IllegalArgumentException("Duplicate entry found with index: ${statusListEntry.statusListIndex}") |
There was a problem hiding this comment.
| throw IllegalArgumentException("Duplicate entry found with index: ${statusListEntry.statusListIndex}") | |
| "Duplicate entry found with index: ${statusListEntry.statusListIndex}" |
| @@ -361,7 +395,7 @@ class StatusListCredentialTest { | |||
| } | |||
|
|
|||
| @Test | |||
| fun `should asynchronously validate if a credential is in the status list using a mock HTTP client`() = runBlocking { | |||
| fun `should asynchronously validate if a credential is in the status list using a mock HTTP client`() { | |||
There was a problem hiding this comment.
I was also confused by this. Asynchronously, to me, means that you would need to explicitly wait until the http GET downloads the credential. That doesn't seem to be what the test is doing. Furthermore, that implementation has a runBlocking bit, which actually makes the async call be blocking.
My suggestion is to rename to should validate if a credential is in the status list using a mock HTTP client
decentralgabe
requested review from
diehuxx,
kirahsapong,
jiyoontbd and
frankhinek
as code owners
Small updates to cred status list impl