Add liquidity and /pools endpoint do not reflect zero-LP_units pools
As synths exist, it is now possible for all liquidity providers in an L1 pool to withdraw their full positions,
leaving the pool with no LP_units and yet still have non-zero balance_asset and balance_rune depths backing synths.
At present my impression is that swaps can still take place through these pools.
This Issue is prompted by this Discord thread.
Summarising from it and other thoughts:
https://stagenet-thornode.ninerealms.com/thorchain/pools?height=6833700
Stagenet has a number of L1 pools (ones not beginning with THOR.).
For solvency reasons, it can be desirable to check that the L1 pools' balance_rune and pending_inbound_rune sums are not greater than the
https://stagenet-thornode.ninerealms.com/thorchain/balance/module/asgard?height=6833700
'rune' balance.
In that endpoint there is no BNB.BNB (or LTC.LTC) pool, but
https://stagenet-thornode.ninerealms.com/thorchain/pool/BNB.BNB?height=6833700
has 0 LP_units and non-zero balance_asset and balance_rune.
Further, the "rewards" event of
https://stagenet-midgard.ninerealms.com/v2/debug/block/6833700
shows that both BNB.BNB and LTC.LTC pools are receiving RUNE block rewards.
This I feel is good reason to remove the chunk of code in queryPools that hides pools with zero LP_units.
|
Particularly, removing it entirely (thus leaving pending RUNE visible for solvency confirmation) rather than only adding more conditions.
|
[Edit: That, or adding specific checks for balances and pending amounts.]
|
[Second edit: Smoke test troubleshooting remains necessary, however.]
|
[Third edit: As explored in more detail in #1509 (with a regression test), THORChain full (gas asset) pool withdrawal is broken, and letting the querier display pools with zero LP_units exposes this to smoke tests (since a smoke test does a full gas asset pool withdrawal).]
|
[Fourth edit: #1509 now seemingly being resolved, for this part of #1506 I currently propose these commits, passed pipeline here.]
There is also no THOR.BNB pool displayed in the above /pools endpoint, making Lending analytics more inconvenient, as there is THOR.BNB in the Stagenet Lending module and a THOR.BNB pool (with zero LP_units and non-zero simulated balances) which could otherwise be more easily used for analytics.
For historical context, it was Stagenet block 3685030 (compare to one block earlier) when the Stagenet BNB.BNB pool first reached zero LP_units, with the only balances at the time backing its synth_supply (synth_supply backed by half its amount in balance_asset as well as an equivalent depth percentage in RUNE, then being 100%).
Beyond this, if a new liquidity provider attempts to add liquidity to the pool (expecting it to again have units), they find the pool swallows their added liquidity, leaving them with no units nor asset_deposit_value nor rune_deposit_value.
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L408
calculatePoolUnitsV98(oldPoolUnits
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L418
P := cosmos.NewDecFromBigInt(oldPoolUnits.BigInt())
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L428
numerator := P.Mul(cross.Add(cosmos.NewDec(2).Mul(r).Mul(a)))
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L435-436
liquidityUnits := numerator.Quo(denominator)
newPoolUnit := P.Add(liquidityUnits)Summary: The old total pool units are 0, so the added pool units are 0, and the new total pool units are 0.
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L641
su.RuneDepositValue = common.GetSafeShare(su.Units, pool.GetPoolUnits(), pool.BalanceRune)
Summary: Since the added units (and for that matter the pool units) are 0, the LPer's Deposit Value is also 0.
(That is, the pool entirely swallows the LPer's deposited coin/s.)
https://gitlab.com/thorchain/thornode/-/blob/v1.107.0/x/thorchain/handler_add_liquidity.go#L415-417
if poolRune.IsZero() || poolAsset.IsZero() {
return addRune, addRune, nil
}Summary: Current code handles creating new units for an empty pool (no LP_units or balances),
but not for a non-empty unitless pool.
Keeping the RUNE-scale initial unit consistency (while acknowledging that for a non-empty pool the add liquidity may be from a single address), I currently propose code similar to the below.
if poolRune.IsZero() || poolAsset.IsZero() || oldPoolUnits.IsZero() {
sUnits := addRune
if addRune.IsZero() {
if !poolAsset.IsZero() {
// Where possible, keep a RUNE scale for new units.
sUnits = addAsset.Mul(poolRune).Quo(poolAsset)
} else {
sUnits = addAsset
}
}
return oldPoolUnits.Add(sUnits), sUnits, nil
}(For example these commits, with this passed pipeline.)
The oldPoolUnits.Add(sUnits) is intended to be unnecessary, but to not discard a pool's prior LP_units information if somehow it temporarily had a zero balance while having non-zero LP_units.
As always feedback is welcome.
2024-05-17:
13 months after fix !2865 (merged)
'Do not swallow add liquidity to a non-empty zero-units pool'
was submitted for this (remaining in the queue unmerged),
a Mainnet user has encountered this failure state,
adding 8,230 RUNE and ~16.55 ETH.WSTETH to the ETH.WSTETH pool
which swallowed the liquididity.
58E0CC3EE396EE54293D88271179E15CC89407122538DCC2AB1823618EBA40C1 (THOR height 15984984)
533C05304F0AA20BC488C2F89FBB98C303DCD2DC9B5290A5A5B8B5078D97A7FC (THOR height 15985003)https://thornode.ninerealms.com/thorchain/pool/ETH.WSTETH-0X7F39C581F595B53C5CB19BD0B3F8DA6C935E2CA0?height=16004440
https://thornode.ninerealms.com/thorchain/pool/ETH.WSTETH-0X7F39C581F595B53C5CB19BD0B3F8DA6C935E2CA0/liquidity_providers?height=16004440
Discord link:
https://discord.com/channels/838986635756044328/839002288436805683/1240938989129830450
As this has now affected a Mainnet user
(loss of funds unless refunded through Treasury or store migration)
I increase the Priority label from P3
P3 = feature request or other new business
to P0
P0 = loss of funds, chain downtime, etc.
.