multi: remove Pool, Loop and Faraday deps from main LNC module #66

ellemouton · 2023-01-10T12:42:30Z

The main goal of this PR is to remove the Pool, Loop and Faraday dependencies from the main LNC module.
This is done by converting the mobile package into a go module and then letting it and the wasm-client module import Litd permissions and JSON callback registrations from Litd itself rather from the core package. The core package is then removed.

Depends on lightninglabs/lightning-terminal#476
Replaces #65

ellemouton · 2023-01-10T14:47:41Z

@guggero & @kaloudis , any idea how to fix the android build issue? have tried a bunch of things without any luck.

Also still trying to figure out how fix the linter which now linting all code in the repo instead of just the code from the PR

kaloudis · 2023-01-10T15:05:36Z

@ellemouton I would try to see if I could update the version of gomobile used here: https://github.com/lightninglabs/lightning-node-connect/blob/master/.github/workflows/main.yml#L52

ellemouton · 2023-01-11T07:58:38Z

@kaloudis, thanks but I tried using @latest but that still results in the same issue of the import not being found

ellemouton · 2023-01-11T08:21:20Z

ok, managed to get the linter working by making use of new-from-rev and by fixing one linter error that it insisted on for some reason. Now just need to figure out the bind stuff

To prevent version incompatabilities in upcoming commits, we change the aperture version here to a version that uses a tor version supported by lnd v0.15.5-beta.

In this commit, the MailboxRPCConnection function is renamed and moved out of the core package and into the mailbox package.

Update the go version used by Dockerfile-wasm along with the go version used by Github actions.

Update linter to 1.50.0

kaloudis · 2023-01-11T16:06:19Z

Nice job! WASM and Android appear to be building now

kaloudis

LGTM

ellemouton · 2023-01-11T18:11:17Z

cool - updated this to point to the merged Lit PR 👍

guggero

Very nice! Only two comments around init().

guggero · 2023-01-11T18:27:34Z

cmd/wasm-client/main.go

