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

Move reusable API parts to a trait #1614

Closed
wants to merge 7 commits into from
Closed

Move reusable API parts to a trait #1614

wants to merge 7 commits into from

Conversation

akumaigorodski
Copy link
Contributor

This takes all reusable parts from Service trait and puts them into a more generally applicable AbstractService trait. The goal is to provide reusable parts for plugins which would establish API of their own (HC is such a plugin).

@akumaigorodski
Copy link
Contributor Author

@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #1614 (930d20e) into master (b75f6c3) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1614      +/-   ##
==========================================
+ Coverage   85.92%   85.95%   +0.02%     
==========================================
  Files         151      151              
  Lines       11471    11471              
  Branches      505      505              
==========================================
+ Hits         9857     9860       +3     
+ Misses       1614     1611       -3     
Impacted Files Coverage Δ
...clair/blockchain/bitcoind/rpc/BatchingClient.scala 86.36% <0.00%> (-13.64%) ⬇️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 93.70% <0.00%> (-1.58%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 80.51% <0.00%> (+0.25%) ⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 76.98% <0.00%> (+2.38%) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 78.49% <0.00%> (+4.30%) ⬆️

Plugins won't need to re-implement splitted part and API rules will be more or less standard across base and plugins.
@akumaigorodski
Copy link
Contributor Author

What about this one, especially in light of #1640?

FWIW my opinion is current approach is good for these reasons:

  1. API is a basic function which users expect to be packaged, not optional.
  2. Once boilerplate code is factored out defining API becomes pretty straightforward both in core and plugins.
  3. Having a common base interface feels better than letting each API-enabled plugin to build this completely from scratch.

# Conflicts:
#	eclair-node/src/main/scala/fr/acinq/eclair/api/Service.scala
@t-bast
Copy link
Member

t-bast commented Dec 30, 2020

We have plans to heavily re-work the JSON API in the not-too-distant future, so I don't think we'll take in changes like the ones you propose until we've figured out where we want to go in more details.

Extending the API for custom nodes can very easily be done in a fork for now, it's a part that doesn't move much so it's easy to keep up-to-date with master.

@akumaigorodski
Copy link
Contributor Author

Will keep it open to give it a chance to be considered still.

@t-bast
Copy link
Member

t-bast commented Aug 12, 2021

Is this still necessary? I believe the changes we made to plugins recently enabled something similar to what this PR was doing.

@akumaigorodski
Copy link
Contributor Author

Just forgot about this one, not relevant any more indeed.

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.

3 participants