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

add cordova android plugin #36

Merged
merged 7 commits into from
Apr 24, 2020
Merged

add cordova android plugin #36

merged 7 commits into from
Apr 24, 2020

Conversation

ecioppettini
Copy link
Contributor

@ecioppettini ecioppettini commented Apr 23, 2020

relates to #31

Add cordova plugin with android implementation

Add simple Javascript interface that can be used to:

  • restore wallet
  • retrieve funds (from block)
  • get total funds

The exposed functions are very primitive, they work exactly the same way as the jni ones. Basically passing the wallet and settings pointers to and from js, exposing also the delete functions. I think we may want to expose something more idiomatic on top, but this serves to set some foundation.

The block is passed to js as base64. I think it may be useful to have some way of getting it from the filesystem, but I'm not sure.

Caveats

  • The tests can be run on the emulator or a device (but I only tested in the emulator). I still don't know how to introduce them in the CI.

  • I used npm pack for the release, but it seems npm tarball modules can't be installed from a local path (which seems to be a bug). The files can still be extracted and the plugin be installed by using the path to the extracted module, though. The other option is publishing directly to the npm registry.

@ecioppettini ecioppettini self-assigned this Apr 23, 2020
@NicolasDP NicolasDP added the VOTE Voltaire and related label Apr 23, 2020
@NicolasDP NicolasDP added this to the VIT: development milestone Apr 23, 2020
@ecioppettini ecioppettini force-pushed the add-cordova-plugin branch 2 times, most recently from 93f3eb8 to b1a6466 Compare April 23, 2020 21:16
Comment on lines +1 to +15
package com.iohk.jormungandrwallet;

public class Wallet {
static {
System.loadLibrary("wallet_jni");
}

public native static long recover(String mnemonics);

public native static void delete(long wallet);

public native static int totalValue(long wallet);

public native static long initialFunds(long wallet, byte[] block0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this can be done with cordova, but it feels like this is the wallet-android itself already. Can it be imported somehow? or maybe using a symlink or something like that?
Just dropping the idea here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we could just copy the file when packaging, the problem is mostly that I don't know how to use kotlin in the plugin. I think it is not technically impossible (maybe we need to transpile in build time, or maybe can do it when the plugin is installed with a hook).

I agree that it would be nice to have only one file, but I'm not sure if we need to do it now. There are a few differences, though, I'm just using the static methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we could bring back the java files if they are more useful. Or just keep them both as they are now. Shouldn't be a problem for now since they are pretty simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the code repetition is a problem now, but we should be careful about changes in the wallet-jni interface, things like adding or removing parameters, changing types, and so on. Maybe we could have some common java wrapper to rely on, and use kotlin on top.

PD: There is also some repetition for building, as building the plugin is kind of a superset of what's done in wallet-android.

@ecioppettini ecioppettini marked this pull request as ready for review April 24, 2020 19:22
@NicolasDP NicolasDP merged commit 00441f9 into master Apr 24, 2020
@NicolasDP NicolasDP deleted the add-cordova-plugin branch April 24, 2020 21:36
@NicolasDP NicolasDP mentioned this pull request Apr 30, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings - cordova VOTE Voltaire and related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants