Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
fubuloubu opened this issue Jul 14, 2020 · 8 comments
Open

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

fubuloubu opened this issue Jul 14, 2020 · 8 comments
Assignees
Labels
bug Bug that shouldn't change language semantics when fixed.

Comments

@fubuloubu
Copy link
Member

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 fubuloubu added the bug Bug that shouldn't change language semantics when fixed. label Jul 14, 2020
@fubuloubu
Copy link
Member Author

fubuloubu commented Jul 14, 2020

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
Copy link
Member Author

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

@fubuloubu
Copy link
Member Author

fubuloubu commented Sep 19, 2020

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
Copy link
Member

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

@fubuloubu
Copy link
Member Author

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
Copy link
Member Author

See this PR, minus the code changes: #2048

@charles-cooper
Copy link
Member

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

# parsing import statements at this stage
# causes issues with recursive imports
# vy_ast.Import,
# vy_ast.ImportFrom,

@fubuloubu
Copy link
Member Author

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

# parsing import statements at this stage
# causes issues with recursive imports
# vy_ast.Import,
# vy_ast.ImportFrom,

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug that shouldn't change language semantics when fixed.
Projects
None yet
Development

No branches or pull requests

3 participants