ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.04k stars 5.7k forks source link

Disallow absolute import paths (path spec v3) #11410

Closed cameel closed 1 year ago

cameel commented 3 years ago

Closes #9346. Depends on #11409. Part of a set of issues that replaces #11138.

Abstract

Disallow absolute paths in the virtual filesystem to make it harder to create non-portable metadata containg system-dependent paths.

Motivation

Absolute paths are in most cases different on every machine (especially when they include the path to user's home directory) and using them results in non-reproducible bytecode (because they end up in metadata and metadata hash is included in the bytecode). This leads to verification tools giving up and just stripping metadata completely. Even if such paths are not directly used in imports, frameworks often use them in Standard JSON input or remappings. While it's entirely possible to structure a project in such a way that it uses only relative paths, it's not always convenient or even clear how to do it when using external libraries.

11409 adds a mechanism that allows conveniently importing files from libraries stored in arbitrary locations using relative paths.

Once it's implemented we could deprecate and later completely disallow absolute paths in the VFS without taking away any useful functionality.

Specification

  1. In the VFS disallow any source unit names that start with /. This means that they would be also disallowed in import statements and on the urls list in source dict in Standard JSON.
  2. Disallow using empty path for --base-path.
    • An empty path is currently the default value. Make the path to the current working directory the default.
  3. On the command line only allow specifying paths to files that are located inside --base-path or --include-paths.
    • The paths on the command line can of course still be both relative and absolute because due to #11408 and #11409 they will always be normalized and made relative before they end up in the VFS.
    • Also adjust solcjs to behave in the same way.
  4. (Optional) In Standard JSON provide a new key called path inside sources, in addition to the currently available content and urls.
    • This would be a way to preserve the ability to load files from absolute paths, which some users find useful (#9346). These are not included in metadata so allowing absolute paths here would not be harmful.
    • The value is not passed to the file loader callback. Instead the file is loaded directly from the specified path.
    • Path can be absolute or relative to the current working directory. It's not affected by --base-path.
    • Path must be within --allowed-paths.
    • The feature is only available when using the native compiler which has access to the underlying filesystem.
    • It cannot be used together with with content or urls.

NOTE: --allowed-directories becomes almost redundant after these changes. There are now only two cases where it's needed:

Backwards Compatibility

The change is not backwards-compatible. It will require adjusting code or compiler options in several use cases:

All of these can easily be solved by using --base-path and --include-paths.

Path resolution in frameworks

This change will require changes in popular frameworks. Of the four I investigated, two (Truffle and Brownie) are using absolute paths in some cases and will require adjustments. The other two (Hardhat and dapp.tools) should be unaffected.

Truffle

Truffle has a resolver package which is responsible for finding a library on disk based on the path used in the import and the path of the importing source unit. The whole system is pretty modular and can find files in npm's node_modules/ (global and local), EthPM registry or even files auto-generated from ABI JSON using abi-to-sol. Located source is inserted into Standard JSON input as content and it does not rely on the file loader callback. In case of npm packages the source unit name used is a relative path.

It allows importing from arbitrary locations in the filesystem too, in which case the path ends up being absolute.

One thing that will no longer be possible in Truffle will be imports like import "../node_modules/@openzeppelin/contracts/utils.sol" in a file located in contracts/ that exists at the same level as node_modules/. I've seen some people using them as workarounds for other problems with imports (https://github.com/trufflesuite/truffle/issues/593#issuecomment-635143731). It works if the source unit name of the importing file in the virtual filesystem is an absolute path because then going one level above node_modules/ in the directory tree is allowed. It the source unit name was just the name of the file, the import would break.

The use of absolute paths has been already recognized as a problem in https://github.com/trufflesuite/truffle/issues/1621.

Hardhat

Hardhat, just like Truffle, uses Standard JSON and does not rely on a file loader callback but its path resolution is much simpler. Its localPathToSourceName() only has special cases for relative imports (starting with ../) and for node_modules/.

It does not allow using absolute paths in imports at all (reports error HH407).

Brownie

Brownie uses Standard JSON but only includes project files as content in it. For loading libraries it relies on the default file loader. It has its own package manager, which by default installs libraries in ~/.brownie/packages/. For example Open Zeppelin 3.0.0 is installed in ~/.brownie/packages/OpenZeppelin/openzeppelin-contracts@3.0.0. By default it adds remappings of the form OpenZeppelin=/home/user/.brownie/packages/OpenZeppelin so that files can be imported with import "OpenZeppelin/openzeppelin-contracts@3.0.0/contracts/math/SafeMath.sol". It also encourages users to use additional remappings to make imports of the form import "@openzeppelin/contracts/math/SafeMath.sol" work as well.

dapp.tools

dapp.tools, like Brownie, uses Standard JSON and relies on the default file loader and import remappings. Unlike Brownie, it requires a rigid project structure where all project and library files are stored (or symlinked) inside the same root directory (project files under src/, libraries under lib/) so absolute paths are not necessary and not used.

cameel commented 3 years ago

We talked about this on today's call but did not make a decision yet. It's at least clear that we do not want this to be a sudden change that catches anyone by surprise. If we want to get it through, we need to reach out to frameworks first and help them get rid of absolute paths from metadata. Only then it would be feasible to phase out paths starting with /.

Reporting warnings instead of errors when absolute paths are used is also not necessarily something we want to do.

Our primary motivation here is actually reducing confusion among users about what absolute paths used in imports mean.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] commented 1 year ago

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.