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

Update to latest rust-dlc #213

Merged
merged 4 commits into from
Mar 7, 2023
Merged

Update to latest rust-dlc #213

merged 4 commits into from
Mar 7, 2023

Conversation

luckysori
Copy link
Contributor

The new rust-dlc branch is still WIP, but it's good to keep this dependency up-to-date as much as possible.

@luckysori luckysori requested review from bonomat and holzeis March 7, 2023 02:09
@luckysori luckysori self-assigned this Mar 7, 2023
@luckysori luckysori force-pushed the chore/ln-dlc-node-deps-upgrade branch from 7723d10 to 2701cb6 Compare March 7, 2023 02:16
Copy link
Contributor

@da-kami da-kami left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1022,5 +1022,5 @@ packages:
source: hosted
version: "2.0.3"
sdks:
dart: ">=2.19.0 <4.0.0"
dart: ">=2.19.0 <3.0.0"
Copy link
Contributor

@da-kami da-kami Mar 7, 2023

Choose a reason for hiding this comment

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

🛠️ I don't think this change should be part of this PR. I keep running into this problem as well on my machine (somehow it is always changed back; but I think for @bonomat it is then changed back to 4.0.0 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it, but according to the pubspec.yaml, this change is correct:

environment:
  sdk: ">=2.17.5 <3.0.0"

If you play around with those constraints and you run flutter pub get the pubspec.lock changes as expected.

Most notably:

- We now depend on the `split-tx-experiment` branch of
`p2pderivatives/rust-lightning` fork. It appears that the changes we
had in our fork are no longer needed and we need to update the
dependency to be able to use the new branch of `rust-dlc`.

- We've had to add our own `CustomSigner` and `CustomKeysManager`
since `rust-dlc` no longer exports them. For now we have just inlined
the implementations used in the `rust-dlc` tests.
We no longer need to patch the dependency after version 2.0 has been
published.
This dependency should ideally only be known by core crates such as
`ln-dlc-node`.

We define a method `get_new_address` on `Node` so that we can control
how we call the underlying on-chain wallet, rather than letting
consumers call the wallet through a trait that is designed for a
completely different purpose.

We also turn the generic `get_dlcs` API into the more specific
`get_confirmed_dlcs`, as this is the only thing that is currently
needed. This way we can avoid consumers needing to understand types
defined by `rust-dlc`. The `get_confirmed_dlcs` API and the matching
`Dlc` type should evolve (and perhaps even disappear) as we meet new
requirements.
@luckysori luckysori force-pushed the chore/ln-dlc-node-deps-upgrade branch from 2701cb6 to 09d9710 Compare March 7, 2023 03:49
@luckysori
Copy link
Contributor Author

bors r+

@luckysori
Copy link
Contributor Author

I pushed 09d9710 by mistake here, but since #214 has already been approved and the build is going I'll let it be and close the other PR once this one merges.

@bors
Copy link
Contributor

bors bot commented Mar 7, 2023

@bors bors bot merged commit 5455551 into main Mar 7, 2023
@bors bors bot deleted the chore/ln-dlc-node-deps-upgrade branch March 7, 2023 04:06
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.

2 participants