Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port the latest web3j release (on master) to Java 1.6 in preparation for an Android release #769

Closed
snazha-blkio opened this issue Nov 5, 2018 · 41 comments
Assignees
Labels
Milestone

Comments

@snazha-blkio
Copy link
Contributor

Background

Provide a port of the web3j 4.0 code to Java 1.6 in order to support an Android release.
The last web3j release for android was version 3.3.1 (refer to the "master-android" branch), a new release for Android needs to be cut.

Objective

To port the Java 1.7 and 1.8 portions of the web3j codebase to 1.6 equivalent constructs

Requirements

  • Port the code on "master" to be Java 1.6 compatible
  • Ensure all existing tests run without error

Technical details

  • The last android release was version 3.3.1 and was cut from the "master-android" branch
  • The current production release is 4.0.0, and is available on the "master" branch
  • An attempt has been made to do this port, this can be found on "release/4.0-android"
  • Most of the remaining work revolves around the usage of Java 1.8's CompletableFuture

Next steps

There are two options.

  1. Option 1: Check out "master" and port all Java 1.7 and 1.8 code to 1.6 yourself
  2. Option 2: Check out "release/4.0-android", which contains a (compiling) attempt to port master to Java 1.6, and port the remaining classes

Should you choose the second path ...

  • release/4.0-android contains code with the straighforward conversions performed, such as streams, optionals, etc
  • we have provided a stubbed CompletableFuture at: org.web3j.backport.concurrent.CompletableFuture; you need to refactor all uses of this stub class and provide Java 1.6-compatable equivalents (such as Futures or other)
  • after you address the few classes that use CompletableFuture you can delete this implementation
  • you can refactor the method signatures/return types (where needed)
@MarkOSullivan94
Copy link
Contributor

Out of interest why does the Android code need to be using Java 1.6?

From the Android documentation.

Android Studio 3.0 and later supports all Java 7 language features and a subset of Java 8 language features that vary by platform version.

https://developer.android.com/studio/write/java8-support

@snazha-blkio
Copy link
Contributor Author

Hi @MarkOSullivan94

We have a significant number of developers that are targeting older versions of Android in their applications, so we'd like to keep them in the web3j builds and offerings.

@MarkOSullivan94
Copy link
Contributor

@snazha-blkio from what I've read it seems that AS 3.0+ will support Java 1.7 for all Android versions.

I know in the latest stable build for AS there is good support for Java 1.8 as well but it would be worthwhile having this tested to see if it's viable.

It just seems like you're giving yourself more work than necessary when you could just support Java 1.7 and let all the developers upgrade to the latest stable version of AS.

I would like to think that reducing the effort required to convert to Java 1.6 would result in easier and faster releases for Web3j for Android.

@snazha-blkio
Copy link
Contributor Author

Valid point @MarkOSullivan94 re: AS 3.0. I will take a look at AS 3.0 as it has good support for some Java 8 classes that are used profusely (e.g. CompletableFuture). I'll post my findings thereafter.

Thanks for the feedback!

@serso
Copy link
Contributor

serso commented Nov 14, 2018

As @MarkOSullivan94 already mentioned, "desugar" procedure, built-in Android's build pipeline, handles some Java 8 language features, such as lambda expressions, method references, default and static interface methods and repeating annotations. It produces Java 6 compatible bytecode from Java 8 code.

However, some Java 8 APIs are not supported on older phones, such as java.util.stream, java.util.function, etc, which means that these APIs can't be used in Web3J if it has to be compatible with Android.

My suggestion: use Java 8 language features but replace incompatible APIs. A good library that can help with that is Guava.

Note also that Java 8 compatibility is not the only one problem on Android. Other issues are:

  • Crippled Bouncy Castle on Android
  • Unsupported XML APIs
  • ...

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to it as part of the Ethereum Foundation fund.

@FrederikBolding
Copy link

I've applied, I wasn't sure if it was allowed to use a library to solve the issue, please let me know @snazha-blkio

@snazha-blkio
Copy link
Contributor Author

snazha-blkio commented Nov 17, 2018

I've applied, I wasn't sure if it was allowed to use a library to solve the issue, please let me know @snazha-blkio

Our concerns around using a library are around licensing and ensuring full android compatibility. Thus, we'd rather no library be used.

