algorand / java-algorand-sdk

Algorand SDK for Java7+ to interact with the Algorand network
https://algorand.github.io/java-algorand-sdk/
MIT License
68 stars 69 forks source link

Move to Java 8 #186

Open winder opened 4 years ago

winder commented 4 years ago

We should deprecate support for Java 7 and move to Java 8.

Java 7 has almost no market share these days, especially in the security conscious crypto space.

If that isn't enough, there are a handful of Android security fixes in Java 8, so requiring that version could protect algorand users.

algorotem commented 4 years ago

Needed to check if it would break any mobile app. @Priegle Any insight here?

srayhunter commented 3 years ago

@winder is anyone working on this item currently?

winder commented 3 years ago

@srayhunter no. Is there a specific reason you would like to see us remove Java7 support?

richdrich commented 3 years ago

Just FYI: I've found an issue when you target minSdkVersion = 24 and run on Android 7.

I'm seeing:

com.fasterxml.jackson.databind.JsonMappingException: No virtual method decode(Ljava/lang/String;)[B in class Lorg/apache/commons/codec/binary/Base64; or its super classes (declaration of 'org.apache.commons.codec.binary.Base64' appears in /system/framework/org.apache.http.legacy.boot.jar)
  at [Source: [B@d361c9b; line: 1, column: 340] (through reference chain: com.algorand.algosdk.v2.client.model.TransactionsResponse["transactions"]->java.util.ArrayList[0]->com.algorand.algosdk.v2.client.model.Transaction["genesis-hash"])

which stems from the org.apache.commons.codec.binary.Base64 picking up a broken BaseNCodec from the bootpath.

Using Java 8 Base64 would fix this.

(I cant find a non hacky way to stop my app loading the broken commons code)

timmolter commented 3 years ago

Please keep it Java 7 compatible. We still need this to run on ancient android 4 runtimes.

timmolter commented 3 years ago

I suggest we update to use the BouncyCastle Base64 class, since it's already on this projects classpath. It will also solve another issue with namespace clashing I'm having on Android 4.

richdrich commented 3 years ago

If this works in the old BouncyCastle that's bundled in old Android? Or, just add a local base64 routine that avoids any library issues - should not be needed I know :-)

timmolter commented 3 years ago

Here's the specific issue I am having: https://github.com/algorand/java-algorand-sdk/issues/271

My option #2 to fix it, is exactly what you suggested.

timmolter commented 3 years ago

For BouncyCastle you can force the usage of the compile dependency using

            Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME);
            Security.insertProviderAt(new BouncyCastleProvider(), 0);

which guarantees ( I think) it uses the one you want and not the old BouncyCastle that's bundled in old Android. This isn't an issue with Java 8 though because you can force the update during runtime.

However there are OTHER issues that I've run into that updating to Java 8 can cause on Android 4, which there are only painful fixes for:

to name a couple. Once you allow Java8, people start using this stuff and then you get crashes usually in production because of some obscure phone that you don't have in hand to test with.

I'm not trying to start an argument, just providing a counter argument. Too bad we just can't update all the phone out there!

richdrich commented 3 years ago

I think that changing the provider makes it use the correct provider for javax.security (JCE) APIs but not where the bouncycastle dependency is imported directly. But I may be wrong.

timmolter commented 3 years ago

Yeah, maybe you're right about that. I'm still learning about this specific issue.... ;)