pendulum-chain / pendulum-squids

The subsquid squids we use for Pendulum/Amplitude/Foucoco.
GNU General Public License v3.0
0 stars 0 forks source link

Fix for sorting pairs before querying the storage #56

Closed gianfra-t closed 6 months ago

gianfra-t commented 6 months ago

A bug was identified where the lookup of the pair given an event swap was not returning the pair, and therefore not indexing the swap.

Since getPairAssetIdFromAssets calls the storage, at this point we need to sort the assets the same way the zenlink pallet does.

The rest of the changes are to adapt the code so that even when we have to reorder the pairs, we are consistent with the asset0, asset1 naming and convention. For example, amount0In should always correspond to the input amount into the swap of the first asset after they are sorted.

Closes #57.

gianfra-t commented 6 months ago

I missed the existing sorting function, thanks! I am not entirely sure about the last changes. I know that the previous condition may not be pretty, but here we are always assuming that amount0Out will be 0, similarly with amount1In. This will work only for the cases were the swap is already ordered.

I am actually surprised to see that this is the case with the bifrost squid implementation. Perhaps for them, they can query a different pair for a switched order, which would make sense to hold this assumption. What do you think @ebma ?

ebma commented 6 months ago

I think it works because the amount0In and amount0Out (same for amount1) are mutually exclusive, meaning only one of them can have a value and the other will be 0. If you check here, when calling the extrinsic you specify amount_in and amount_out and depending on the order of assets in path it will either be amount_in for asset 0 or for asset 1, with the remaining asset being used for amount_out.

With this check we then determine which amount should be used for which token, so it should work. You could argue that the declaration of amount0Out and amount1In in our code is now useless as they will always be 0. Though I guess it's okay to keep it for illustrative purposes. We can remove the useless variables but I think the logic overall should work like this anyways.

gianfra-t commented 6 months ago

Okay so it seems then that like you say, it still will works. But it is a little bit confusing to me. If we look at the swap events for these two alternatives: image image we can see that the first one is telling us that DOT was the asset in, and PEN the asset out, which we know was the case from the actual transaction. This is with the version 18 uploaded yesterday.

On the other hand, in the latest iteration this is reversed. I checked in the remaining parts of the code and the only variable used is the total amount, so it doesn't break anything but the record itself.

ebma commented 6 months ago

Hmm you are totally right, thanks for elaborating on this in more detail.

I changed it back to something that should work, please have another look @gianfra-t.

gianfra-t commented 6 months ago

Alright, seems good! Thanks. I will bump the other versions because it should affect them also.

ebma commented 6 months ago

Let's bump all squids to the same version (in this case 18). Then it's easier to see in the cloud app which squid has which features.