trufflesuite / truffle

:warning: The Truffle Suite is being sunset. For information on ongoing support, migration options and FAQs, visit the Consensys blog. Thank you for all the support over the years.
https://consensys.io/blog/consensys-announces-the-sunset-of-truffle-and-ganache-and-new-hardhat?utm_source=github&utm_medium=referral&utm_campaign=2023_Sep_truffle-sunset-2023_announcement_
MIT License
14.02k stars 2.32k forks source link

Let compiler handle cyclic dependencies #135

Closed jeremyellis closed 7 years ago

jeremyellis commented 8 years ago

If I have two contracts like so

import "./pong.sol"
contract ping
{
    pong p;
}

and

import "./ping.sol"
contract pong
{
    ping p;
}

I get a cyclic dependency error when compiling. I was under the impression this shouldn't happen as they only have each other as members in the contracts, not inherit from each other.

markledev commented 8 years ago

I have the same error. @tcoulter Could you explain why we should avoid this cyclic dependency AT ALL COST while designing smart contracts?

redsquirrel commented 8 years ago

Cyclic dependencies are impossible in [certain situations](If a contract wants to create another contract, the source code %28and the binary%29 of the created contract has to be known to the creator.):

If a contract wants to create another contract, the source code (and the binary) of the created contract has to be known to the creator.

But it does seem like they should be allowed when the contracts are members of each other.

tcoulter commented 8 years ago

@redsquirrel For some reason your link is 404'ing for me. That said, @jeremyellis and @phuongvietle, you solve cyclic dependencies using abstract contracts: http://solidity.readthedocs.io/en/latest/contracts.html?highlight=abstract#abstract-contracts

Imagine a scenario where contract A will create a new contract B (A depends on B). And B will use the contract A. Here, there's a cyclic dependency. You can resolve that by creating an abstract version of A and using that within B. Example:

A.sol

import "B.sol";
contract A {
  B b;
  function someFunction() {
    b = new B();
  }
}

AbstractA.sol

contract AbstractA {
  function someFunction(); // No implementation!
}

B.sol

import "AbstractA.sol";

contract B {
  AbstractA a;

  function setA(AbstractA new_a) {
    a = new_a;
  }

  function useA() {
    a.someFunction();
  }
}

Obviously this isn't a great example because B doesn't do anything useful, but you get around the cyclic dependencies using the abstract version of A. Note: You can't create new instances of an abstract contract (i.e., new AbstractA() will throw an error). If you need both contracts to create a new version of A, you should create a third contract called "AFactory" whose sole job it is to create A's.

Closing as this isn't a truffle issue.

jeremyellis commented 8 years ago

@tcoulter But solc happily compiles the ping and pong contracts, is there a reason truffle doesn't allow it?

redsquirrel commented 8 years ago

Sorry for the bad link. Here's where I was trying to point: http://solidity.readthedocs.io/en/latest/contracts.html#creating-contracts

@tcoulter I do think this is actually a Truffle issue. contracts.js looks for cycles in contract dependencies and errors if they are detected.

Cyclic dependencies don't appear to be a problem in Solidity as long as A and B don't need to create each other. If C did the creating and connected A and B to each other, then everything should technically work (but Truffle would still error). Only cyclic creation dependencies are impossible.

tcoulter commented 8 years ago

Ah, perhaps I misunderstood. Reopening.

tcoulter commented 8 years ago

Truffle creates its own dependency graph in order to determine which files need to be compiled when a specific solidity file has been edited. Perhaps it's overzealous about cyclic dependencies, and should allow the compiler to handle that.

markledev commented 8 years ago

@tcoulter I am looking forward to new version of truffle! When will the new version be released?

tcoulter commented 8 years ago

@phuongvietle I'm working on some documentation right now on a PR that needs to be merge. That PR as well as other small fixes will make up the 3.0 release. Follow this: https://github.com/ConsenSys/truffle/pull/143 . I'll remove the WIP header and have documentation there today, likely within the next hour.

markledev commented 8 years ago

@tcoulter Got it! Thank you for saving us huge amount of time figuring out a sound workaround for this issue.

tcoulter commented 8 years ago

:+1: :)

PS: docs might be a couple hours, rather than an hour. :)

D-Nice commented 7 years ago

Ran into same issue, and hopefully looking to have solc handle this.

cryptophonic commented 7 years ago

+1 same issue. the check doesn't seem to be working correctly either. For example, I'm getting this cyclic dependency error from a contract that has a sole import of an abstract contract that (a) no other contract imports and (b) doesn't import anything else. Looking into how to disable this check..

tcoulter commented 7 years ago

This has been fixed in truffle-compile, v1.1.1. You should be able to get this update if you uninstall and reinstall Truffle, if on Truffle 3.x. Thanks for the reports!

cryptophonic commented 7 years ago

Using this new version of Truffle, I have a simple contract that imports another contract (not a library). When the imported contract is changed, it's not rebuilding the contract that imports it. Using 'truffle compile --compile-all' resolves the problem by forcing a complete rebuild. Hopefully this isn't an either / or situation where we need the cyclic dependency check in order to have dependencies built automatically.

tcoulter commented 7 years ago

@mjackson001 The issue you reported sounds very different to the one reported here. I'd recommend you create a new ticket instead.

When doing so, it's very helpful to provide code and show us your directory structure, and tell us where you're importing from, say from npm, or ethpm, etc. If from npm or ethpm, that's a known issue.

cryptophonic commented 7 years ago

ok, let me try to create a simple test case and submit a separate issue...

cryptophonic commented 7 years ago

Test case submitted: https://github.com/trufflesuite/truffle/issues/384