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

VIP: Add support for relative imports. #1367

Closed
jacqueswww opened this issue Mar 25, 2019 · 16 comments · Fixed by #1578
Closed

VIP: Add support for relative imports. #1367

jacqueswww opened this issue Mar 25, 2019 · 16 comments · Fixed by #1578
Labels
Easy Pickings Used to denote issues that should be easy to implement VIP: Approved VIP Approved

Comments

@jacqueswww
Copy link
Contributor

jacqueswww commented Mar 25, 2019

Simple Summary

Allow using . and .. to specify relative imports of interfaces.

Abstract

Following a long discussion on #1361. It came to light that frameworks and programmers need more felxibility in terms of importing interfaces.

Motivation

The motivation is critical for VIPs that add or change Vyper's functionality. It should clearly explain why the existing Vyper functionality is inadequate to address the problem that the VIP solves as well as how the VIP is in line with Vyper's goals and design philosophy.

Specification

.. will specify parent directory of the current .vy being parsed.
. will specify current directory of the .vy being parsed.

e.g.

from .token_interface import TokenInterface

Will create an interface names TokenInterface from a file currently located at the same directory as the .vy file that is currently being parsed.

Environment variable:
Vyper shold be allowed to change it's lookup directory from CWD to one set in VYPER_PATHS.

Backwards Compatibility

Is forward compatible.

Dependencies

None.

Copyright

Copyright and related rights waived via CC0

@jacqueswww jacqueswww changed the title Add support for Add support for relative imports. Mar 25, 2019
@jacqueswww jacqueswww added VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting VIP: Approved VIP Approved labels Mar 25, 2019
@fubuloubu
Copy link
Member

@mikeshultz We decided to take a different tactic to do what you were doing in #1361. We hope you might comment on it here :)

@mikeshultz
Copy link

For what it's worth, the toolkit I'm working on now invokes the vyper module directly and does import resolution itself instead of relying on invoking the CLI script.

That said, I still think this is a good idea. And if this resolution is not implemented in the compiler(but the CLI script with the package), this is the kind of thing that should be documented for other tools utilizing Vyper.

Generally, maybe this is the kind of thing that would be good to get some documented rules/best practices for how to resolve absolute paths as well. If you all think the current working directory is the best place to resolve from, that should be documented. Maybe absolute paths and the possibility of having a library PATH is a different discussion, however.

@fubuloubu
Copy link
Member

Maybe the compiler API can be written such that if the compiler encounters any import statements, it references a "lookup" mapping of those packages to the specific source files they reference, which must be provided by the CLI script (and any other tools). This keeps the compiler functionally pure, and mitigates introducing the concept of the filesystem into the compiler.

@mikeshultz
Copy link

Isn't that what it does now? Well, except the file contents are provided instead of a path. The downside of that would be that whatever invokes the compiler will still need to know the location of the contract/package names to provide the compiler. In that case, may as well leave it the same.

Maybe a list in which each provided path is checked in order? That would be more inline with traditional PATH-like imports.

An example might be:

paths = [
    '~/my-project/contracts',
    '~/.local/lib/vyper/site-packages',
    '/usr/lib/vyper/site-packages',
]
vyper.compile_code(
    source_text,
    ['bytecode', 'abi'],
    import_paths=paths,
)

I guess for both relative and absolute imports it should be figured out if you want to keep the compiler filesystem agnostic, and leave that up CLI scripts. If so, some standards would still be good to keep everything inline so the user isn't guessing how it might work.

@fubuloubu
Copy link
Member

It might need some sort of "project root" object for it to check against.

File directory:

root/
    contractA.vy
    more/
        contractB.vy

contractA.vy:

from more.contractB import InterfaceB
# ... stuff

contractB.vy:

from ..contractA import InterfaceA
# ... stuff

compiler:

paths = {
    'contractA': contractA_source,
    'more': {
        'contractB': contractB_source,
    }
}
vyper.compile_code(
    source_text,
    ['bytecode', 'abi'],
    import_paths=paths,
)

The thought is that the compiler would have paths to look at when determining what to map to when it encounters an import statement.

Probably a lot wrong with this approach though.

