fix uniswap trades #4699
fix uniswap trades #4699
Conversation
|
Workflow run id 6668424342 approved. |
|
Workflow run id 6668424160 approved. |
|
Workflow run id 6668508388 approved. |
|
Workflow run id 6668508538 approved. |
|
Workflow run id 6668625737 approved. |
|
Workflow run id 6668625781 approved. |
|
Workflow run id 6668715133 approved. |
|
Workflow run id 6668715038 approved. |
|
Workflow run id 6668740704 approved. |
|
Workflow run id 6668740709 approved. |
this error means the seed file in dbt doesn't exist to run the test against. here are some helpful things to know in the setup:
in order to force the seed to build, you can make a minor modification to the seed file in this PR for CI test to build it out for downstream usage. you can add a newline or some other similar dummy change, to force into PR. can you give that a shot, see if it gets past that error? |
|
as for the change being made in the PR, that's an interesting find. my initial thought is to be a bit careful, as there are quite a few other spells which are forks of uniswap v2, that use the same logic. i'd imagine we want to universally update the logic in order to have we are actually in the process of a i wonder if we wait to make changes there? here is the PR, for reference |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
Workflow run id 6775491831 approved. |
|
I have read the CLA Document and I hereby sign the CLA |
|
Workflow run id 6775547224 approved. |
|
Workflow run id 6775547312 approved. |
|
@jeff-dude thoughts on |
this may have been just bad timing, as prod is going through a refresh and you may have run into when that spell was being refreshed. lets try to simply rerun it first |
|
Thanks for the input @jeff-dude. Although other projects are probably even less affected by this than uniswap is, I agree the suggestions here should be incorporated in #4533 (and/or #4730). I can submit PRs to those branches with the relevant changes, or we can wait until those are merged and only then incorporate the suggestions from this PR? Or did you have something else in mind? |
the changes in #4533 will be in "beta" until the entire edit: |
This reverts commit cee04c7.
|
Workflow run id 6809310977 approved. |
|
Workflow run id 6809311103 approved. |
|
Workflow run id 6809441380 approved. |
|
Workflow run id 6809441508 approved. |
Yea, in that case I'd suggest going ahead with the fix as it impacts analyses like LVR computation which require knowledge of all tokens bought/sold by the pool
Agreed. And to #4730 too (cc @0xBoxer). Let me know how you guys would like to coordinate on that front |
|
@jeff-dude just wanted to confirm when this is merged will the changes propagate to all queries/spells that depend on this, like dex.trades? Or will the changes by default only be applied to new data as it comes in? |
yes, anything downstream will be refreshed along with these |
| @@ -5,7 +5,7 @@ | |||
| materialized = 'incremental', | |||
| file_format = 'delta', | |||
| incremental_strategy = 'merge', | |||
| unique_key = ['block_date', 'blockchain', 'project', 'version', 'tx_hash', 'evt_index'], | |||
| unique_key = ['block_date', 'blockchain', 'project', 'version', 'tx_hash', 'evt_index', 'token_bought_address', 'token_sold_address'], | |||
There was a problem hiding this comment.
adding two new unique keys is concerning, as we use the exact same set of unique keys in all projects that feed into dex.trades and it has worked well universally. why are we adding two more here?
There was a problem hiding this comment.
This is needed because this new implementation potentially decodes the swap event into two records - token0 for token1 and token1 for token0. See failed checks right before this commit for more context.
An alternative approach would be to aggregate those two records into a single one, but it would make the code quite a bit more complex/verbose (we'd have to figure out what is the token sold/bought based on the net amounts in/out of tokens 0/1) and potentially cause issues with negative values (still need to confirm this), so that's the tradeoff
There was a problem hiding this comment.
It does seem that the alternative approach I mentioned would result in negative values. Here's an example of an unusual swap event (index 128) :
amount0In : 659774361155999
amount1In : 250390
amount0Out : 100000355
amount1Out : 0
The net amounts are such that the pool's balance of both tokens increases. An heuristic that tried to infer the tokens sold/bought would result in both tokens as token_sold (ie sold by the trader to the pool). Still unclear to me why this happens, but I assume it's something along the lines of someone having transferred token0 to the pool before the actual trade occurred, and the Swap event just catching up with the state.
There was a problem hiding this comment.
I've added the two new keys to the dex.trades schema in 1b9c1fb
|
can you rebase with main branch & pull in the new uniswap macro for dex redesign, make same changes there? |
|
Workflow run id 6901624651 approved. |
|
Workflow run id 6901625001 approved. |
|
Workflow run id 6901840923 approved. |
|
Workflow run id 6901841044 approved. |
|
Workflow run id 6901908334 approved. |
|
Workflow run id 6901908775 approved. |
8559930, should be ready for review |
|
@markusbkoch thanks again for raising this issue. i think this has a larger impact across spellbook than anticipated, so i've linked a gh issue which outlines the bug found. let's focus conversation there until we come to an agreement, then we can revisit this PR and finalize. |
Swap events contain amounts in and out for both tokens 0 and 1, so we actually have two "swaps" in each event:
Most of the time, one of these swaps has empty/zero amounts, ie the trader is only swapping one token for another. However, there are some edge cases where both amounts for one of the tokens is non-zero, and both amounts for the other token are zero. eg 0x2643cc80871c4134704863b17f10901ea6d11646fc10162ff1fb504e7137c193, evt index 75
In other cases, both amounts for both tokens are non-zero eg 0x5afc81f74fbfca62c7d083cccbadd01158f65bfdb1f2c7fb24e4866c1d96e346, evt index 27
As this query shows, dex.trades only partially reflects the information present in the swap events mentioned above. This happens because the current implementation of
uniswap_v2_ethereum_trades.sqlassumes there is only one swap contained in the Swap event: ifamount0Out == 0the code assumes token 1 is being bought, else token 0 is being bought.This PR addresses this limitation by extracting both swaps from the Swap event and discarding the "empty swap" that is present in the vast majority of cases.
While events like the ones in the transactions mentioned here are very much the exception, they can have significant impact in some analyses. For example, the data regarding the swap txn
0x2643cc...would lead to believe that the pool sold 50M USDC for 0 WETH when in reality the pool sold 50M USDC for 50.1M USDC.cc @mendesfabio @viniabussafi @thetroyharris for comments/review