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

First basic draft version of a java client library. need feedback #2223

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@renepickhardt
Copy link
Collaborator

renepickhardt commented Jan 5, 2019

I have started to create a Java Client Library for c-lightning called JLightning which is basically following pylightning code.

This PR is work in progress. I would like to have some feedback before I continue to finish the work.

  1. As Java is a typed language I wonder if the API calls should really give back JSON or just Strings?
  2. Naming methods and arguments should follow a certain standard. I started out taking over the argument names from c-lightning but java has a different naming conventions.
  3. DocStrings. I started to copy the doc strings from pylightning but again Java doc strings are usually formatted differently producing a particular line for each parameter.
  4. The Client Library handles the communication with the Unix Domain Socket differently than pylightning (less complex but it is still working) am I missing something? I guess I will not be backward compatible with older c-lightning nodes.
  5. Java does not support default values for methods so calls without arguments would get a lot of null values.
  6. The check which fields in the payload are null is quite annoying in java I wonder if I should check this within the lib when adding the fields to the HashMap
@jb55
Copy link
Collaborator

jb55 left a comment

Just a comment regarding reading from the socket. I'm not sure if this needs to be in the clightning repo, perhaps it could be distributed as a standlone Java library on your github?

while(true) {
int read = this.is.read(buf);
response = response + new String(buf, 0, read, "UTF-8");
if(read <=0 || response.contains("\n\n" )) {

This comment has been minimized.

Copy link
@jb55

jb55 Jan 6, 2019

Collaborator

in my Haskell implementation I count open and closed curly brackets (ensuring they sum to 0) to determine when to stop reading.

This could fail in an unlikely event that both \n's appear at the end and start of two separate reads. This would result in the entire program blocking indefinitely on the next read call.

This comment has been minimized.

Copy link
@renepickhardt

renepickhardt Jan 7, 2019

Author Collaborator

Response is the string that has been appended with the buffer so I see no problem with split \n

On the other side the sum of open and closed brakets might fail as those could be part of the strings in the json data. So those need to be ignored

This comment has been minimized.

Copy link
@jb55

jb55 Jan 7, 2019

Collaborator

ah good point on the curly's. I see now that it gets appended before the check so yeah looks like it would be fine. I should probably do something similar... I do find this part a bit strange. It looks like lightning-cli does it by parsing tokens as they come in, and ends the reading when there is no more to parse.

@jeffrade

This comment has been minimized.

Copy link

jeffrade commented Feb 15, 2019

@renepickhardt I'm personally not a fan of using Optional, but if using Java 1.8, you can use it to handle null.

* network at http://ln.rene-pickhardt.de
*
* @author Rene Pickhardt

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

nit - extra line break.

public static void main(String[] args) {
// TODO Auto-generated method stub

JLightningRpc rpc_interface = new JLightningRpc("/tmp/spark-env/ln1/lightning-rpc");

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

I would inject this "/tmp/spark-env/ln1/lightning-rpc" via command line. Will be available in args.

package de.renepickhardt.ln;

/**
* JLightning a small test / example class to demonstrate how to use JLightningRpc

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

Maybe move this into a README.md and add the necessary code snippets that you provide here in the main method.

@@ -0,0 +1,14 @@
Draft version of the JLightning rpc interface for c-lightning.

Using this client library is is as simple as the following code:

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

Even if someone is familiar with java, would be nice to have the actual command to build and launch: e.g.

$ javac <details-to-build>
$ java <details-to-run>

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

On that note, have you though of using Gradle or Maven to handle building and dependency management? Preferably Gradle

public String delInvoice(String label, Status status) {
HashMap<String, String> payload = new HashMap<String,String> ();
payload.put("label", label);
payload.put("status", status.toString());

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

To handle NullPointerException here on toString(), you can use String.valueOf(status). In case of null, it will return the String "null".

payload = new HashMap<String, String>();
}

//remove null items from payload

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

There's no need to add the keys to a new HashSet here. You can simply iterate through the payload.keySet() and and remove an entry if value is null.

Here's some sample code:

    Map<String, String> m = new HashMap<String, String>();
    m.put("1", "One");
    m.put("2", null);
    System.out.println(m);
    for(String k:m.keySet()) {
      m.remove(k, null);
    }
    System.out.println(m);

And output is:

{1=One, 2=null}
{1=One}
json.put("method", method);
json.put("params", new JSONObject(payload));
// FIXME: Use id field to dispatch requests
json.put("id", Integer.toString(this.id++));

This comment has been minimized.

Copy link
@jeffrade

jeffrade Feb 15, 2019

this.id isn't thread-safe since it is static. Use AtomicInteger instead and then call getAndIncrement().

@greenaddress

This comment has been minimized.

Copy link

greenaddress commented Feb 17, 2019

Not sure how relevant but in abcore I needed json rpc to talk to Bitcoin core daemon running in the background and I used https://github.com/Polve/bitcoin-rpc-client , maybe it can be adapted?

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Apr 11, 2019

@renepickhardt are you planning on moving this library into its own repository? Otherwise I could give it a new home in the lightningd org and add you there :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.