@pipermerriam
Copy link
Contributor

I'd like to toss something out that may or may not be relevant which is about how we deal with namespacing of imports. This design will have long lasting influence on the language.

Python has dependency issues. It is really difficult to juggle dependencies since version conflicts are so easy to run into and the runtime uses a single namespace for dependency imports. Another way to say this is that you can only ever have one version of something installed and import web3 will import the same web3 module across all python files in a given runtime environment.

My understanding is that Node took a different approach, giving every package it's own namespace, thus allowing each package to install their own versions of each dependency and not sharing these dependencies across different packages. This fundamentally solves an entire class of dependency issues at the cost of a heavier runtime environment.

My inclination is to say that the Node approach is better in most/many of the ways that I think matter to Vyper. I'm curious to hear people's thoughts on this.

@mikeshultz
Copy link

This fundamentally solves an entire class of dependency issues at the cost of a heavier runtime environment.

For what it's worth, that cost can be pretty high. The worst part about dealing with node projects is dealing with dependencies. That's why there's a handful of competing package managers, and a ton of tools built up around just dealing with dependencies, and why the first solution to things "not working" is to rm -rf node_modules && npm i. Worked on a JS project recently that took about 30 minutes to install all the dependencies for their monorepo and it was a constant source of jokes and frustration.

That's not to say that Python is so much better when you have a project with mutual dependencies with your dependencies and projects that change their API like they change their underwear...

Figure I'd also mention that I've seen some projects that have taken to including version names in their contracts. For example, V01_MyContract. That allows them to not only keep a reference around for deployed contracts, but allow multiple versions to run side by side, and make it absolutely clear to the devs that versions of interfaces are different.

Not sure if that's the greatest solution, but thought I'd mention it since I've seen it in the wild.

@fubuloubu
Copy link
Member

fubuloubu commented Mar 27, 2019

My inclination is to say that the Node approach is better in most/many of the ways that I think matter to Vyper.

Not to say the Node approach is at all better than the Python approach, but the fact of the matter is that this approach makes a lot of sense for smart contracts. It's also how ethPM works. Dependencies for smart contract should be orders of magnitude less than Javascript or Python (at least they are now), so I think this approach is reasonable and scaleable. We at least won't be constructing a "ethpm packages" directory denser than a neutron star.


That being said, I still think there is a requirement for a "local" context. Perhaps . can serve as that. For a flat contracts/ directory, that would be import .Contract for a file at Contract.{vy|json}. For things with multiple levels, this sort of limits it to only one level of depth in the relative import statement (e.g. import ..Contract) which I also believe is reasonable and scaleable.

The most complex smart contract systems I have seen have been on the order of 50 files, which fits fine in a 1-level deep folder structure. Vyper contract systems will be less file-heavy, due to the structure that Vyper contracts must fit within a single file (except for interfaces). I would argue anything that's larger than this level of complexity deserves to be composed into separate projects and handled through multiple deployed contract systems.

@jacqueswww jacqueswww added the Easy Pickings Used to denote issues that should be easy to implement label Jun 6, 2019
@iamdefinitelyahuman
Copy link
Contributor

I've been working on this issue as a lead-in to #1520, as the json input will require source paths. Came across an interesting edge case bug:

Before compiling, a mapping is generated that associates each interface import name with it's file path:

https://github.com/ethereum/vyper/blob/e9f3d566782c88f6fa8735136723c3506d2a6cf2/bin/vyper#L115-L120

In interface.extract_file_interface_imports there is a check to prevent duplicate imports using the same name within the same contract:

https://github.com/ethereum/vyper/blob/e9f3d566782c88f6fa8735136723c3506d2a6cf2/vyper/signatures/interface.py#L217-L221

However - lacking a collision check in get_interface_codes, if multiple contracts import different interfaces but assign them to the same name, the last parsed contract's interface is the one the compiler will use for all contracts. If this happens, the compile ultimately fails by raising a FunctionDeclarationException.

What would be a better solution to this?

  1. Check for collisions within get_interface_codes and enforce that a user cannot compile two contracts that assign different interfaces to the same name
  2. Call get_interface_codes separately for each contract

