sol-compiler: Imported contracts are not compiled

[PaulRBerg]

Expected Behavior

Imported contracts that are not extended should also be compiled and parsed as artifacts. This is the default behaviour in truffle. For instance, see @sohkai's file from the aragon codebase:

import "@aragon/os/contracts/acl/ACL.sol";
import "@aragon/os/contracts/kernel/Kernel.sol";
import "@aragon/os/contracts/factory/DAOFactory.sol";
import "@aragon/os/contracts/factory/EVMScriptRegistryFactory.sol";

import "@aragon/apps-vault/contracts/Vault.sol";

import "@aragon/apps-shared-migrations/contracts/Migrations.sol";
import "@aragon/test-helpers/contracts/EtherTokenConstantMock.sol";
import "@aragon/test-helpers/contracts/TokenMock.sol";
import "@aragon/test-helpers/contracts/TokenReturnFalseMock.sol";
import "@aragon/test-helpers/contracts/TokenReturnMissingMock.sol";

// You might think this file is a bit odd, but let me explain.
// We only use some contracts in our tests, which means Truffle
// will not compile it for us, because it is from an external
// dependency.
//
// We are now left with three options:
// - Copy/paste these contracts
// - Run the tests with `truffle compile --all` on
// - Or trick Truffle by claiming we use it in a Solidity test
//
// You know which one I went for.

contract TestImports {
    constructor() public {
        // solium-disable-previous-line no-empty-blocks
    }
}

Current Behavior

The contracts that have been imported, but not extended from, should be compiled and be parsed as artifacts (like Kernel from the example above).

Possible Solution

I suspect that this is similar to how normal contracs imports from node_modules are processed.

Steps to Reproduce (for bugs)

1. git clone git@github.com:0xProject/dev-tools-truffle-example.git
2. yarn add @openzeppelin/contracts
3. Create a file called `TestImports.sol` which imports a contract from OpenZeppelin

Context

I have a contract which I use in my tests but I don't use in my codebase, thus I end up having to make truffle pick it up. Everything works fine when I run my tests, but I get a bunch of warnings when running any tool from the 0x stack.

Your Environment

Package	Version
sol-compiler	3.1.13
truffle	5.0.33

Network
development

[dekz]

Are you able to explicitly list the contracts you want compiled in the compiler.json file? Does this then result in the coverage working correctly?

[PaulRBerg]

I tried to do that but it didn't work:

Error: Failed to resolve Sablier
    at SpyResolver.Resolver.resolve (/Users/paulrberg/.nvm/versions/node/v10.15.3/lib/node_modules/@0x/sol-compiler/node_modules/@0x/sol-resolver/src/resolvers/resolver.ts:8:19)

I get the same error for contracts from @openzeppelin/contracts.

[PaulRBerg]

I'm sorry to spam the thread, but this is a rather serious problem with sol-compiler. It causes some really weird behaviour that's hard to debug. I'm thinking to drop the 0x stack altogether, even if it has many neat features.

I know compilers and coverage tools aren't exactly your business, and I respect that, but it'd be good to know if this stack (sol-compiler, sol-coverage, sol-trace) are going to be maintained.

[abandeali1]

@PaulRBerg how did you specify the import in your compiler.json? I actually just tested this and had no issues (@openzeppelin/contracts was installed in my project's node_modules, not globally):

{
    "artifactsDir": "./generated-artifacts",
    "contractsDir": "./contracts",
    "useDockerisedSolc": false,
    "isOfflineMode": false,
    "compilerSettings": {
        "evmVersion": "constantinople",
        "outputSelection": {
            "*": {
                "*": [
                    "abi",
                    "devdoc",
                    "evm.bytecode.object",
                    "evm.bytecode.sourceMap",
                    "evm.deployedBytecode.object",
                    "evm.deployedBytecode.sourceMap"
                ]
            }
        }
    },
    "contracts": ["@openzeppelin/contracts/crowdsale/Crowdsale.sol"]
}

I think it would be a bit strange to compile unused imports by default since they are not part of the bytecode of any contracts. Another alternative would be to inherit the imports you want to use in a test contract, and then compile the test contract:

contract TestCrowdale is Crowdsale {}

These tools are definitely going to be maintained (and even improved!). We have admittedly had low bandwidth for changes to them recently though.

[PaulRBerg]

how did you specify the import in your compiler.json?

It was different to what you did here. Now it works:

{
  "contracts": [
    "@sablier/protocol/contracts/Types.sol",
    "@sablier/protocol/contracts/Sablier.sol",
    "@sablier/protocol/contracts/interfaces/IERC1620.sol"
  ],
}

But of course only these 3 contracts get compiled. Is there a way to include all contracts in the contractsDir but also extend the list with a few cherry picked contracts?

I think it would be a bit strange to compile unused imports by default since they are not part of the bytecode of any contracts.

I see where you're coming from, but I don't think it would cause any harm to do this, no? Compiling imports by default would be helpful for tests, would alleviate the contractsDir issue from above and would finally make the behaviour similar to truffle's. The only cost I'm aware of is a slightly longer compile time.

These tools are definitely going to be maintained (and even improved!).

Amazing, thank you 🥳

[abandeali1]

But of course only these 3 contracts get compiled. Is there a way to include all contracts in the contractsDir but also extend the list with a few cherry picked contracts?

I don't think so at the moment. Would that be enough for your use case? I think it should be fairly easy to allow something like:

{
  "contracts": [
    "@sablier/protocol/contracts/Types.sol",
    "@sablier/protocol/contracts/Sablier.sol",
    "@sablier/protocol/contracts/interfaces/IERC1620.sol",
    "*"
  ],
}

Compiling all imports by default would actually be a pretty difficult change with the way the code is currently structured.

[PaulRBerg]

Compiling all imports by default would actually be a pretty difficult change with the way the code is currently structured.

Ah, I see.

Would that be enough for your use case?

For sure, now I can make it work, but I am left with a tiny bit of technical debt. That is, every time a new contract is added to the codebase, it has to be added into the array in compiler.json. I asked the question in the first place because I'd tried adding "*" to the array, but that doesn't work as expected:

Error: Failed to resolve *
    at SpyResolver.Resolver.resolve (/Users/paulrberg/.nvm/versions/node/v10.15.3/lib/node_modules/@0x/sol-compiler/node_modules/@0x/sol-resolver/src/resolvers/resolver.ts:8:19)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because no activity occured in 7 days after being marked as stale. If it's still relevant - feel free to reopen. Thank you for your contributions.