This (overgrown) MR has been closed in favour of the more-specific !2597 (merged) and !2599 (closed) (and of lesser importance !2600 (merged)).
[Second edit: Following the edits recorded below, at present I am changing this from Draft to Ready.
Synth buckets will still (for now) exist without being destroyed even if emptied of synths,
but since they are both on-chain (not requiring migrate costs) and only for gas assets I do not feel a pressing need for this to be altered in this Merge Request.]
In POL testing, gavmcd noticed that a Stagenet experimental synth bucket was destroyed for having no RUNE balance.
https://stagenet-thornode.ninerealms.com/thorchain/buckets?height=3685679
Existent.
https://stagenet-thornode.ninerealms.com/thorchain/buckets?height=3685680
Non-existent.
https://stagenet-midgard.ninerealms.com/v2/debug/block/3685680
Suspended.
Draft-relevant point 1: ETH/ETH balance_asset was 107010 before suspension, but the block 3685680 "withdraw" event's emit_asset was 0 and no synths were burnt in a fee-related way. I think this bears further consideration.
[Edit: Following removeLiquidityProviders, this is intended to emit a "withdraw" event with all zeros except for the liquidity units, which in the example match what is expected. While I would like to review the code which handles a bucket's unit calculation/alteration, I am reassured that skipping cyclePools for synth buckets is sufficient to avoid the effect of removeLiquidityProviders, and (at present) this effect a greater reason for skipping.]
Draft-relevant point 2: According to this MR's changes at time of writing, a synth bucket would remain Staged for the entirety of its existence. I do not see a problem with this, but if preferred to be set to Available in its first cyclePools that should be possible.
[Edit: Code now added in addLiquidityV98 to set bucket status to Available on creation.]
Draft-relevant point 3: It seems preferable for a bucket to be deleted when it no longer contains any synth, but the comments on deletion of a normal pool warn the below.
// check if the rune balance is zero, and asset balance IS NOT
// zero. This is because we don't want to abandon a pool that is in
// the process of being created (race condition). We can safely
// assume, if a pool has asset, but no rune, it should be
// abandoned.As I do not know what race condition to lead to a pool otherwise being abandoned in the process of creation (a pending asset state?),
I ask that someone who does know confirm whether such a race condition could happen for a synth bucket (being subject to deletion in the process of being created).
[Edit: As noted for point 1 above, removeLiquidityProviders is designed to emit nothing; if a bucket were to be destroyed using removeLiquidityProviders, care should be taken that this was definitely only done when there were no contained synth that should be withdrawn (or burnt).]
[Third edit: My current impression is that the race condition is if a pool cycle occurs when a pool has a pending Asset or RUNE which has not yet been matched, and so has not yet become BalanceAsset or BalanceRune; buckets I believe are expected to never have a pending coin, being single-sided only.]
Draft-relevant point 4: Though unrelated to point 1, a concern about withdraw amounts:
https://gitlab.com/thorchain/thornode/-/blob/v1.97.2/x/thorchain/withdraw_current.go#L107-108
When a withdraw is from a synth bucket (rather than a Layer 1 pool), calculateVaultWithdrawV1 is used with pool.GetPoolUnits() as the first argument.
GetPoolUnits returns the sum of a pool's/bucket's LPUnits and SynthUnits.
Earlier in withdrawV91, CalcUnits calls CalcUnitsV80, which calculates and sets SynthUnits.
|
My current impression is that this code always treats a synth bucket as having a large amount of SynthUnits (and thus PoolUnits) relative to LPUnits, since referring to the entire SynthSupply of the asset type (whether in the bucket or not).
|
Returning to calculateVaultWithdrawV1, it refers to its first argument as 'vaultUnits' and calculates the asset amount to withdraw with withdrawAsset := common.GetSafeShare(unitsToClaim, vaultUnits, assetAmt)
, which I currently predict to only withdraw half of what the synth staker is entitled to, since 'unitsToClaim' is calculated from 'lpUnits' (the third argument of calculateVaultWithdrawV1, 'originalLiquidityProviderUnits' in withdrawV91).
|
There is a similar concern for when receiving units for adding liquidity,
namely whether a new staker would receive a disproportionate amount of units relative to previous stakers.
[Second edit: I have now added commits intended to resolve this.]