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 support for lnrpc subservices #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johncantrell97
Copy link

This adds support for all of the rpc subservices that lnd exposes. The original implementation of lnrpc was loading only the services found in rpc.proto (lightning, walletUnlocker).

I ended up refactoring a lot of how lnrpc.js works by allowing a user pass which services they want to load (defaults to [lightning, walletUnlocker) so it won't break existing users.

I also bumped the version number and the version of grpc being used.

This works fine for the project I needed subservices for (router, signer) but probably needs to be cleaned up further to be merged into master.

The old code exposed all of the lightning and walletUnlocker methods on the root lnrpc object. This presents a problem because some of the subservices share the same method names as those found in the lightning service. For now I did not change how this works so if you want to use a subservice you are required to use it directly (e.g. lnrpc.signer.verifyMessage({})) whereas if you used lnrpc.verifyMessage it would default to the method exposed by the lightning service.

@Matt-Jensen
Copy link
Owner

Hey John, thanks for adding support for all these new services! There's obviously been some big API changes since I last played with LND, so I appreciate you doing all the leg work bringing this project back up to speed.

I like how you've implemented tree shaking on all the new services, which might otherwise be overkill for the average lnrpc user. Great design decision here 👍 .

Here's what I'm thinking needs to be addressed to merge:

  • DRY out all the new services in lib/*.js
  • Address your concern: This presents a problem because some of the subservices share the same method names as those found in the lightning service, by either making API changes w/ a major release or ensuring all future services are only accessible under their own namespace (excluding walletUnlocker). IMO a major release feels cleaner.
  • Add test coverage
  • Add subservice/tree shaking README documentation

Anything I missed? Would love your feedback. Btw I'll commit to this PR soon, so if you've got to move on, you've done more than enough already! However I'll be sure your work gets published.

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