Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reform eth transactions #3386

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

escottalexander
Copy link
Contributor

Utilize event logs for ERC20 transactions.

Copy link
Collaborator

@kajoseph kajoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't functionally tested yet, but it looks good from reading through the changes. Just a couple of requests

@tonik-ru
Copy link

tonik-ru commented Jul 5, 2022

we also added logs to db in our fork. its impossible to track balances without logs.
but why are you doing getTransactionReceipt for each tx?
this will kill performance, making ~100 additional calls to parity for each block.
also in getPastBlocks you could filter by topics.
and what about erc1155?
also i would suggest adding "loadLogs" in config. because logs are space intensive.

@escottalexander
Copy link
Contributor Author

escottalexander commented Jul 7, 2022

@tonik-ru I appreciate your thoughts.

I was able to remove the reliance on getTransactionReceipts. It just means we don't have event logs on txs found via subscription but I don't think this is a problem as those are replaced when the block is processed.

I like your idea about making the logs configurable. We might tackle that at a later date. Feel free to submit a PR.

The getPastLogs method only accepts a maximum of four topics so it would limit the results too much to use them. It's a good consideration though if anyone is just relying on the logs for one event type.

Currently we are only decoding the logs with a handful of ABIs. I don't think we are even using the ERC721 and Multisig events. I don't see much of a point to adding ERC1155 but if someone needed that functionality then it should be pretty easy for them to add it by following the "template" laid out by the existing ABIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants