Skip to content

[mobile] Mobile RPCs#3282

Merged
Roasbeef merged 9 commits intolightningnetwork:masterfrom
halseth:mobile-rpcs
Sep 9, 2019
Merged

[mobile] Mobile RPCs#3282
Roasbeef merged 9 commits intolightningnetwork:masterfrom
halseth:mobile-rpcs

Conversation

@halseth
Copy link
Contributor

@halseth halseth commented Jul 9, 2019

This PR adds the necessary APIs and build scripts in order to compile lnd for mobile platforms.

Building mobile libraries

Prerequisites

gomobile

Follow gomobile in order to intall gomobile and dependencies.

Remember to run gomobile init! (otherwise the lnd build might just hang).

falafel

Install falafel:

go get -u -v github.com/halseth/falafel

Building lnd for iOS

make ios

Building lnd for Android

make android

make mobile will build both iOS and Android libs.

Libraries

After the build has succeeded, the libraries will be found in mobile/build/ios/Lndmobile.framework and mobile/build/android/Lndmobile.aar. Reference your platforms' SDK documentation for how to add the library to your project.

API docs

TODO

@cfromknecht cfromknecht self-requested a review July 9, 2019 22:12
Copy link
Contributor

Choose a reason for hiding this comment

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

splitting the extraArgs string breaks if I want to pass a path that contains a space.
e.g. /Library/Application\ Support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you check if this helps: 22f7b0c

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like it's working 🙌
haven't done a lot of testing though.

@cfromknecht
Copy link
Contributor

@halseth didn't get a chance to review the autogenerated code :(

@halseth
Copy link
Contributor Author

halseth commented Jul 11, 2019

@cfromknecht added a temp commit with the generated API!

Copy link
Contributor

Choose a reason for hiding this comment

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

same name & package as the SendPayment method in lightning_api_generated.go.
maybe add a prefix to the methods generated by sub-services?

Copy link

@sergioabril sergioabril Jul 11, 2019

Choose a reason for hiding this comment

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

@ottosuess agree. But as @halseth suggested somewhere else, you can try setting usePrefex=1 on gen_bindings.sh

(Haven't tried though, I manually changed names)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it possible to add directly to make (73cb325):

make ios tags="routerrpc" prefix=1

@ottosuess
Copy link
Contributor

Would be cool to have a way to inject custom protoc plugins like protoc-gen-swift.

At zap we have a custom "falafel" plugin to generate swift apis that i'd like to inject somehow.

Choose a reason for hiding this comment

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

Just thought I'd mention that I didn't have this command, had to run brew install protobuf to get it. Might be worth adding it as a prerequisite to the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed: 7d01dc5

@halseth
Copy link
Contributor Author

halseth commented Jul 19, 2019

Would be cool to have a way to inject custom protoc plugins like protoc-gen-swift.

At zap we have a custom "falafel" plugin to generate swift apis that i'd like to inject somehow.

I think this is a pro-level feature for now, so can be done by modifying gen_bindings directly 😄

@halseth halseth force-pushed the mobile-rpcs branch 3 times, most recently from 5aded27 to 3760f29 Compare July 24, 2019 06:59
@wpaulino wpaulino added this to the 0.8.0 milestone Jul 25, 2019
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour golang/build system Related to the go language and compiler mobile labels Jul 29, 2019
@halseth
Copy link
Contributor Author

halseth commented Aug 27, 2019

This commit currently breaks the build script: golang/mobile@597adff

So need to stay on an older revision for now to successfully build.

Latest gomobile@HEAD fixes this issue.

@halseth halseth force-pushed the mobile-rpcs branch 2 times, most recently from 8d06494 to 742c2b5 Compare August 27, 2019 08:29
@halseth
Copy link
Contributor Author

halseth commented Aug 27, 2019

PTAL @Roasbeef @valentinewallace

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

The auto generated code should be removed from this diff before we merge this PR.

lnd.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comment explaining what the empty function closure return value is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not totally sure what you mean, but expanded the comment to note what the default return values should be: 78bc0b1

@halseth halseth force-pushed the mobile-rpcs branch 2 times, most recently from 78bc0b1 to 85244f7 Compare September 2, 2019 09:27
@halseth
Copy link
Contributor Author

halseth commented Sep 2, 2019

Rebased and removed generated code. Generated code can still be inspected here: https://github.com/lightningnetwork/lnd/tree/78bc0b1128edc56f5f7d095a66e3a025d6a2d89e/mobile

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

nits aside, LGTM 🍾 yay!

mobile/README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/I order/In order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 210eeb8

@halseth halseth added the ready to merge LGTM'd, may need rebase to address conflicts, anyone can merge label Sep 4, 2019
@halseth
Copy link
Contributor Author

halseth commented Sep 4, 2019

Should now be ready to merge

ListenerCfg allows passing custom listeners to the main method, to be
used for the wallet unlocker and rpc server. If these are set these will
be used instead of the regular RPC listeners.
gen_bindings uses falafel to generate Go bindings from the lnrpc
protos.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvements to existing features / behaviour golang/build system Related to the go language and compiler mobile P2 should be fixed if one has time ready to merge LGTM'd, may need rebase to address conflicts, anyone can merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants