vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.86k stars 794 forks source link

Error with Mutual importing when interfaces are used as a variable #2107

Open fubuloubu opened 4 years ago

fubuloubu commented 4 years ago

What's your issue about?

Was working off of 41508c3, where I changed the Exchange contract to import the Registry interface (everything works fine there). However, when I try to have the the Registry contract import the Exchange interface, I get a really weird error about ERC20 missing:

$ vyper examples/tokenswap/Registry.vy 
Error compiling: examples/tokenswap/Registry.vy
vyper.exceptions.UnknownType: No builtin or user-defined type named 'ERC20'
contract "Exchange", line 6:14 
     5
---> 6 token: public(ERC20)
---------------------^
     7 owner: public(Registry)

For reference, here is the diff:

$ git diff
diff --git a/examples/tokenswap/Registry.vy b/examples/tokenswap/Registry.vy
index ca4a818d..a07727dc 100644
--- a/examples/tokenswap/Registry.vy
+++ b/examples/tokenswap/Registry.vy
@@ -1,7 +1,4 @@
-interface Exchange:
-    def token() -> address: view
-    def receive(_from: address, _amt: uint256): nonpayable
-    def transfer(_to: address, _amt: uint256): nonpayable
+from . import Exchange

How can it be fixed?

There may be some logic that is unable to resolve the mutual import because it's trying to resolve itself as a type for ABI generation to obtain an interface, where instead it should just implicitly convert it to address (as an interface is an extension to the address ABI type). I'm not sure of this theory though

fubuloubu commented 4 years ago

It also seems to incorrectly raise an error with a contract compiling if it cannot compile an interface it imports enough to determine it's ABI signature:

$ vyper examples/tokenswap/*.vy
Error compiling: examples/tokenswap/Exchange.vy
vyper.exceptions.UnknownType: Compilation failed with the following errors:

UnknownType: No builtin or user-defined type named 'Exchange'
contract "Registry", line 10:35 
      9 # Maps token addresses to exchange addresses
---> 10 exchanges: public(HashMap[address, Exchange])
-------------------------------------------^
     11

UnknownType: No builtin or user-defined type named 'Registry'
contract "examples/tokenswap/Exchange.vy", line 7:14 
     6 token: public(ERC20)
---> 7 owner: public(Registry)
---------------------^
     8

For reference, here I am making the change to using Exchange inside a HashMap as the value type (which cannot be done yet per #1824)

fubuloubu commented 4 years ago

Also apparently an issue with the CI since this worked locally: https://github.com/vyperlang/vyper/runs/867618539

fubuloubu commented 4 years ago

It seems if the other file being imported also imports some interfaces and uses them in it's interface (say, as a public getter method), that the file doing the importing doesn't realize this and doesn't load the missing interface

As a resolution, it should probably raise instead, saying you need to import that interface/type too.

Alternatively, if interfaces in function signatures were downcasted to address when imported, that would also work

charles-cooper commented 3 years ago

@fubuloubu I'm having trouble reproducing this, could you include a minimal example in this issue?

fubuloubu commented 3 years ago

I believe the issue is if you try to import a file as an interface (e.g. from . import Exchange) which itself imports another interface (e.g. from vyper.interfaces import ERC20)

fubuloubu commented 3 years ago

See this PR, minus the code changes: https://github.com/vyperlang/vyper/pull/2048/

charles-cooper commented 3 years ago

Yeah anything with mutually recursive imports is going to break. That's why I commented out parsing Import statements here https://github.com/vyperlang/vyper/blob/5d0d8e6b5a55139fb8e94d6e759c97074ca526dd/vyper/ast/signatures/interface.py#L110-L113

fubuloubu commented 3 years ago

Yeah anything with mutually recursive imports is going to break. That's why I commented out parsing Import statements here

https://github.com/vyperlang/vyper/blob/5d0d8e6b5a55139fb8e94d6e759c97074ca526dd/vyper/ast/signatures/interface.py#L110-L113

It should be possible however, since imports are "limited depth", meaning it only should export their interface, not all of their code. If it sees another file importing itself, it should be possible to replace it with "whatever my own interface is". Think more like .h files mutually importing