Restore operation log entries for deposits #5915
Conversation
elsirion
force-pushed
the
2024-08-restore-deposit-log
branch
from
48b3286 to
2002349
Compare
elsirion
changed the title
elsirion
force-pushed
the
2024-08-restore-deposit-log
branch
from
2002349 to
1470dbc
Compare
| /// Returns an error for old deposit operations created prior to the 0.4 | ||
| /// release and not driven to completion yet. This should be rare enough | ||
| /// that an indeterminate state is ok here. |
There was a problem hiding this comment.
@maan2003 important to note for integrators
| WaitingForConfirmation { | ||
| // Caution: previously this variant contained `BitcoinTransactionData`, it should not have | ||
| // been persisted by the fedimint client though | ||
| btc_out_point: bitcoin::OutPoint, | ||
| }, |
There was a problem hiding this comment.
Maybe add confirmations?
There was a problem hiding this comment.
Out of scope for this PR imo
elsirion
force-pushed
the
2024-08-restore-deposit-log
branch
from
1470dbc to
d2147eb
Compare
| bail!("Operation is not a deposit operation"); | ||
| }; | ||
|
|
||
| let Some(tweak_idx) = tweak_idx else { |
There was a problem hiding this comment.
Document tweak idx being related to new oeprations
elsirion
force-pushed
the
2024-08-restore-deposit-log
branch
from
d2147eb to
136cb8b
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin releases/v0.4
git worktree add -d .worktree/backport-5915-to-releases/v0.4 origin/releases/v0.4
cd .worktree/backport-5915-to-releases/v0.4
git switch --create backport-5915-to-releases/v0.4
git cherry-pick -x ceeec00b4c5a821791c39508ce31832261544ebe 136cb8b7665d43d3b686280cbca8c5f3e101afd1 |
| }, | ||
| Claimed { | ||
| #[serde(with = "bitcoin::amount::serde::as_sat")] | ||
| btc_deposited: bitcoin::Amount, |
There was a problem hiding this comment.
@maan2003 and I were looking at this and I learned that this amount does not take into account the fee that we deducted by the federation.. maybe this is OK for now since the fee is a fixed amount.. but probably not the best pattern. I imagine an integrator would want to know the amount of BTC that was actually "claimed" to be used as their e-cash.
There was a problem hiding this comment.
Yeah, unfortunately it's very hard to accurately show the amount received once e-cash fees are taken into account. So rather than showing a mostly correct value I opted for just supplying the raw deposit value.
This PR is in no way ideal, it's a stop-gap solution till we probably rebuild the pegin monitor to allow for better observeability.
| btc_out_point, | ||
| }).await; | ||
|
|
||
| yield DepositStateV2::Claimed { |
There was a problem hiding this comment.
Wondering if we're missing the "await_primary_module_output" (or equivalent) here to actually indicate in the update stream that the e-cash has been issued and added to the balance..? Similar to https://github.com/fedimint/fedimint/blob/5e4247572386518809f5edb86397451a523edf3e/modules/fedimint-ln-client/src/lib.rs#L1624C43-L1624C71
There was a problem hiding this comment.
Yeah, missed that, but should be easy to fix. Less of a prio for users imo since this shouldn't fail, but definitely useful for testing.
The functionality to list and observe deposits was accidentally removed in #5473.
It's a bit tricky because technically a single generated address can now cause multiple operations. My plan is to create a first operation on address generation, this will correspond to the first deposit seen on-chain.
Possible extension
Any successive deposit could result in another operation log entry. Since this will be hard to do 100% consistently (we don't model the "transaction seen but unconfirmed" state anymore) and the previous behavior was to ignore this anyway I'm not including this here.