issue #1098 encrypt and decrypt private keys with PGPainless#1103
issue #1098 encrypt and decrypt private keys with PGPainless#1103
Conversation
tomholub
left a comment
There was a problem hiding this comment.
Looks good, adding a code clarity request.
Not sure about tests, our Android tests can be quite brittle when run in CI, so it's not clear if it passes or not sometimes until Den runs them locally. Hopefully over time it can be improved.
| val clipboardText = msg.obj as String | ||
| try { | ||
| val nodeKeyDetails = NodeCallsExecutor.parseKeys(clipboardText) | ||
| val nodeKeyDetails = PgpKey.parseKeys(clipboardText.toByteArray()).second |
There was a problem hiding this comment.
These .first and .second are not obvious enough - we should find a way to refer to them in a more obvious way
There was a problem hiding this comment.
I've tried to not introduce extra classes and just use standard Pair class. This can be improved by adding a new data class with more readable field names.
There was a problem hiding this comment.
Alternatively we can completely drop returning format indication (I just tried to mimic what Typescrpt does) and return just key list, as NodeCallsExecutor.parseKeys() did.
There was a problem hiding this comment.
I don't see actually that such indication is used anywhere - so maybe just drop it is the best way to improve.
There was a problem hiding this comment.
Looks good, adding a code clarity request.
Not sure about tests, our Android tests can be quite brittle when run in CI, so it's not clear if it passes or not sometimes until Den runs them locally. Hopefully over time it can be improved.
Yes, Android tests constantly fail here or there. Can we do something to improve that? Last time I've increased time limit and they've passed - maybe another increase needed?
There was a problem hiding this comment.
Data class is much, much better here. PgpKey.parseKeys(clipboardText.toByteArray()).isArmored and PgpKey.parseKeys(clipboardText.toByteArray()).keys is much better than .first and .second from code clarity perspective. The added data class is always worth it.
There was a problem hiding this comment.
@tomholub @IvanPizhenko JUnit tests were broken. That's why tests were not passed.
I will be able to look at them in the morning.
| var passphrase: String?, | ||
| var errorMsg: String?) : Parcelable { | ||
| var errorMsg: String?, | ||
| val keyRing: PGPKeyRing? = null) : Parcelable { |
There was a problem hiding this comment.
@IvanPizhenko Please note if you change a constructor of data class that implements Parcelable you should modify the implementation of Parcelable methods. Otherwise, you will receive a runtime error. It's Android thing. I hope in the future after #1034 we will have not to do that.
There was a problem hiding this comment.
@IvanPizhenko Please don't try to fix that for now. It seems I need to make some code changes. Too many Android things.
There was a problem hiding this comment.
well, I didn't try to fix anything. Also I didn't know that, I'm generally not developing for Android.
| .done() | ||
| .armorWithFlowcryptHeaders() | ||
| } catch (ex: Exception) { | ||
| "" |
There was a problem hiding this comment.
@IvanPizhenko Please tell me is there any reason to don't print the stacktrace of an exception?
There was a problem hiding this comment.
@DenBond7 This was done to mimic what the Typescript does - it returns empty string on failure. Of course, we can output the exception. But I am just not sure what is the right way to do it on the Android? Just println()? Some logger? Please advice.
There was a problem hiding this comment.
Looked around - looks like just e.printStackTrace() would work good - will add
There was a problem hiding this comment.
Yes, e.printStackTrace() - it will be enough. Anyway, don't need to drop using e.printStackTrace() over the whole Java(Kotlin) code if you don't have enough reason to do that. Usually, it helps to debug errors.
DenBond7
left a comment
There was a problem hiding this comment.
Approved after some code changes
…ckup() from the CI tests.| #1098
Yes, if we are talking about Android Instrumentation tests ( |
close #1098