feat: Sync wallet when contract is not confirmed yet. #2570
Conversation
There was a problem hiding this comment.
I'm not a fan of this :(
I think it makes sense to sync eagerly in some cases. But when I call is_dlc_channel_confirmed I don't always want the sync to trigger e.g. in certain tests.
I would be okay with defining a separate API with similar behaviour, to be called in certain spots.
coordinator/src/trade/mod.rs
Outdated
| .is_dlc_channel_confirmed(&dlc_channel_id) | ||
| .await? | ||
| { | ||
| bail!("Did not see funding tx in mempool yet."); |
There was a problem hiding this comment.
🔧 I don't like this change because it assumes that the DLC channel will always be confirmed as soon as the transaction hits mempool, but this may not always be the case.
I think the original error message is fine, but if you want better error messages you could adapt the is_dlc_channel_confirmed API to return more information to build a better error message here.
There was a problem hiding this comment.
🔧 I don't like this change because it assumes that the DLC channel will always be confirmed as soon as the transaction hits mempool, but this may not always be the case.
I thought this was the case. When do we have to wait for a confirmation?
There was a problem hiding this comment.
What I mean is that the error message now reflects an implementation detail: that the number of required confirmations is always 0. And the implementation detail may change in the future, but this error message is independent of that and could easily get stale.
There was a problem hiding this comment.
Fair enough! I will adapt the message.
hmm.. Can you explain in what tests you would not want to eagerly sync if the dlc channel is signed, but the contract is not confirmed? I thought this could only be the case if we are missing a sync, because use 0-conf everywhere.
I can definitely do that, but I guess I will replace all calls to the current |
coordinator/src/trade/mod.rs
Outdated
| .is_dlc_channel_confirmed(&dlc_channel_id) | ||
| .await? | ||
| { | ||
| bail!("Did not see funding tx in mempool yet."); |
There was a problem hiding this comment.
What I mean is that the error message now reflects an implementation detail: that the number of required confirmations is always 0. And the implementation detail may change in the future, but this error message is independent of that and could easily get stale.
|
I will see to rename the function to make it clearer. |
holzeis
force-pushed
the
fix/adapt-unconfirmed-dlc-channel-message
branch
from
a177ac0 to
4e62b51
Compare
This is covering an edge case where the user opens a dlc channel and wants to close the position within the sync window of the coordinator.
It allows to instantly close the position without having to manually sync the wallet after the dlc channel has been created.
It's probably not the most important feature for prod, but it makes testing a bit easier... Happy to close this PR if the reviewer thinks that this is an unnecessary overhead.