+
+func init() {
+	var err error
+	permsMgr, err = perms.NewManager(true)


I'm not sure init() works correctly with WASM. Can we do that at the beginning of main() instead?
I guess we should also test the changes with the demo HTTP page.

guggero · 2023-01-11T18:29:13Z

mobile/mobile.go

+
+func init() {
+	var err error
+	permsMgr, err = perms.NewManager(true)


Same here, not sure if the mobile framework calls init(). Could be in InitLNC (after the mutex, with a nil check in case it was already initialized).

ellemouton · 2023-01-12T06:11:58Z

@guggero , I did test the wasm client with the example page to make sure that the permissions manager works as expected 👍 So is it ok to leave the init function in that case or have you experienced that the init function does not consistently work in wasm?

@kaloudis - could you test if the mobile package's HasPermissions function works as expected with this PR?

guggero · 2023-01-12T09:26:54Z

@guggero , I did test the wasm client with the example page to make sure that the permissions manager works as expected +1 So is it ok to leave the init function in that case or have you experienced that the init function does not consistently work in wasm?

Hmm, I remember there being an issue with init() in some cases, but can't seem to find it anymore. Not sure if it was WASM or mobile, or something else. But since we have a main() method anyway, I think it would be cleaner to do the initialization there.

In this commit, the dependency on the core package is removed and hence the imports of loop, pool and faraday are also removed from LNC. The mobile and wasm-clients now instead import the permissions and JsonCallbacks that they need from Litd.

ellemouton · 2023-01-12T09:44:52Z

cool cool - updated to remove the init funcs :)

guggero · 2023-01-12T10:09:03Z

Very nice! Waiting with merge until we have a confirmation that everything works on mobile as expected.

ellemouton · 2023-01-12T10:18:37Z

^ cc @kaloudis 🙏 📱

kaloudis · 2023-01-12T16:05:26Z

Tested in Android using a modified version of the lnc-rn connect demo:

I used a read-only session and called the hasPerms call for the last two values below:

guggero · 2023-01-12T16:23:48Z

Awesome, thanks for confirming!

ellemouton force-pushed the lncModulesRefactor branch 8 times, most recently from 63a4c71 to ceda9eb Compare January 10, 2023 13:51

ellemouton mentioned this pull request Jan 10, 2023

multi: add some litd client helpers lightninglabs/lightning-terminal#476

Merged

ellemouton force-pushed the lncModulesRefactor branch 2 times, most recently from ceda9eb to 0c3ee4e Compare January 10, 2023 14:43

ellemouton force-pushed the lncModulesRefactor branch 3 times, most recently from b2b871f to a2d30b7 Compare January 11, 2023 08:17

ellemouton force-pushed the lncModulesRefactor branch 6 times, most recently from 70e0b79 to 6d3cf8e Compare January 11, 2023 14:03

ellemouton added 6 commits January 11, 2023 16:12

go.sum: run go mod tidy

415d87e

multi: update aperture, lnd and tor version

63d868e

To prevent version incompatabilities in upcoming commits, we change the aperture version here to a version that uses a tor version supported by lnd v0.15.5-beta.

multi: move client conn creation func to mailbox

d72d0a9

In this commit, the MailboxRPCConnection function is renamed and moved out of the core package and into the mailbox package.

wasm-client: update go version to 1.18

824eb2c

multi: update go version to 1.18

3203c51

multi: update GH and docker go version

27a464b

Update the go version used by Dockerfile-wasm along with the go version used by Github actions.

ellemouton added 3 commits January 11, 2023 16:38

.github: update actions/checkout and cache to v3

abecf7d

Makefile+.golangcli: update linter version

fe3f6b7

Update linter to 1.50.0

mobile: convert into a go module

38d34d9

ellemouton force-pushed the lncModulesRefactor branch from 6d3cf8e to c458647 Compare January 11, 2023 14:55

ellemouton requested review from guggero and kaloudis January 11, 2023 14:55

kaloudis approved these changes Jan 11, 2023

View reviewed changes

ellemouton force-pushed the lncModulesRefactor branch from c458647 to f591ff6 Compare January 11, 2023 18:10

guggero reviewed Jan 11, 2023

View reviewed changes

multi: remove core package

34664e6

In this commit, the dependency on the core package is removed and hence the imports of loop, pool and faraday are also removed from LNC. The mobile and wasm-clients now instead import the permissions and JsonCallbacks that they need from Litd.

ellemouton force-pushed the lncModulesRefactor branch from f591ff6 to 34664e6 Compare January 12, 2023 09:39

ellemouton requested a review from guggero January 12, 2023 09:44

guggero approved these changes Jan 12, 2023

View reviewed changes

levmi assigned ellemouton Jan 12, 2023

guggero merged commit ee29cbd into lightninglabs:master Jan 12, 2023

ellemouton deleted the lncModulesRefactor branch January 12, 2023 17:52

kaloudis mentioned this pull request Feb 16, 2023

Makefile: apple: accomodate move of mobile to go module #69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: remove Pool, Loop and Faraday deps from main LNC module #66

multi: remove Pool, Loop and Faraday deps from main LNC module #66

ellemouton commented Jan 10, 2023 •

edited

Loading

ellemouton commented Jan 10, 2023

kaloudis commented Jan 10, 2023

ellemouton commented Jan 11, 2023

ellemouton commented Jan 11, 2023

kaloudis commented Jan 11, 2023

kaloudis left a comment

ellemouton commented Jan 11, 2023

guggero left a comment

guggero Jan 11, 2023

guggero Jan 11, 2023

ellemouton commented Jan 12, 2023

guggero commented Jan 12, 2023

ellemouton commented Jan 12, 2023

guggero commented Jan 12, 2023

ellemouton commented Jan 12, 2023

kaloudis commented Jan 12, 2023

guggero commented Jan 12, 2023

multi: remove Pool, Loop and Faraday deps from main LNC module #66

multi: remove Pool, Loop and Faraday deps from main LNC module #66

Conversation

ellemouton commented Jan 10, 2023 • edited Loading

ellemouton commented Jan 10, 2023

kaloudis commented Jan 10, 2023

ellemouton commented Jan 11, 2023

ellemouton commented Jan 11, 2023

kaloudis commented Jan 11, 2023

kaloudis left a comment

Choose a reason for hiding this comment

ellemouton commented Jan 11, 2023

guggero left a comment

Choose a reason for hiding this comment

guggero Jan 11, 2023

Choose a reason for hiding this comment

guggero Jan 11, 2023

Choose a reason for hiding this comment

ellemouton commented Jan 12, 2023

guggero commented Jan 12, 2023

ellemouton commented Jan 12, 2023

guggero commented Jan 12, 2023

ellemouton commented Jan 12, 2023

kaloudis commented Jan 12, 2023

guggero commented Jan 12, 2023

ellemouton commented Jan 10, 2023 •

edited

Loading