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

use as plugin #16

Merged
merged 2 commits into from
Sep 19, 2019
Merged

use as plugin #16

merged 2 commits into from
Sep 19, 2019

Conversation

rsbondi
Copy link
Contributor

@rsbondi rsbondi commented Sep 15, 2019

This is just to get the conversation started, I think using as a plugin would be beneficial, especially if you are running outside of the default, the plugin knows where to find the rpc file, so configuration easier without needing to launch with env and can be just another entry in the lightning config file, also can be managed from the cli/rpc with the plugin command.

This was a bit more than I expected with the logging, but hopefully straight forward.

I could not get the tests to pass, not on a fresh install, but I don't see how the macaroon header is set in the tests, but I am probably missing something.

Also, if this looks like a good idea, additional readme update required

@saubyk
Copy link
Collaborator

saubyk commented Sep 15, 2019

Hi, thanks for the PR.
The APIs as plugin is definitely a better approach. I didn't spent enough time exploring that, as we were focused on getting the UI running with the APIs.

Also, regarding the tests, I need to fix them.

Thanks for all the changes, will review.

@saubyk
Copy link
Collaborator

saubyk commented Sep 19, 2019

Hi @rsbondi
What do I need to pass in PATH_TO_PLUGIN, while executing lightningd?

@rsbondi
Copy link
Contributor Author

rsbondi commented Sep 19, 2019

your local repo directory followed by /plugin.js

it should be executable when you check it out? if not chmod it

@@ -17,6 +17,7 @@
"dependencies": {
"atob": "^2.1.2",
"body-parser": "^1.19.0",
"clightningjs": "0.0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

May need to fork this library before adding it to our dependencies. Vary of adding freshly developed libraries, due to security reasons.

Copy link
Contributor Author

@rsbondi rsbondi Sep 19, 2019

Choose a reason for hiding this comment

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

it is from a very active c-lightning collaborator

https://github.com/ElementsProject/lightning/commits?author=darosior

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will merge in that case. Thanks.

@saubyk
Copy link
Collaborator

saubyk commented Sep 19, 2019

your local repo directory followed by /plugin.js

it should be executable when you check it out? if not chmod it

Tested it. Works beautifully. Thanks.

Copy link
Collaborator

@saubyk saubyk left a comment

Choose a reason for hiding this comment

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

Approving this PR. Thanks.

@saubyk saubyk merged commit 854d6ec into Ride-The-Lightning:master Sep 19, 2019
@saubyk
Copy link
Collaborator

saubyk commented Sep 23, 2019

@rsbondi Tests are fixed now in master. Will add more coverage soon.

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

Successfully merging this pull request may close these issues.

None yet

2 participants