services/horizon: Liquidity pool effects only use the first change for an operation

[paulbellamy]

What did you do?

Look at the effects on this operation.

Note that there are two trades through liquidity pool 15ffc747eb21ffbb6b4003e6f52a3ed2151d7013693f3a26ed6e3e77ad77a099. From Even -> Aqua -> Even.

For both effects, the liquidity pool reserves are shown as:

"reserves": [
  {
    "asset": "AQUA:GBNZILSTVQZ4R7IKQDGHYGY2QXL5QOFJYQMXPKWRRM5PAV7Y4M67AQUA",
    "amount": "169.7936447"
  },
  {
    "asset": "EVEN:GAEZ5U33KLLDEDH5XUS4H4BNB2YTOMOW3ED7YZXSKRHYOARLOJXX35MJ",
    "amount": "23.9653275"
  }
]

But that's only the reserves for the first operation.

What did you expect to see?

I expected the second trade through the liquidity pool to have different reserves.

Specifically, the reserves for the second trade on that liquidity pool should be:

"reserves": [
  {
    "asset": "AQUA:GBNZILSTVQZ4R7IKQDGHYGY2QXL5QOFJYQMXPKWRRM5PAV7Y4M67AQUA",
    "amount": "-171.4861544"
  },
  {
    "asset": "EVEN:GAEZ5U33KLLDEDH5XUS4H4BNB2YTOMOW3ED7YZXSKRHYOARLOJXX35MJ",
    "amount": "39.9846339"
  }
]

Note: This txn temporarily withdraws more reserves than are available in the liquidity pool, but because claim atoms for strict-receive operations are processed backwards along the path it is allowed.

What did you see instead?

The reserves were the same. In this code in OperationsProcessor, we use the first change we find for that liquidity pool. This won't handle multiple trades through a pool in a single operation.

[paulbellamy]

To be fair, the "right" solution here is to also process liquidity pool claim atoms for strict receives in reverse order (like core does), which would keep the pool reserves from going negative. But that means big changes in the EffectsProcessor and TradesProcessor.

[paulbellamy]

Blocking #4178

[jonjove]

@paulbellamy and I have been investigating the “right” way to solve this problem. At this point we have reached the conclusion that it is impossible for the effects to satisfy all three of the following desirable properties:

• occur in path order (meaning if the path is A -> B -> A then the effects for A -> B precede the effects for B -> A
• trade amounts match the claim atoms
• reserves do not jump

Instead, it is a classic three options choose two scenario. I will now demonstrate the three possible choices. The trade amounts are the post-reserves minus the pre-reserves, where negative amounts are sells and positive amounts are buys.

Option 1: Path order, trade amounts match

EVEN -> AQUA
Pre:   {"AQUA":  5110734438, "EVEN":   79460211} // This isn't the initial state of the liquidity pool
Trade: {"AQUA": -3412797991, "EVEN":  160193064}
Post:  {"AQUA":  1697936447, "EVEN":  239653275}

AQUA -> EVEN
Pre:   {"AQUA":  1697936447, "EVEN":  238693558} // This isn't the intermediate state of the liquidity pool
Trade: {"AQUA":  3412797991, "EVEN": -159233347}
Post:  {"AQUA":  5110734438, "EVEN":   79460211}

Option 2: Path order, reserves match

EVEN -> AQUA
Pre:   {"AQUA": 1697936447, "EVEN":  238693558}
Trade: {"AQUA": 3412797991, "EVEN": -159233347} // Pool should be selling AQUA and buying EVEN
Post:  {"AQUA": 5110734438, "EVEN":   79460211}

AQUA -> EVEN
Pre:   {"AQUA":  5110734438, "EVEN":   79460211}
Trade: {"AQUA": -3412797991, "EVEN":  160193064} // Pool should be buying AQUA and selling EVEN
Post:  {"AQUA":  1697936447, "EVEN":  239653275}

Option 3: Trade amounts match, reserves match

AQUA -> EVEN                                     // EVEN -> AQUA happens first on the path
Pre:   {"AQUA":  1697936447, "EVEN":  238693558}
Trade: {"AQUA":  3412797991, "EVEN": -159233347}
Post:  {"AQUA":  5110734438, "EVEN":   79460211}

EVEN -> AQUA                                     // AQUA -> EVEN happens second on the path
Pre:   {"AQUA":  5110734438, "EVEN":   79460211}
Trade: {"AQUA": -3412797991, "EVEN":  160193064}
Post:  {"AQUA":  1697936447, "EVEN":  239653275}

Once it's decided which two of three properties will be preserved, the appropriate approach can be extended to any number of trades such as A -> B -> A -> B. Which leaves the critical question, which two of the three properties are most important?

[paulbellamy]

@sydneynotthecity suggests we prioritize the fields which cannot be looked up elsewhere. That seems like path ordering is the least important (can be looked up from the operation). But! the worst case is if you do ABABA trades. So, we could add a "path index" field (not sure on naming), to the effect, which would be the path ordering index of that trade. Then we could have all the data and have it all make sense.

[paulbellamy]

Unfortunately, fixing this probably implies a full reingestion, since it will change the generated effects for strict-receive txns.