Merge pull request #51 from ConsenSys/fix-selector-computation

Fix canonical signature hashes (`selector`s) computation

Author: blitz-1306

src/ast/implementation/declaration/variable_declaration.ts

@@ -1,5 +1,6 @@
+import { getUserDefinedTypeFQName } from "../../../types";
 import { ASTNode } from "../../ast_node";
-import { DataLocation, Mutability, StateVariableVisibility } from "../../constants";
+import { ContractKind, DataLocation, Mutability, StateVariableVisibility } from "../../constants";
 import { encodeSignature } from "../../utils";
 import { Expression } from "../expression/expression";
 import { OverrideSpecifier } from "../meta/override_specifier";
@@ -158,33 +159,55 @@ export class VariableDeclaration extends ASTNode {
         const type = this.vType;
 
         if (type instanceof UserDefinedTypeName) {
-            const declaration = type.vReferencedDeclaration;
+            const site = this.getClosestParentByType(ContractDefinition);
 
-            if (declaration instanceof StructDefinition) {
-                const signatures = declaration.vMembers.map(
-                    (member) => member.canonicalSignatureType
+            if (site === undefined) {
+                throw new Error(
+                    `Unable to compute canonical signature type for variables outside of contract: ${this.print()}`
                 );
-
-                return "(" + signatures.join(",") + ")";
             }
 
-            if (declaration instanceof ContractDefinition) {
-                return "address";
-            }
+            const declaration = type.vReferencedDeclaration;
 
-            if (declaration instanceof EnumDefinition) {
-                const length = declaration.children.length;
+            if (site.kind === ContractKind.Library) {
+                if (
+                    declaration instanceof ContractDefinition ||
+                    declaration instanceof StructDefinition ||
+                    declaration instanceof EnumDefinition
+                ) {
+                    return getUserDefinedTypeFQName(declaration);
+                }
+            } else {
+                if (declaration instanceof StructDefinition) {
+                    const types = declaration.vMembers.map(
+                        (member) => member.canonicalSignatureType
+                    );
 
-                for (let n = 8; n <= 32; n += 8) {
-                    if (length < 2 ** n) {
-                        return "uint" + n;
-                    }
+                    return "(" + types.join(",") + ")";
                 }
 
-                throw new Error("Unable to detect enum type size - member count exceeds 2 ** 32");
+                if (declaration instanceof ContractDefinition) {
+                    return "address";
+                }
+
+                if (declaration instanceof EnumDefinition) {
+                    const length = declaration.children.length;
+
+                    for (let n = 8; n <= 32; n += 8) {
+                        if (length < 2 ** n) {
+                            return "uint" + n;
+                        }
+                    }
+
+                    throw new Error(
+                        "Unable to detect enum type size - member count exceeds 2 ** 32"
+                    );
+                }
             }
 
-            throw new Error("Unknown user defined type");
+            throw new Error(
+                `Unhandled user-defined type when computing canonical signature type: ${declaration.print()}`
+            );
         }
 
         return this.typeString;

src/types/utils.ts

@@ -130,7 +130,7 @@ export function generalizeType(type: TypeNode): [TypeNode, DataLocation | undefi
     return [type, undefined];
 }
 
-function getUserDefinedTypeFQName(
+export function getUserDefinedTypeFQName(
     def: ContractDefinition | StructDefinition | EnumDefinition
 ): string {
     return def.vScope instanceof ContractDefinition ? `${def.vScope.name}.${def.name}` : def.name;

test/integration/factory/copy.spec.ts

@@ -51,6 +51,9 @@ describe(`ASTNodeFactory.copy() validation`, () => {
                     .join("\n")
                     .replace(new RegExp(process.cwd(), "g"), ".");
 
+                // Uncomment next line to update snapshots
+                // fse.writeFileSync(snapshot, result, { encoding: "utf-8" });
+
                 const content = fse.readFileSync(snapshot, { encoding: "utf-8" });
 
                 expect(result).toEqual(content);

test/integration/sol-ast-compile/tree.spec.ts

@@ -7,7 +7,12 @@ const cases = [
         "test/samples/solidity/declarations/interface_060.sol",
         "test/samples/solidity/declarations/interface_060.tree.txt"
     ],
-    ["test/samples/solidity/interface_id.sol", "test/samples/solidity/interface_id.tree.txt"]
+    ["test/samples/solidity/interface_id.sol", "test/samples/solidity/interface_id.tree.txt"],
+    [
+        "test/samples/solidity/library_fun_overloads.sol",
+        "test/samples/solidity/library_fun_overloads.tree.txt"
+    ],
+    ["test/samples/solidity/fun_selectors.sol", "test/samples/solidity/fun_selectors.tree.txt"]
 ];
 
 for (const [sample, snapshot] of cases) {

test/samples/solidity/fun_selectors.sol

@@ -0,0 +1,92 @@
+contract D {
+    uint a;
+}
+
+struct T {
+    address y;
+}
+
+enum X {
+    A
+}
+
+library Foo {
+    enum Y {
+        A
+    }
+
+    struct S {
+        uint x;
+    }
+
+    function funD(D d) public {}
+    function funS(S memory s) public {}
+    function funT(T memory t) public {}
+    function funX(X x) public {}
+    function funY(Y y) public {}
+}
+
+interface Bar {
+    function funD(D d) external;
+    function funS(Foo.S calldata s) external;
+    function funT(T calldata s) external;
+    function funX(X x) external;
+    function funY(Foo.Y y) external;
+}
+
+contract Baz {
+    function funD(D d) external {}
+    function funS(Foo.S calldata s) external {}
+    function funT(T calldata s) external {}
+    function funX(X x) external {}
+    function funY(Foo.Y y) external {}
+
+    function main() public {
+        assert(Foo.funD.selector == bytes4(keccak256("funD(D)")));
+        assert(Foo.funD.selector == 0x46467911);
+
+        assert(Foo.funS.selector == bytes4(keccak256("funS(Foo.S)")));
+        assert(Foo.funS.selector == 0x2d9de9c3);
+
+        assert(Foo.funT.selector == bytes4(keccak256("funT(T)")));
+        assert(Foo.funT.selector == 0x8c551140);
+
+        assert(Foo.funX.selector == bytes4(keccak256("funX(X)")));
+        assert(Foo.funX.selector == 0x20c5a75c);
+
+        assert(Foo.funY.selector == bytes4(keccak256("funY(Foo.Y)")));
+        assert(Foo.funY.selector == 0xc79a4d37);
+
+        assert(Bar.funD.selector == bytes4(keccak256("funD(address)")));
+        assert(Bar.funD.selector == 0x4e209091);
+
+        assert(Bar.funS.selector == bytes4(keccak256("funS((uint256))")));
+        assert(Bar.funS.selector == 0xe373f962);
+
+        assert(Bar.funT.selector == bytes4(keccak256("funT((address))")));
+        assert(Bar.funT.selector == 0x3793b6f0);
+
+        assert(Bar.funX.selector == bytes4(keccak256("funX(uint8)")));
+        assert(Bar.funX.selector == 0x0a42a215);
+
+        assert(Bar.funY.selector == bytes4(keccak256("funY(uint8)")));
+        assert(Bar.funY.selector == 0x0a035664);
+        
+        assert(type(Bar).interfaceId == 0x9a812b72);
+
+        assert(Baz.funD.selector == bytes4(keccak256("funD(address)")));
+        assert(Baz.funD.selector == 0x4e209091);
+
+        assert(Baz.funS.selector == bytes4(keccak256("funS((uint256))")));
+        assert(Baz.funS.selector == 0xe373f962);
+
+        assert(Baz.funT.selector == bytes4(keccak256("funT((address))")));
+        assert(Baz.funT.selector == 0x3793b6f0);
+
+        assert(Baz.funX.selector == bytes4(keccak256("funX(uint8)")));
+        assert(Baz.funX.selector == 0x0a42a215);
+
+        assert(Baz.funY.selector == bytes4(keccak256("funY(uint8)")));
+        assert(Baz.funY.selector == 0x0a035664);
+    }
+}