Pending-liquidity zero-balance pools can hinder fuzzy matching
Issue-prompt (paraphrased from below):
An ETH.USDC swap succeeded while AVAX.USDC failed (and was refunded), because fuzzyAssetMatchV83
ignores the deep-RUNE-balance ID-present pool and only looks at the pending-liquidity-only pool that exactly matches the swap memo asset.
"=:AVAX.USDC:0x2702d89c1c8658b49c45dd460deebcc45faec03c:1223965400" (refunded)
"=:ETH.USDC:0x5e624A7Ad13b5c01d547B1A95A386D1f6147Bf56:47972455443" (succeeded)
|
The reason was "pool(AVAX.USDC) is not available", though the
AVAX.USDC-0XB97EF9EF8734C71904D8002F8B6BC66DD9C48A6E
pool was definitely available in that block (and held over 100,000 RUNE).
Context (this Discord thread):
In block 7615806 there was a THORChain-side add liquidity for pool 'AVAX.USDC'.
https://thornode-v1.ninerealms.com/txs/A71399C69EFB4C2B5D7337A0AF5AD7FAAF45D849F82C3B6481D27D40E9726204
"+:AVAX.USDC:0xe72fed3f02005a74e3acdb537eb027dffec896f9"
One block before, there was no AVAX.USDC pool with or without ID.
https://thornode-v1.ninerealms.com/thorchain/pools?height=7615805
This created a an 'AVAX.USDC' pool record with no ID and pending liquidity, able to pass fuzzyAssetMatchV83
's PoolExist
check.
https://thornode-v1.ninerealms.com/thorchain/pool/AVAX.USDC/liquidity_providers?height=7615806
The result is that while an ETH.USDC swap succeeds an AVAX.USDC fails (and is refunded), because fuzzyAssetMatchV83
ignores the deep-RUNE-balance ID-present pool and only looks at the pending-liquidity-only pool that exactly matches the swap memo asset.
"=:AVAX.USDC:0x2702d89c1c8658b49c45dd460deebcc45faec03c:1223965400" (refunded)
"=:ETH.USDC:0x5e624A7Ad13b5c01d547B1A95A386D1f6147Bf56:47972455443" (succeeded)
|
The reason was "pool(AVAX.USDC) is not available", though the
AVAX.USDC-0XB97EF9EF8734C71904D8002F8B6BC66DD9C48A6E
pool was definitely available in that block (and held over 100,000 RUNE).
I note that !2624 (merged) would not have caught this, as the AVAX.AVAX gas asset pool did exist when the IDless AVAX.USDC add liquidity took place.
I also note that even if a check were put in to insist on IDs for non-gas assets, an ID of a few characters could similarly get in the way of shortened memos.
I currently propose that either
{a new version of fuzzyAssetMatch
do a full check for the matching pool with deepest BalanceRune}
or alternatively, maybe preferable for performance, to do a direct existence check first with instead a GetPool
and if !pool.BalanceRune.IsZero()
(assuming that usually non-gas pools lacking legitimate IDs will not be able to reach the point of having non-zero depths, only pending liquidity).
UPDATE: When looking into a smoke test, a certain thing stood out.
https://gitlab.com/thorchain/thornode/-/blob/v1.98.1/x/thorchain/handler.go#L354-369
// if we found no matches, return the argument given
if len(matches) == 0 {
return origAsset
}
// find the deepest pool
winner := NewPool()
for _, pool := range matches {
if winner.BalanceRune.LT(pool.BalanceRune) {
winner = pool
}
}
winner.Asset.Synth = origAsset.Synth
return winner.Asset
If there are no matches then the unmatched asset is returned,
but if there is one (or more) match and it has zero BalanceRune, the function will return the empty (nil?) Asset of NewPool
rather than a match or origAsset.
I thus also favour simultaneously changing the LT
to an LTE
so that the returned asset is only ever origAsset or a matched Asset, never an empty Asset.