Add unlock feature#1232
Conversation
zaelgohary
left a comment
zaelgohary
left a comment
There was a problem hiding this comment.
Inline review below. On-device check: built on a physical device with the locked-tokens flag forced on and the card pointed at a real address holding 15 time-locked TFT escrows (~1,254,188.75 TFT) — the listing renders correctly. The unlock action was not exercised (irreversible mainnet), so unlock-path notes are code-review only. Severity assumes the flag-gated, disabled-by-default rollout.
Verified correct (not flagged): preauth-tx submission without an extra signature, minTime units (seconds), ChangeTrust('0') trustline removal, the fee/sequence model, and the flag wiring (safe false fallback).
| if (lockedToken.amount > 0) { | ||
| builder.addOperation( | ||
| PaymentOperationBuilder( | ||
| keyPair.accountId, asset, lockedToken.amount.toStringAsFixed(7)) |
There was a problem hiding this comment.
🔴 Amount precision (unlock path). amount is a double (parsed at L65 via double.parse) re-serialized here with toStringAsFixed(7). Stellar amounts are 7-dp fixed point; a double represents every stroop exactly only up to 2^53 = 900,719,925.47 TFT. Above that this string can differ from the escrow's real balance → leftover dust makes the later ChangeTrust('0') fail, or overpay makes the Payment op_underfunded; either way the whole drain tx reverts and the balance can't be unlocked. Carry the exact balance string end-to-end (never double) and re-read the live balance at drain time.
| ); | ||
|
|
||
| builder.addOperation( | ||
| AccountMergeOperationBuilder(keyPair.accountId) |
There was a problem hiding this comment.
🔴 AccountMerge needs the escrow's HIGH threshold. All three ops are sourced from the escrow, but the tx is signed only by the main key (L223). Payment/ChangeTrust are medium-threshold; AccountMerge is high. Nothing verifies the main key's signer weight on the escrow meets the high threshold → op_bad_auth reverts the whole drain. AccountMerge also requires zero remaining sub-entries, so any extra signer/trustline left on the escrow blocks it. Worth validating against the real escrow setup before enabling unlock.
| if (lockedToken.unlockHash != null) { | ||
| final submitted = await _submitUnlockTransaction(lockedToken); | ||
| if (!submitted) continue; | ||
| lockedToken.unlockHash = null; |
There was a problem hiding this comment.
🔴 Two-phase unlock isn't atomic and the error is swallowed. TX1 (_submitUnlockTransaction) is irreversible — it consumes the preauth_tx signer; if TX2 (_transferLockedBalance, L160) then throws, the catch … continue at L164 drops the token, so the UI reports failure while on-chain the escrow is unlocked-but-undrained. Recoverable on retry, but misleading. Distinguish 'couldn't unlock' from 'unlocked but transfer failed — retry to claim', and report per-escrow outcomes instead of swallowing.
| } | ||
|
|
||
| final data = jsonDecode(response.body); | ||
| final String xdr = data['transaction_xdr']; |
There was a problem hiding this comment.
🟠 A token-service hiccup hides real funds. No null check on data['transaction_xdr'], and any non-200/transport error throws → _getLockedTokenDetails returns null (L112) → the escrow is silently dropped from the list (the card may even render empty). A transient tokenservices.threefold.io outage makes locked balances disappear with no error shown. Prefer surfacing the token as 'locked, unlock time unknown' rather than dropping it.
| amount: amount, | ||
| unlockHash: unlockHash, | ||
| unlockFrom: minTime, | ||
| canBeUnlocked: minTime != null && nowSeconds >= minTime, |
There was a problem hiding this comment.
🟠 Preauth tx with no minTime shows as permanently locked. canBeUnlocked = minTime != null && … → if the unlock tx has no lower time bound (claimable immediately), this is false forever and the button never enables. The immediate-claim path is only handled when there is no preauth signer at all (the unlockHash == null branch).
| 'https://tokenservices.threefold.io/threefoldfoundation'; | ||
|
|
||
| /// Asset codes we consider when looking for locked balances. | ||
| const List<String> _allowedAssetCodes = ['TFT', 'TFTA', 'FreeTFT']; |
There was a problem hiding this comment.
🔵 Only TFT is supported — narrow this to ['TFT']. Keeping TFTA/FreeTFT means the scanner can pick them up, and an escrow holding TFT plus another allowed asset would leave a second trustline so AccountMerge (L217) fails (the multi-asset hazard). Restricting to ['TFT'] removes that and makes the hardcoded 'Locked TFT' label and _totalLocked sum correct.
| } | ||
| } | ||
| logger.i( | ||
| '[LockedTokens] Escrow ${account.accountId}: ${balance.balance} ${balance.assetCode}, ' |
There was a problem hiding this comment.
🔵 PII in logs. This (plus L23 and L178) logs the wallet's Stellar address, each escrow accountId, and balances at logger.i — these ship in release logs and expose holdings/identity. Drop to debug or redact. (Good: the secret is correctly never logged.)
| /// Submits the stored unlock transaction to the Stellar network. Returns `false` | ||
| /// when the time-lock has not expired yet. | ||
| Future<bool> _submitUnlockTransaction(LockedToken lockedToken) async { | ||
| final unlockTx = await _fetchUnlockTransaction(lockedToken.unlockHash!); |
There was a problem hiding this comment.
🔵 Redundant network fetch on the sensitive path. This re-fetches the unlock tx already retrieved during listing (_getLockedTokenDetails → _fetchUnlockTransaction); minTime/unlockFrom is already on the model. Reuse it instead of a second POST per escrow.
| '[LockedTokens] Escrow ${account.accountId}: ${balance.balance} ${balance.assetCode}, ' | ||
| 'unlockHash=${unlockHash ?? 'none'}'); | ||
|
|
||
| final detailed = await _getLockedTokenDetails( |
There was a problem hiding this comment.
🔵 N+1, sequential. One token-service POST per time-locked escrow, awaited inside this loop (which runs from the card's initState). With many escrows the Assets screen blocks for forSigner + K serial round-trips, and one slow escrow stalls the rest. Run them concurrently with Future.wait.
| Widget build(BuildContext context) { | ||
| // Hide the section entirely when there is nothing locked. | ||
| if (!_loading && _lockedTokens.isEmpty) { | ||
| return const SizedBox.shrink(); |
There was a problem hiding this comment.
🟡 'Locked' section flashes on every wallet open. build() only short-circuits when !_loading && isEmpty, so while loading it always renders the Divider + 'Locked' header + spinner — every wallet, every open — then collapses for the common no-locked case (layout jump). Gate the header on loaded and non-empty, and consider caching so it doesn't re-fetch on every entry.
- Drain escrow using its exact live balance string instead of a re-serialized double, avoiding stroop drift that could revert the drain transaction - Surface escrows as "locked, unlock time unknown" on token-service failures instead of silently dropping them; validate transaction_xdr in the response - Treat a preauth tx with no minTime as claimable immediately - Paginate forSigner so wallets signing >200 accounts are not truncated - Restrict locked assets to TFT to avoid multi-asset AccountMerge failures - Log address/escrow/balance details at debug level to keep PII out of release - Resolve escrow unlock details concurrently instead of serially - Report per-escrow unlock outcomes so an unlocked-but-undrained escrow tells the user to retry rather than reporting an outright failure - Hide the Locked section until loaded and non-empty to stop header flashing
2 participants
No description provided.