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

feat!: support decoding multiple ABIs at the same time, including ds-note library logs #757

Merged
merged 62 commits into from
Jul 27, 2022

Conversation

banteg
Copy link
Contributor

@banteg banteg commented May 28, 2022

What I did

  • if no abi is provided to Receipt.decode_logs, it would fetch all contract types that have emitted logs and decode all events. undecoded logs are returned as is.
  • add decoding of ds-note which is commonly found in MakerDAO contracts

How I did it

added ds-note decoding to ecosystem api and added a fallback to it in the receipt api. this is very cheap, it can deduce whether it's a note from byte padding in the topic before fetching any contract types. then it finds a function which matches the selector and decodes the data based on the function abi.

How to verify it

list(chain.provider.get_transaction('0x460cb2476a7a0e8e1933d935a1c386851d5c007e957dbf9bd2e32a8494b38e9d').decode_logs())

See also my similar pull request in brownie.

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

src/ape/api/transactions.py Outdated Show resolved Hide resolved
src/ape/api/transactions.py Outdated Show resolved Hide resolved
src/ape/api/transactions.py Outdated Show resolved Hide resolved
src/ape/api/transactions.py Outdated Show resolved Hide resolved
@banteg banteg marked this pull request as draft June 6, 2022 23:56
@banteg banteg marked this pull request as ready for review July 7, 2022 00:56
@banteg banteg requested a review from antazoey July 7, 2022 00:58
@banteg
Copy link
Contributor Author

banteg commented Jul 7, 2022

ready for review

fubuloubu
fubuloubu previously approved these changes Jul 7, 2022
src/ape_ethereum/ecosystem.py Outdated Show resolved Hide resolved
antazoey
antazoey previously approved these changes Jul 7, 2022
Copy link
Contributor

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

My only real concern is the potential AttributeError, but looks good!

src/ape/api/transactions.py Outdated Show resolved Hide resolved
src/ape_ethereum/ecosystem.py Outdated Show resolved Hide resolved
src/ape/api/transactions.py Outdated Show resolved Hide resolved
src/ape/api/networks.py Outdated Show resolved Hide resolved
src/ape_ethereum/ecosystem.py Outdated Show resolved Hide resolved
@antazoey antazoey requested a review from fubuloubu July 25, 2022 13:30
@antazoey
Copy link
Contributor

antazoey commented Jul 25, 2022

[Doing QA with @dtdang and wearing that hat right now]

Tried to follow the How to verify it section but doing this:

ape console

list(chain.provider.get_transaction('0x460cb2476a7a0e8e1933d935a1c386851d5c007e957dbf9bd2e32a8494b38e9d').decode_logs())

and it hangs for 2 minutes before timing out.

I realize now that I am supposed to use mainnet. However - I think the default timeout should be smaller for local networks. Thoughts?

  • Make timeout smaller for local networks

@fubuloubu
Copy link
Member

I think the default timeout should be smaller for local networks. Thoughts?

Yes, I think this makes sense

@antazoey
Copy link
Contributor

Yes, I think this makes sense

Done! https://github.com/ApeWorX/ape/pull/911/files
I made it a separate PR as it was a bigger refactor than expected.

@antazoey antazoey changed the title feat: receipt events and ds-note decoding feat!: support decoding multiple ABIs for event at the same time, including ds-note library logs Jul 26, 2022
@antazoey antazoey changed the title feat!: support decoding multiple ABIs for event at the same time, including ds-note library logs feat!: support decoding multiple ABIs at the same time, including ds-note library logs Jul 26, 2022
@@ -305,13 +305,13 @@ def encode_transaction(
"""

@abstractmethod
def decode_logs(self, abi: EventABI, raw_logs: List[Dict]) -> Iterator[ContractLog]:
def decode_logs(self, logs: List[Dict], *events: EventABI) -> Iterator["ContractLog"]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the variadic argument feels a bit weird here

Copy link
Contributor

Choose a reason for hiding this comment

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

@fubuloubu wanted it and I was hesitant at first, but I kind of like how it feels by not having to wrap single ABIs in a list first.

But I can go either way still, let's reach a consensus

Copy link
Member

Choose a reason for hiding this comment

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

It's a breaking change anyways, live life on the edge 🤘

src/ape_ethereum/ecosystem.py Outdated Show resolved Hide resolved
src/ape_ethereum/ecosystem.py Show resolved Hide resolved
@banteg
Copy link
Contributor Author

banteg commented Jul 26, 2022

wen merge

@antazoey
Copy link
Contributor

antazoey commented Jul 26, 2022

  • Currently experiencing weird hanging behavior,, Will check box once resolved

When using Hardhat, it makes infinite requests to eth_getLogs

Edit: Ah okay, I am literally checking the entire blockchain because I am using mainnet-fork.

But literally this command will check the entire blockchain for logs of a single event: all_logs = [log for log in contract_instance.NumberChange].

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

What's with the use of snake casing everywhere?

src/ape/types/__init__.py Outdated Show resolved Hide resolved
src/ape/utils/misc.py Outdated Show resolved Hide resolved
@antazoey antazoey merged commit f87d2a1 into ApeWorX:main Jul 27, 2022
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.

None yet

3 participants