dapptools icon indicating copy to clipboard operation
dapptools copied to clipboard

dapp: package local remappings

Open d-xo opened this issue 4 years ago • 10 comments

As discussed in https://github.com/dapphub/dapptools/issues/715#issuecomment-890358156 , this modifies dapp remappings so that it now generates a unique set of remappings for each src folder in the dependency tree that points into the adjacent lib folder.

This allows for multiple versions of a given package in the dependency tree, and means ds-pain is happily obsolete. You can check out https://github.com/dapphub/remappings-test for an example of a project that would be impossible to compile successfully with the old remappings shceme, but works fine with the new project local remappings.

The new remappings use the context field in the remappings format, which is supported back to at least solc 0.4.0 (I didn't check any further back...).

I'm still slightly on the fence if it's not better to just completely remove dapp remappings and force people to use relative paths for imports, but that would be a pretty major break to backwards compatibility, and I think probably ugly enough for larger projects to justify the continued existence of remappings for now.

There are a few breakages:

  1. Projects will need to declare all their dependencies up front (e.g. if I dapp install ds-token, I only get ds-token available for import via remappings in the top level project, and must run a seperate dapp install ds-test to make ds-test available, even though ds-test is installed as a dep of ds-token). This means that some projects that would compile with an older version of dapp will fail with with newer versions. Maybe it's worth introducing some legacy compatibility options.

  2. Since remappings are included in the metadata hash, this will break bit for bit bytecode verification when comparing the same project built with two different dapp versions. I don't see this as a big issue.

Are there considerations I'm missing? Was there a reason for enforcing the same version of a package across the dependcy tree?

Still needs changelogs and docs before merge...

d-xo avatar Aug 02 '21 11:08 d-xo

Exciting! I think this is a good approach to ameliorate the biggest pain point with a small change. Will investigate the consequences

MrChico avatar Aug 02 '21 15:08 MrChico

oo love this

transmissions11 avatar Aug 02 '21 16:08 transmissions11

As discussed in https://github.com/dapphub/dapptools/issues/715#issuecomment-891319804, solc can throw errors for contracts that make use of multiple inheritance if multiple versions of the same contract exist in the inheritance tree.

I therefore added a processing step to the remappings generation that deduplicates identical package versions, and remaps them all to the same path. I also added another test case to https://github.com/dapphub/remappings-test that demonstrates the issue (and compiles successfuly with the deduplication step). Note that the deduplication step will only help for inheritance trees where the multiple contract instances are all genuinly from exactly the same package version (i.e. identical git hash).

d-xo avatar Aug 04 '21 09:08 d-xo

Added:

  • A legacy compatibility mode that allows direct imports from transitive dependencies if there is only a single version of that dependency in the tree
  • A fallback sha256sum based routine for getting a hash of a dependencies contents if the dep is not a git repo

With the above two changes we can build a complex project like tinlake-tests without needing to use custom remappings :tada:

For now the legacy mode is behind a flag instead of being the default since I think it's behaviour will be a little unpredictable (updating the version of a single dep deep in the dependency tree could break imports much higher up). Projects that need it can set it in a .dapprc or shell.nix file if required.

d-xo avatar Aug 04 '21 16:08 d-xo

Added docs & changelog entry.

d-xo avatar Aug 05 '21 11:08 d-xo

The biggest downside that I can see so far is that for complex projects the generated remappings can get quite huge. For example, if we run the new dapp remappings against tinlake-tests, we get this, whereas with the current approach, we get this (along with a ton of warnings related to mismatched package hashes....), which will make manually editing the generated remappings a lot harder (although hopefully this will not be needed nearly so much going forwards...).

d-xo avatar Aug 05 '21 12:08 d-xo

Tested a bit with a few public repos, and it seems as though compatibility is generally good, with the exception of repos that depend on dss-deploy. This actually makes sense, since if I depend on dss-deploy, I probably also want to reuse the version of dss that it depends on. Starting to wonder if it doesn't make more sense to allow transitive imports by default :thinking:

repo default --allow-transitive-imports
dss
tinlake-tests
drai
fuse-simulations
dss-cdp-manager
tinlake

d-xo avatar Aug 05 '21 14:08 d-xo

I was told issue #734 might be relevant to this PR.

livnev avatar Aug 11 '21 16:08 livnev

lfg

MrChico avatar Sep 08 '21 00:09 MrChico

After discussing with @rainbreak over the last few days I think I'm gonna let this sit a little longer and experiment with some alternative approaches. In particular it may be better to make a full break and jump straight to a workflow involving a fully flattened directory hierarchy under lib, which would significantly reduce network and disk bloat, while also making workflows that involve local edits to dependencies much more predictable.

d-xo avatar Sep 09 '21 07:09 d-xo