@snazha-blkio snazha-blkio removed awaiting-user-input Require more info or input from user needs-ci-approvals labels Nov 17, 2018
@FrederikBolding
Copy link

@snazha-blkio Then I don't think I'm the man for the job - Sorry 😄

@serso
Copy link
Contributor

serso commented Nov 17, 2018

@snazha-blkio, I was interested in this task and even started experimenting with the code. It seems to me that the simplest way to make Web3j Android compatible is, as mentioned before, to port only missing APIs (method, classes and packages) leaving lambdas and other Java 8 features intact.
To ensure that only supported APIs are used in the library, it can be built against Android version of JDK. In this case, the biggest issue would probably be porting "function" and "stream" APIs which can be done with the help of streamsupport library (GPL v2). Do you think it's something you'd accept? Should I continue digging in this direction?

@FrederikBolding
Copy link

@serso That is exactly what I experimented with as well..

@serso
Copy link
Contributor

serso commented Nov 19, 2018

Code in the task branch is now binary compatible with Android JDK. Streamsupport library is used only in "core" module.

@frankchen07
Copy link

hey @serso, Frank from Gitcoin here, could you please hit "start work" here: https://gitcoin.co/issue/web3j/web3j/769/1777 so @ceresstation can approve you for work (since you've started already?) :)

@serso
Copy link
Contributor

serso commented Nov 21, 2018

@frankchen07 done :)

@conor10
Copy link
Contributor

conor10 commented Nov 23, 2018

Hi, sorry just catching up on this. web3j needs to use Apache 2 friendly libraries. Pulling in GPLv2 is problematic for this reason.

@serso
Copy link
Contributor

serso commented Nov 23, 2018

@conor10 please read #769 (comment)
They don't use GPLv2 but GPLv2 with the classpath exception which makes a huge difference.

@antonydenyer
Copy link
Member

@serso I think you're correct with regards to licensing. Can we proceed on that assumption and get everything else ready?

I think we just need to double check with the lawyers before we hit merge.

@serso
Copy link
Contributor

serso commented Nov 23, 2018

@antonydenyer yes, we can proceed.

Current status: https://github.com/serso/web3j/commits/my-android

The only remaining thing left to do in the code is to abstract crypto provider API. The bundled version of Bouncy Castle in Android is crippled and doesn't contain ciphers used by web3j. It has to be substituted by Spongy Castle in Android. I patched it myself here but we need a more maintainable solution than that.

My plan is to create a submodule (or a separate project) with a sample Android app to verify that the library works as expected. I would like to add a set of Proguard rules as well. Proguard is often used in Android to obfuscate and minimize the code. I know that web3j has several issues with it.

I also can take a look into removing streamsupport and streamsupport-cfuture dependencies if needed. Removal of the former should be trivial but might take some time. Removal of the latter can be tricky but might be solved by substituting CompletableFuture with some other [Java 6] Future class. I think it should be possible to port CompletableFuture but it might take a lot of time.
@snazha-blkio @conor10 what do you think?

@serso
Copy link
Contributor

serso commented Nov 23, 2018

@snazha-blkio @conor10 note also that in my WIP branch I didn't convert Java 8 language features supported on Android, such as lambdas. Please confirm that this is okay (the task description says "Port everything to Java 1.6").

@antonydenyer
Copy link
Member

I would like to add a set of Proguard rules as well. Proguard is often used in Android to obfuscate and minimize the code. I know that web3j has several issues with it.

I think if developers want to do this when deploying their applications that's their business, not ours.

@serso
Copy link
Contributor

serso commented Nov 23, 2018

I think if developers want to do this when deploying their applications that's their business, not ours.

On the other hand it might be difficult to write Proguard rules if you are not familiar with web3j.

@gitcoinbot
Copy link

@serso Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@serso
Copy link
Contributor

serso commented Nov 29, 2018

Status update: I've created a test Android app with the binaries built from the tip of the task branch. The app seems to work fine on Pixel and Moto G.
The app generates or loads a previously generated wallet, displays its mnemonic and address and looks up its balance (on Rinkeby).

It seems that there are no issues neither with XML APIs nor with Bouncy Castle (BC) mentioned above. BC issue seems to be resolved due to this.
Note that on Android you might need to prepare the BC as described here.

I haven't tried signing yet but the changes seem to be promising. Implementing signing in the test app is the next step for me.