To me, 1 feels more aligned with Vyper's ethos, but it would be trivial for a user to just compile the contracts separately and so adding a check isn't really enforcing much - just ensuring the exception raised is less confusing to the user.

@fubuloubu
Copy link
Member

I think 1 is the way to go. Yes, you can just compile them separately if you're being sneaky, but we don't design for sneaky users, just regular ones.

@fubuloubu fubuloubu changed the title Add support for relative imports. VIP: Add support for relative imports. Aug 8, 2019
@fubuloubu fubuloubu removed the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Aug 8, 2019
@davesque
Copy link
Contributor

davesque commented Aug 8, 2019

@fubuloubu @iamdefinitelyahuman I actually think 2 is the way to go here. Imagine the case that we start adding package support to vyper. Then, we may have a situation where you'd like to import some interface that you didn't write. It would be pretty annoying if you were planning to use a name that's already in use in the library. IMHO it also seems likely that you'd run into naming conflicts pretty quickly within a single project. I can imagine users coming up with their own "namespacing" techniques to avoid compilation errors by prefixing the import names of things with the name of the contract into which an interface is being imported. Then you have an ad hoc system that is prone to error (via typos and such).

@davesque
Copy link
Contributor

davesque commented Aug 8, 2019

I suppose, for now, we can just go with the global namespace, but I'd like to revisit this. I'm pretty sure this will become a stumbling block for users.

@fubuloubu
Copy link
Member

Great points, yeah I agree with that. (2) might be more difficult for (1), but avoids issues later.

@iamdefinitelyahuman
Copy link
Contributor

Re: from imports, the current syntax for built-in interfaces is:

from <folder> import <filename> (as filename)

the alias is assumed, if a contract specifies one it is ignored by the compiler. For example, this contract fails to compile with Contract "Foo" not declared yet:

from vyper.interfaces import ERC20 as Foo

@public
def bar() -> uint256:
    return ERC20(0).balanceOf(msg.sender)

@public
def baz() -> uint256:
    return Foo(0).balanceOf(msg.sender)

The proposed change for this VIP follows a pattern of:

from <.folder.filename> import <alias>

This creates ambiguity in the from import syntax. Because this syntax was already established for built-in interfaces, I think relative imports should instead use from <.folder> import <filename> to maintain consistency. Attempting to alias a from import should either fail loudly, or be supported. Supporting it keeps Vyper more in-line with how Python works, disallowing it arguably makes for more readable code... and possibly I'm not considering something here.

@davesque @fubuloubu thoughts?

@fubuloubu
Copy link
Member

fubuloubu commented Aug 15, 2019

Proposed amended rule set:

# file heirarchy:
# contracts/
#     contractA.vy
#     interfaceB.json
#     folder/
#         contractC.vy
#         interfaceD.json

# contractA.vy:
from .. import fail  # Fails if `contracts/` is used as root folder (outside access)
import ..fail  # Fails same as above

import interfaceB  # `interfaceB` interface made available
import .interfaceB  # `interfaceB` interface made available (same as above)

import folder.contractC  # `contractC` interface made available
import folder.interfaceD as MyInterface  # `MyInterface` interface made available (alias)

# json files can't reference other interfaces (by design)

# contractC.vy:
from .. import contractA  # `contractA` interface made available
import ..interfaceB  # `interfaceB` interface made available

import .interfaceD as MyInterface  # `MyInterface` interface made available (alias)
from . import contractC as Self  # `Self` interface made available
# Should this be allowed? Why not? There's no technical reason not to

Edit: This makes the assumption of "derived interfaces", which I'm not sure is a reality in the current codebase, but could/should definitely be able to be implemented.


Edit: to be clear, Self is not any sort of "special sauce", it would just be a convention for calling with your own interface as the external interface

@fubuloubu
Copy link
Member

A good example of self-referencing would be something like Uniswap exchanges, when a Token to Token swap is desired. Basically, you have one function that wishes to make a call to another function using the same interface (but different deployed addresses).

I mean, we try to avoid mutual recursion as a rule, but we can't stop people from doing it if they're clever, so might as well make it easy to be clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Pickings Used to denote issues that should be easy to implement VIP: Approved VIP Approved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants