Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Credential Manager implementation by modularizing digital credential dependencies and introducing support for credential issuance registration and password-based sign-in. The review feedback identifies a compilation error due to a missing parenthesis in the issuance registration logic, recommends replacing unsafe non-null assertions with safe calls to prevent potential crashes, and suggests removing redundant object wrapping and unnecessary coroutine scopes to improve code clarity and efficiency.
| // [START android_identity_handle_issuance_create_option_selected] | ||
| val pendingIntentRequest = PendingIntentHandler.retrieveProviderCreateCredentialRequest(intent) | ||
| val request = pendingIntentRequest!!.callingRequest | ||
| if (request is CreateDigitalCredentialRequest) { |
There was a problem hiding this comment.
The use of the non-null assertion operator (!!) is unsafe here. If retrieveProviderCreateCredentialRequest returns null, the application will crash. Using a safe call (?.) is recommended to prevent potential NullPointerExceptions.
| if (request is CreateDigitalCredentialRequest) { | |
| val request = pendingIntentRequest?.callingRequest |
| resultData, | ||
| CreateDigitalCredentialResponse(response.responseJson) | ||
| ) | ||
| setResult(RESULT_OK, resultData) |
There was a problem hiding this comment.
The response parameter is already an instance of CreateDigitalCredentialResponse. Re-wrapping it in a new CreateDigitalCredentialResponse object is redundant and inefficient.
| resultData, | |
| CreateDigitalCredentialResponse(response.responseJson) | |
| ) | |
| setResult(RESULT_OK, resultData) | |
| PendingIntentHandler.setCreateCredentialResponse( | |
| resultData, | |
| response | |
| ) |
| coroutineScope { | ||
| try { | ||
| val result = credentialManager.getCredential( | ||
| context = activityContext, | ||
| request = credentialRequest | ||
| ) | ||
| handlePasswordSignIn(result) | ||
| } catch (e: GetCredentialException) { | ||
| // Handle failure | ||
| } | ||
| } |
There was a problem hiding this comment.
The coroutineScope builder is redundant here as there is only a single block of suspending code and no concurrent operations are being launched. Removing it simplifies the code without changing its behavior.
try {
val result = credentialManager.getCredential(
context = activityContext,
request = credentialRequest
)
handlePasswordSignIn(result)
} catch (e: GetCredentialException) {
// Handle failure
}
No description provided.