Critical code review PR - Last sprint handover checklist of 0.7.0 release#110
Critical code review PR - Last sprint handover checklist of 0.7.0 release#110Prafulrakhade merged 0 commit intomasterfrom
Conversation
| else -> {} | ||
| } | ||
|
|
||
| return Base64.decode(base64, Base64.DEFAULT) |
There was a problem hiding this comment.
Can we not simply do "return Base64.decode(input, Base64.DEFAULT or Base64.URL_SAFE)" ?
|
|
||
| }catch (e: Exception){ | ||
| Log.e("PixelPass",e.toString()) | ||
| logger.severe("Error occurred while converting Qr Data to Base64 String::$e") |
There was a problem hiding this comment.
we are catching a generic exception class, how we can give a specific error message here ?
| return toBitmap(qrcode) | ||
| fun generateQRCode(data: String, ecc: ECC = ECC.L, header: String = ""): String { | ||
| val dataWithHeader = generateQRData(data, header) | ||
| val qrcodeImage = convertQrDataIntoBase64(dataWithHeader, ecc) |
There was a problem hiding this comment.
Can we change the method name as per proper camel convention ? like convertQRDataIntoBase64
| toolVersion = "0.8.11" | ||
| reportsDirectory = layout.buildDirectory.dir("reports/jacoco") | ||
| } | ||
|
|
There was a problem hiding this comment.
why is kotlin/PixelPass/libs/cose-java-1.1.0.jar commit to the repo and not taken as a maven dependency ?
| fun encodeToString(image: Bitmap): String? { | ||
| val imageString: String? | ||
| val bos = ByteArrayOutputStream() | ||
| image.compress(Bitmap.CompressFormat.PNG, 100, bos) |
There was a problem hiding this comment.
why is 100 hardcode here, should this be a setting ?
| return toBitmap(qrcode) | ||
| fun generateQRCode(data: String, ecc: ECC = ECC.L, header: String = ""): String { | ||
| val dataWithHeader = generateQRData(data, header) | ||
| val qrcodeImage = convertQrDataIntoBase64(dataWithHeader, ecc) |
There was a problem hiding this comment.
Any specific reason this is base64 encoded and not URL safe base 64 encoded ?
|
|
||
| private fun verifyAndDecode(rawCbor: ByteArray, oneKey: OneKey, mapper: Map<String, String>): String { | ||
| try { | ||
| val ctx: CwtCryptoCtx = CwtCryptoCtx.sign1Verify(oneKey.PublicKey()) |
There was a problem hiding this comment.
why in philsys QR code alone we do the verification also here ? Is that not the responsibility of vc-verifier ?
| val dataWithHeader = generateQRData(data, header).toByteArray() | ||
| val qrcode = QrCode.encodeText(String(dataWithHeader), ecc.mEcc) | ||
| return toBitmap(qrcode) | ||
| fun generateQRCode(data: String, ecc: ECC = ECC.L, header: String = ""): String { |
There was a problem hiding this comment.
Is readme file updated as per the new interface returns ?
| } | ||
|
|
||
|
|
||
| fun decodeCWT( |
There was a problem hiding this comment.
readme on this new method missing
| package io.mosip.pixelpass.common | ||
|
|
||
|
|
||
| expect fun decodeFromBase64UrlFormatEncoded(content: String): ByteArray |
There was a problem hiding this comment.
we can rename this to decodeFromBase64UrlFormat
2 participants
This PR is created just to add the review comments as part of the critical code review task of last sprint handover checklist for release of 0.7.0 version.
** THIS PR SHOULD NOT BE MERGED **