feat: implemented clementine deposit approval#3204
Conversation
bernd2022
left a comment
There was a problem hiding this comment.
Review: Clementine Deposit Approval
Guter Ansatz — die manuelle Genehmigung vor dem Senden von 10 BTC ist ein wichtiger Sicherheitsmechanismus. Ein paar Punkte:
🔴 Double-Spend-Risiko in checkDepositCompletion
In der aktuellen Implementierung gibt es folgendes Crash-Szenario:
isDepositApproved()→truesendBtcToAddress()sendet 10 BTC erfolgreich- Prozess crasht bevor
orderRepo.save()incheckOrder()ausgeführt wird - Nächster Cron-Run:
btcTxIdist nochundefined(alter Wert aus DB), Adresse ist noch im Setting - → 10 BTC werden erneut gesendet
Vorschlag: removeDepositApproval vor sendBtcToAddress aufrufen:
// Remove approval FIRST to prevent double-send on crash
await this.removeDepositApproval(correlationData.depositAddress);
// Now safe to send BTC
const btcTxId = await this.sendBtcToAddress(correlationData.depositAddress, CLEMENTINE_BRIDGE_AMOUNT_BTC);Falls der Prozess dann zwischen Remove und Send crasht, wurde kein BTC gesendet und der Operator kann einfach erneut approven. Das ist der sichere Failure-Mode.
🟡 Veralteter Kommentar (Correlation-ID-Format)
Der Kommentar in Zeile 38-39 beschreibt noch das alte Format:
* - Deposit: clementine:deposit:{depositAddress}:{btcTxId}
Das Format ist jetzt clementine:deposit:{base64-encoded-json}. Bitte aktualisieren, analog zum Withdraw-Kommentar.
🟡 Kein Timeout für Approval-Wartezeit
Wenn die manuelle Genehmigung nie erteilt wird, bleibt die Order dauerhaft IN_PROGRESS. Beim Withdraw gibt es OPTIMISTIC_TIMEOUT_MS (12h). Wäre ein ähnlicher Timeout (oder zumindest ein Warn-Log nach X Stunden) sinnvoll?
Release Checklist
Pre-Release
Post-Release