Consensys / surya

A set of utilities for exploring Solidity contracts
Apache License 2.0
1.06k stars 118 forks source link

Graph draw method in wrong contract when using super inside a function. #189

Closed vittominacori closed 9 months ago

vittominacori commented 10 months ago

I'm concerning about the below behavior.

If a contract inherit from multiple contracts, it seems that graph insert a method within the wrong contract when that method is called using super.methodName.

Having the code below :

pragma solidity ^0.8.20;

abstract contract A {
    function fooA() public pure returns (string memory) {
        return "A";
    }
}

abstract contract B {}

contract C is A, B {
    function fooC() public pure returns (string memory) {
        return super.fooA();
    }
}

and then using

npx surya graph MyContract.sol | dot -Tpng > MyContract.png

produces the below result

graph

But contract B has no method fooA.

Why is it saying that there is an external call to B.fooA? Am I missing something?

GNSPS commented 9 months ago

Whoof! This is definitely me needing to handle the inheritance pattern better. Hopefully, it's just me messing up the ordering of the inheritance statement. The worst-case scenario is a complete disregard for where the specific method name is inherited from.

I'll dig deeper. Thank you for reporting.

GNSPS commented 9 months ago

After analysis this is definitely the worst-case scenario my "super" keyword decoding in the inheritance tree is extremely dumb! 🙈 I'll fix it, might take a while.

GNSPS commented 9 months ago

It was a super easy fix after all! 😄 Please try v0.4.10! 🙏

Thank you again!

vittominacori commented 9 months ago

Thanks. This solved the above issue but I have just another doubt.

Considering the below code (I have more complex contracts inheriting but this should reproduce the issue):

pragma solidity ^0.8.20;

abstract contract Foo {
    string private _fooA;
    string private _fooB;

    constructor(string memory fooA_) {
        _setFooA(fooA_);
    }

    function _setFooA(string memory fooA_) internal {
        _fooA = fooA_;
    }

    function _setFooB(string memory fooB_) internal {
        _fooB = fooB_;
    }
}

contract MyContract is Foo {
    constructor() Foo("A") {
        super._setFooB("B");
    }
}

Should it say that:

This is the output produced and seems that both the calls are inside the Foo constructor.

Am I wrong?

MyContract

vittominacori commented 9 months ago

Hi @GNSPS, have you had a chance to look at this behavior?

GNSPS commented 9 months ago

Thank you for that again. I've taken a look. Released a new version with a fix. v0.4.11. Feel free to try it out.