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

refactor!: Calltree to be generic #1040

Closed
wants to merge 6 commits into from
Closed

refactor!: Calltree to be generic #1040

wants to merge 6 commits into from

Conversation

NotPeopling2day
Copy link
Contributor

@NotPeopling2day NotPeopling2day commented Sep 7, 2022

What I did

When a non-EVM chain wishes to display traces, ape core should be able to accept a generic call tree. This PR makes a generic interface for call tree models.

fixes: ape-62

evm-trace will have to be released before this PR and the setup.py updated to reflect.
ApeWorX/evm-trace#16

How I did it

How to verify it

Checklist

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

@NotPeopling2day NotPeopling2day changed the title refactor: calltree to be generic refactor!: Calltree to be generic Sep 7, 2022
@NotPeopling2day
Copy link
Contributor Author

Failing test based on structure before evm-trace changes are released. However, this all works locally.

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.

Thinking out loud here:
My original interface ideas for ape core was to make it so printing a tree was just a method you could call, and however EcosystemAPI/ProviderAPI managed it under the hood would make it work.

The way I saw it, there were three things we needed to support:

  • display a trace (have the CallTree convert itself to a Tree)
  • fetch gas report information (Dict[<contract_type>, Dict[<method_id>, List[GasCost]])
  • fetch coverage information (Dict[<contract_type>, Dict[SourceId, Set[LineNo]])

Maybe worth having a conversation about this design again

@antazoey
Copy link
Member

antazoey commented Sep 8, 2022

Thinking out loud here: My original interface ideas for ape core was to make it so printing a tree was just a method you could call, and however EcosystemAPI/ProviderAPI managed it under the hood would make it work.

Your original idea also includes that it must use the original display from evm-trace when it is unable to properly decode the contract data. That is the tricky part and why the design is like this - needs to reference the original display abilities on a line by line basis.

Edit: Did some refactoring that at least should get things in the right direction while maintaining the behaviors.

@antazoey
Copy link
Member

antazoey commented Sep 8, 2022

Requires this PR: ApeWorX/evm-trace#17

@@ -26,6 +40,22 @@
_DEFAULT_INDENT = 2


class TreeLike(Protocol):
Copy link
Member

Choose a reason for hiding this comment

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

Wait, does protocol work with data model fields?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, it does not:

>>> from typing import Protocol
>>> class TreeLike(Protocol):
...     a: int
...     b: bytes
>>> class Tree(TreeLike):
...     pass
>>> t = Tree()
>>> t.a
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Input In [6], in <cell line: 1>()
----> 1 t.a

AttributeError: 'Tree' object has no attribute 'a'

Copy link
Member

Choose a reason for hiding this comment

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

It is not really supposed to

Copy link
Contributor Author

@NotPeopling2day NotPeopling2day Sep 8, 2022

Choose a reason for hiding this comment

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

It doesn't work the way you're using it, in that a model doesn't inherit from the protocol, but I do think ultimately this needs a rework. using the protocol could be a good way of enforcing some control over the interface without requiring users to subclass.
https://www.pythontutorial.net/python-oop/python-protocol/

Copy link
Member

Choose a reason for hiding this comment

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

evm-trace is not going to import a base model from ape, nor really the other way around.
Protocol is about typing / types / conformity.

I am open to other ideas but wanted to make sure the stage is set

Comment on lines +44 to +52
address: Any
calls: Iterable
calldata: Any
returndata: Any
failed: bool
call_type: Any
gas_cost: int
value: int
gas_limit: int
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Protocol will work quite right here

This might need a revisit...

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain more?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is getting registered as class variables, not fields

Copy link
Member

Choose a reason for hiding this comment

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

It does not need to be fields because it is not a base class of anything.

Look at this example:

from typing import Protocol

class Stocky(Protocol):
    stock_level: int

class Corn:
    stock_level = 1
    something_else = 99

class Wheat:
    stock_level = 2
    something_else_else = 99

def grind(stalk: Stocky):
    print(stalk.stock_level)


if __name__ == "__main__":
    corn_0 = Corn()
    corn_1 = Corn()
    corn_1.stock_level = 3
    grind(corn_0)
    grind(corn_1)
    grind(Wheat())

it outputs:

1
3
2

Notice that the classes Corn and Wheat are not subclasses of the Stocky protocol - they merely adhere to it. And the method grind() takes a value that adheres to that protocol.

If I run mypy on the script, there are no issues.

@NotPeopling2day
Copy link
Contributor Author

Closing this ticket in favor of a SPIKE on refactoring tracing into ecosystems and pulling the logic up to a higher abstraction.

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.

3 participants