Conversation
fhenneke
left a comment
fhenneke
left a comment
There was a problem hiding this comment.
I added some comments.
Functionality looks good.
| for token_address, imbalance in token_imbalances.items(): | ||
| if imbalance != 0: | ||
| transaction_tokens.append((tx_hash, token_address)) | ||
| for token_address in token_imbalances.keys(): |
There was a problem hiding this comment.
I would reorganize code here.
Instead of fetching token_imbalances in the beginning, and then having this loop here, both should be moved into a self.imbalances.get_transaction_tokens function. That function would then be called here. The function self.blockchain_data.get_transaction_tokens should then be deleted.
There was a problem hiding this comment.
As it is still not clear where and how the raw imbalances will be used, i decided to keep it separate and not part of a get_transaction_tokens function.
Rearranged a few things and modified the test as well (not sure if you like how it looks like), in order to check the current version of the code. Ended up removing the test as it wasn't easy to make it work
There was a problem hiding this comment.
One reason for separating functionality into functions is that it makes it possible to test things.
Moving a function into the main body and removing tests is not ideal.
I can handle moving the function into token imbalances if you do not have the capacity.
There was a problem hiding this comment.
Ok i am attempting to do this now
| f"postgresql+psycopg://postgres:postgres@localhost:5432/mainnet" | ||
| ) | ||
| db = Database(engine, "mainnet") | ||
| # list contains duplicate entry in order to test how this is handled |
There was a problem hiding this comment.
I would make this a separate test test_write_prices_duplicates. Otherwise we might get stuck with complicated tests as in solver-rewards where we have one huge test case with lots of settlements. It then becomes difficult to add new cases and a failed test does not show why it failed.
2 participants
This PR addresses the following issues:
Currently, we only fetch prices for tokens for which there is a non-zero imbalance. Due to network/protocol fees being implicit here, this is not enough as we might end up with slippage on tokens whose raw token imbalance is zero. With this PR, we fetch a price for all tokens for which there was a transfer to or from the settlement contract.
When multiple txs happen at the same block, the write_prices function that writes price entries in the database sometimes attempts to write a row that already exists in the db (e..g, when WETH is traded in both txs). This PR proposes to first check if the entry exists, and only proceed with writing if the entry is non-existent in the db.