@snazha-blkio @conor10 can you please follow up on this and this?

@iikirilov
Copy link
Contributor

@antonydenyer yes, we can proceed.

Current status: https://github.com/serso/web3j/commits/my-android

The only remaining thing left to do in the code is to abstract crypto provider API. The bundled version of Bouncy Castle in Android is crippled and doesn't contain ciphers used by web3j. It has to be substituted by Spongy Castle in Android. I patched it myself here but we need a more maintainable solution than that.

You have answered your own question stick with bc

My plan is to create a submodule (or a separate project) with a sample Android app to verify that the library works as expected. I would like to add a set of Proguard rules as well. Proguard is often used in Android to obfuscate and minimize the code. I know that web3j has several issues with it.

👍

I also can take a look into removing streamsupport and streamsupport-cfuture dependencies if needed. Removal of the former should be trivial but might take some time. Removal of the latter can be tricky but might be solved by substituting CompletableFuture with some other [Java 6] Future class. I think it should be possible to port CompletableFuture but it might take a lot of time.
@snazha-blkio @conor10 what do you think?

@conor @snazha please confirm that the license of streamsupport is compatible. It would be nice to have this on the android branch to make porting less of a burden in the future

@snazha-blkio @conor10 note also that in my WIP branch I didn't convert Java 8 language features supported on Android, such as lambdas. Please confirm that this is okay (the task description says "Port everything to Java 1.6").

I have no idea on this...

@serso
Copy link
Contributor

serso commented Nov 29, 2018

It would be nice to have this on the android branch to make porting less of a burden in the future

My goal was to force devs to write Android compatible code from the beginning. If this PR lands on master only features supported on Android will be available for development. Of course, It's not a problem to port these changes into the release branch and prepare a "4.0.0-android" package for the corresponding "4.0.0" release.

I think it's better if we avoid releasing Android specific packages in the future by making the main package Android compatible. And this PR ensures that.

@serso
Copy link
Contributor

serso commented Dec 1, 2018

I'm done with testing. Everything seems work fine on Android API 21+.
On Android API 15+ I had issues with unsupported SSLv1.2 and couldn't connect to Infura.

Things that have been tested:

  1. BIP44 wallet generation
  2. BIP44 wallet import
  3. Message signing
  4. Communication with Ethereum node via eth_getBalance (and other methods done under the hood)
  5. Message signing
  6. Transaction signing
  7. Sending ETH
  8. ENS address lookup

Test app: https://github.com/serso/web3a
Proguard rules: https://github.com/serso/web3a/blob/master/app/proguard-rules.pro

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 300.0 DAI (300.0 USD @ $1.0/DAI) has been submitted by:

  1. @serso

@ceresstation please take a look at the submitted work:


@serso
Copy link
Contributor

serso commented Dec 3, 2018

When the review is done I'll rebase the task branch on 4.0.0-android. It should be pretty simple as originally I based my work on the stable branch.

@serso
Copy link
Contributor

serso commented Dec 4, 2018

Hey guys,

It would be nice if you could provide some feedback on the PR. You don't need to review the whole code, just check out the approach and confirm that it's okay.

@iikirilov
Copy link
Contributor

Fixed in #809

snazha-blkio added a commit that referenced this issue Dec 7, 2018
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @serso.

@snazha-blkio snazha-blkio added the awaiting-release On release branch, awaiting build label Dec 9, 2018
@serso
Copy link
Contributor

serso commented Dec 14, 2018

@snazha-blkio when do you plan to release the Android version? I tried some snapshot artifacts (abi, compat, crypto, rlp and utils) and they seem to work fine on Android.

@snazha-blkio snazha-blkio removed the awaiting-release On release branch, awaiting build label Dec 17, 2018
@snazha-blkio
Copy link
Contributor Author

ried some snapshot artifacts (abi, compat, crypto, rlp and utils) and they seem to work fine on Android.

4.1.0-android has just been released.

@snazha-blkio
Copy link
Contributor Author

Hi @serso

Would you have any issues if we took a cut of your web3a (https://github.com/serso/web3a) code and published it in our web3j repo as a sample android app?

The idea is to build upon it to have a good app to test functionality with.

Please let us know if have any issues with it.

Thanks.
Sam.

@serso
Copy link
Contributor

serso commented Mar 31, 2019

@snazha-blkio No, I don't mind that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants