Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

refactor(lnd): promisify lnd calls #1319

Merged
merged 6 commits into from
Jan 18, 2019

Conversation

korhaliv
Copy link
Member

Description:

Promisify lnd calls

Motivation and Context:

Refactor lnd methods to use promisify. This reduces boilerplate code, makes it more compact and easier to read. Also simplifies addition of new methods in future

How Has This Been Tested?

Manually. Tested functionality that involves corresponding lnd calls

Types of changes:

Refactor

Checklist:

  • My code follows the code style of this project.
  • I have reviewed and updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes where needed.
  • All new and existing tests passed.
  • My commits have been squashed into a concise set of changes.

@coveralls
Copy link

coveralls commented Jan 11, 2019

Coverage Status

Coverage increased (+0.4%) to 19.598% when pulling 0a0a5ed on korhaliv:refactor/promisify-lnd into 81ddf35 on LN-Zap:next.

@mrfelton mrfelton self-requested a review January 12, 2019 10:00
@mrfelton mrfelton added the type: refactor Improvement to the codebase label Jan 12, 2019
Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

Concept ACK

@mrfelton
Copy link
Member

@korhaliv this is a good improvement. I think you should extract the promisify wrapper method to a separate file and apply the same treatment across the rest of the lnd method proxies.

@korhaliv
Copy link
Member Author

Agree, I was going to apply the same kind of refactor in some other parts of the app. One thing that stopped me from extracting the method at this stage is because lnd expects us to pass args as an object (as opposed to spreading them). This may not be the case in other places, so need to check

@mrfelton
Copy link
Member

Have you seen https://github.com/carlessistare/grpc-promise @korhaliv ? Not saying that we should implement necessarily, but with a look to see what they are doing at least.

Also some discussion about this topic here grpc/grpc-node#54

@korhaliv
Copy link
Member Author

korhaliv commented Jan 12, 2019

No, I will check that. I prefer to do as much as possible using standard tools (like node promisify in this case). If you can't do something with a reasonable complexity with standard packages, only then I look for 3rd party packages. Also another thing is that particular package you are referring to doesn't seem very popular judging from a number of stars (may not be the best metric, but it's another thing in my checklist when looking for 3rd party packages).

@mrfelton
Copy link
Member

Agreed. no need for additional dependency here, but if there are issues you are facing the chances are that they have been solved by the people working on that package, so could be a good point of reference.

@korhaliv
Copy link
Member Author

Extracted promisifiedCall

@mrfelton mrfelton changed the base branch from master to next January 16, 2019 16:35
@korhaliv
Copy link
Member Author

Promisified the entire lnd/methods folder

Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

Tested ACK 0a0a5ed

@mrfelton mrfelton merged commit 6f96351 into LN-Zap:next Jan 18, 2019
@korhaliv korhaliv deleted the refactor/promisify-lnd branch January 19, 2019 21:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: refactor Improvement to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants