Allow for withdrawing on-chain funds #61
Conversation
| @@ -208,6 +208,60 @@ where | |||
| Ok(self.inner.lock().unwrap().get_balance()?) | |||
| } | |||
|
|
|||
| pub(crate) fn send_to_address( | |||
There was a problem hiding this comment.
Perhaps it is better to allow pass an optional fee rate?
Also a lightning node user would probably want to send all the onchain funds (empty the wallet) so perhaps this use case should be taken into account when exposing this ability at the Node level.
There was a problem hiding this comment.
Perhaps it is better to allow pass an optional fee rate?
We likely don't want to expose all the complexity of on-chain fee estimation to the user, but provide sane defaults, otherwise we'll end up replicating more and more of BDK/LDK APIs. We could consider letting them set a specific ConfirmationTarget, but I'm not fully convinced that we want to complicate the API that far here. That said, we probably should provide a config knob allowing to bound the fee rate (for all on-chain operations) eventually, which is tracked here: #55
Also a lightning node user would probably want to send all the onchain funds (empty the wallet) so perhaps this use case should be taken into account when exposing this ability at the Node level.
Ah, good idea. I now added a withdraw_all method (and renamed send_onchain_payment to withdraw for consistency).
tnull
changed the title
tnull
force-pushed
the
2023-04-allow-onchain-spending
branch
from
d5c5e01 to
4ca987c
Compare
tnull
force-pushed
the
2023-04-allow-onchain-spending
branch
from
4ca987c to
76070f9
Compare
|
Rebased after #13 has been merged. |
tnull
force-pushed
the
2023-04-allow-onchain-spending
branch
from
64c4f18 to
7f128d9
Compare
tnull
force-pushed
the
2023-04-allow-onchain-spending
branch
from
7f128d9 to
fde2fe9
Compare
|
Rebased on main. |
| self.wallet.get_balance() | ||
| } | ||
|
|
||
| /// Send an on-chain payment to the given address. | ||
| pub fn send_to_onchain_address( |
There was a problem hiding this comment.
Any reason why we don't expose the wallet? Something to do with the runtime?
There was a problem hiding this comment.
I think there are a number of reasons. For example I'd really like to keep the main API centered around a single Node object, at least as long as that is still feasible. We hopefully never get to a place where the API surface is getting unfeasibly large for a single object. If it does this might rather act as a canary that we should refactor it into something more minimal rather than breaking out individual objects.
Additionally, it's tbd how and what additional wallet behavior we will/should expose to give users more flexibility. Post-0.1 we might think about offering a wallet interface to allow users to bring their own implementation, which I'm however still not convinced of, as this might as well be quite a footgun. However, if we were to go this route, the exposed API also needs to be separate from the concrete wallet implementation. We'll see..
src/wallet.rs
Outdated
| Ok(txid) | ||
| } | ||
|
|
||
| pub(crate) fn drain_to_address(&self, address: &bitcoin::Address) -> Result<Txid, Error> { |
There was a problem hiding this comment.
Could we dry this up by delegating to send_to_address or I suppose some common helper taking Option<u64> for the amount?
There was a problem hiding this comment.
The former is unfortunately not possible, as we won't ever get the amount right, which is exactly the use case of drain_to_address. However I now DRYed it up into a single method taking an amount_msat_or_drain: Option<u64>.
Previously we only exposed methods to generate new addresses, retrieve the balance, and fund channels. Here, we fix the oversight and allow users to actually withdraw their funds again. We also rename some API methods for consistency of the term `onchain`.
tnull
force-pushed
the
2023-04-allow-onchain-spending
branch
from
0690996 to
1f3dfa8
Compare
|
Squashed without further changes. |
Based on #13, fixes #58.Previously we only exposed methods to generate new addresses, retrieve
the balance, and fund channels. Here, we fix the oversight and allow
users to actually withdraw their funds again.
We also rename some API methods for consistent use of the term
onchain.