-
Notifications
You must be signed in to change notification settings - Fork 23
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
Manage spendable outputs generated by LDK #1007
Conversation
@@ -679,6 +684,45 @@ where | |||
remote_handle | |||
}; | |||
|
|||
tokio::spawn({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I guess a task is working for the coordinator, but for the mobile app we may need to trigger it on startup? or allow for manually triggering that sync? I don't think users will have the app open for 30 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is triggered on start-up inherently 😄
I agree that exposing a different mechanism for app users would also be useful. But for now we can just tell them to restart the app if they want to ensure that missing funds are claimed? I didn't want to get into adding stuff to the frontend with this change.
// Could not find spendable output as expected | ||
Err(Error::NotFound) => {} | ||
_ => { | ||
panic!("Expected to not be able to find deleted spendable output"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ why do we need to panic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just followed the pattern of the other test where we delete a row. The point is that we are expecting a specific error when loading a deleted row, so we can fail the test in all other cases.
/// Number of confirmations required to consider an LDK spendable output _spent_. | ||
/// | ||
/// We use this to determine which outputs we can forget about. | ||
const REQUIRED_CONFIRMATIONS: u32 = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Is 6
some kind of recommended value or arbitrary?
|
||
/// Decide on which [`Action`] should be performed based on the characteristics and status of a | ||
/// [`SpendableOutputDescriptor`]. | ||
fn choose_spendable_output_action( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃 It might be worth querying the status and height outside of this function from esplora_client
; then we could create a pure function that can be tested with unit tests.
For all `lnd_dlc_nod::Node`s we spin up a task which ensures that LDK spendable outputs that are "owned" by LDK are moved to the `Node`s on-chain wallet. This functionality could be split between coordinator and app (the two types of `Node` currently conceived), but it does not seem worth it at this stage.
c5a99d4
to
45337e7
Compare
// note, force pushed due to adding an entry to the changelog |
bors r+ |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Fixes #846.
For all
lnd_dlc_nod::Node
s we spin up a task which ensures that LDK spendable outputs that are "owned" by LDK are moved to theNode
s on-chain wallet.This functionality could be split between coordinator and app (the two types of
Node
currently conceived), but it does not seem worth it at this stage.