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

Feature/simplify api dsl #1690

Merged
merged 25 commits into from
Mar 9, 2021
Merged

Conversation

tompro
Copy link
Contributor

@tompro tompro commented Feb 14, 2021

So here is my first try on a bit of API restructuring. Handler are now isolated from each other and grouped first by topic (of my choosing, currently just getting started with Lightning tech) and then for the public route.

Common directives like auth control and default headers are only applied on the top level entry point and free up some setup in unit tests. What I currently don't really like is the customized implicit timeout as it has to be injected into every completer but I could'nt figure out a way to create a completer that provides an implicit implicitly ;)

What do you think?

apply auth and headers in public route, use specific tested handler in all specs, only use credentials in Basic auth tests
@codecov-io
Copy link

codecov-io commented Feb 14, 2021

Codecov Report

Merging #1690 (9a8fcd7) into master (fa759f1) will increase coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
+ Coverage   86.09%   86.28%   +0.18%     
==========================================
  Files         151      151              
  Lines       11523    11701     +178     
  Branches      493      501       +8     
==========================================
+ Hits         9921    10096     +175     
- Misses       1602     1605       +3     
Impacted Files Coverage Δ
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 98.28% <0.00%> (-0.98%) ⬇️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.21% <0.00%> (-0.55%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.30% <0.00%> (-0.54%) ⬇️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 96.15% <0.00%> (ø)
...cala/fr/acinq/eclair/blockchain/WatcherTypes.scala 100.00% <0.00%> (ø)
.../eclair/payment/relay/PostRestartHtlcCleaner.scala 90.62% <0.00%> (ø)
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala 92.85% <0.00%> (ø)
...la/fr/acinq/eclair/transactions/Transactions.scala 96.79% <0.00%> (+0.05%) ⬆️
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.50% <0.00%> (+0.06%) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 93.57% <0.00%> (+0.06%) ⬆️
... and 3 more

@t-bast
Copy link
Member

t-bast commented Feb 15, 2021

We're currently finalizing a few features because we want to publish a new release of eclair, I'll have a look at this PR once I'm done with the release work.

Don't worry if we're unable to review it this week, we will pick this up once we have some time available.

@t-bast
Copy link
Member

t-bast commented Feb 23, 2021

Thanks, that's a really nice refactoring, things are much cleaner afterwards.

I'd change a bit the categorization of the existing APIs (it's really hard because some APIs fit in multiple categories, and some don't really fit in any category). There is already a categorization that you can find in eclair-cli (https://github.com/ACINQ/eclair/blob/master/eclair-core/eclair-cli). I'd like to modify it to the following to take your proposal into account:

  === Node ===
    - getinfo
    - connect
    - disconnect
    - peers
    - audit

  === Channel ===
    - open
    - close
    - forceclose
    - channel
    - channels
    - allchannels
    - allupdates
    - channelstats

  === Fees ===
    - networkfees
    - updaterelayfee

  === Path-finding ===
    - findroute
    - findroutetonode
    - findroutebetweennodes
    - networkstats
    - nodes

  === Invoice ===
    - createinvoice
    - getinvoice
    - listinvoices
    - listpendinginvoices
    - parseinvoice

  === Payment ===
    - usablebalances
    - payinvoice
    - sendtonode
    - sendtoroute
    - getsentinfo
    - getreceivedinfo

  === Message ===
    - signmessage
    - verifymessage

  === OnChain ===
    - getnewaddress
    - sendonchain
    - onchainbalance
    - onchaintransactions

Can you update the eclair-cli file with that listing and apply it for the API handlers?

Apart from that everything looks good in the PR. Just a couple of comments about styling:

  • we like long lines and usually break at around 180 characters (I'm sure everyone has a decent screen nowadays and can affort these long lines)
  • we use IntelliJ's default formatting for scala (ctrl+alt+L) and avoid having multiple blank lines

Thanks again, I think this is a really nice refactoring and paves the way for more improvements afterwards, we will include this PR once we're finished with the next eclair release.

@tompro
Copy link
Contributor Author

tompro commented Feb 24, 2021

Yes, noticed the long lines, will get used to it :). Will apply your suggestions as soon as I have some spare time on my hand.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Thanks, a few nits but otherwise looks good.

There is a regression though that needs to be fixed:

> alice-eclair-cli getinf
The requested resource could not be found.

> bob-eclair-cli getinf
parse error: Invalid numeric literal at line 1, column 4

where alice-eclair-cli uses master and bob-eclair-cli uses your branch.

I have the same issue if I provide an invalid password, master correctly returns The supplied authentication is invalid whereas your change returns parse error: Invalid numeric literal at line 1, column 4.

You may be interested in https://github.com/t-bast/lightning-cfg to simplify E2E testing locally (this is what I used with tweaks to use your branch for Bob).

tompro and others added 16 commits March 7, 2021 14:04
…matting changes, re-added auth header in tests as it can not be isolated in handler chain order.
Since we almost always know which transactions will spend the utxos that we are watching, we can optimize the watcher to look for those instead of starting from scratch.
This fixes the `multiarg infix syntax looks like a tuple and will
be deprecated` warning.
If a channel closes when we've received an UpdateFailHtlc, signed it but
not yet received our peer's revocation, we need to fail the htlc upstream.

That specific scenario was not correctly handled, resulting in upstream
htlcs that were not failed which would force our upstream peer to close
the channel.
When using anchor outputs, the commitment feerate is kept low (<10 sat/byte).
When we need to force-close a channel, we must ensure the commit tx and htlc
txs confirm before a given deadline, so we need to increase their feerates.

This is currently done only once, at broadcast time.
We use CPFP for the commit tx and RBF for the htlc txs.
If publishing fails because we don't have enough utxos available, it will
be retried after the next block is confirmed.

Note that it's still not recommended to activate anchor outputs.
More work needs to be done on this fee bumping logic and utxos management.
This command is sent by the `Peer` to its `PeerConnection`, therefore it
needs to be a `RemoteType` and have its own codec.
* add tests on funding mindepth

We verify that when using wumbo channels:
- if we are funder we keep our regular min_depth
- if we are fundee we use a greater min_depth

* use lenses to simplify tags handling

Co-authored-by: Bastien Teinturier <[email protected]>
The module is still there and can be enabled if needed.
We previously used a Set, which means you could theoretically have a feature
that is both activated as `optional` and `mandatory`.

We change that to be a Map `feature -> support`.
We just merged lightning/bolts@b201efe in the spec
which added Bolt 3 tests, invalidating one of our tests and failing the build.
Drop mention of p2sh-segwit. We should encourage users to use bech32.
# Conflicts:
#	eclair-node/src/main/scala/fr/acinq/eclair/api/serde/JsonSerializers.scala
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tompro!

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

Looks good! Just one nit.

@t-bast t-bast merged commit 9ff2f83 into ACINQ:master Mar 9, 2021
@t-bast
Copy link
Member

t-bast commented Mar 9, 2021

Thanks @tompro!

@t-bast t-bast mentioned this pull request Mar 12, 2021
tompro added a commit to tompro/eclair that referenced this pull request Mar 21, 2021
Refactor the API handlers.
Split handlers and directives in several files to make them more composable.

Co-authored-by: Pierre-Marie Padiou <[email protected]>
Co-authored-by: Bastien Teinturier <[email protected]>
@tompro tompro deleted the feature/simplify-api-dsl branch March 21, 2021 07:22
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.

4 participants