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.]
Merge request reports
Activity
added 2 commits
Thank you for mentioning this! Linking the code in question--
https://gitlab.com/thorchain/thornode/-/blob/v1.97.2/x/thorchain/handler_add_liquidity.go#L427-435
--If setting them to Available, doing so on creation is certainly more pleasing than spending a cycle Staged first, unless including Staged-dependent bucket behaviour.(Perhaps adding a
&& !pool.Asset.IsSyntheticAsset()to theiffor a lateraddLiquidityV.)
[Edit: Now added!]
[Second edit: Something to consider: If a bucket is Available,withdrawV91might try to implement Impermanent Loss Protection?
[Third edit: No, I'm reassured to see thatIsVaultAssetis already checked for there.](Separately, I seem to recall that there was code handling
CalcUnitsbehaving differently for buckets, but I cannot find it at the moment.)
[Second edit: Found it,calculateVaultWithdrawV1called here.
Adding a concern about this to the main description--]
[Fifth edit: --and new commits now added, intended to address this.]
Fourth edit:
Ah, and I had also forgotten that synth buckets are only possible for gas asset pools.
I'm wondering somewhat now about what happens when a THORChain address tries to stake synths.
(Currently not allowed without swapping them for Layer 1 asset first?)Edited by Multipartite
added 6 commits
Toggle commit listmentioned in merge request !2597 (merged)
This is quite large and the critical thing we definitely need in 1.98 is the pool status and cycle fix so I've cherry picked those into a separate PR here so we can get them in sooner: !2597 (merged).
I see there are a couple other tweaks to add/withdraw LP logic here, but I defer review of that to someone with more depth on the LP calculations. I think in current form this PR might be too daunting to get the eyes it needs, so I would suggest updating or recreating with a concise description and dropping the refactoring on the
withdrawfiles (in general we should separate cleanup from logic changes), and then we can request review from SoO or Heimdall without requiring too much of their time to sort through it.-
Thank you for this feedback--assuming I have time today, I will aim to fragment it into parallel rather than sequential commits.
(The use of LPUnits rather than PoolUnits does look critical to me for bucket behaviour.)
Edit: !2599 (closed) now created for that; closing this !2579 (closed) in favour of !2597 (merged) and !2599 (closed).
|
Second edit: I have now created !2600 (merged) (negligible importance) to be a cleanup-specific vessel for the refactoring parts of this MR.Edited by Multipartite
