dapptools
dapptools copied to clipboard
dapp: package local remappings
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:
-
Projects will need to declare all their dependencies up front (e.g. if I
dapp install ds-token, I only getds-tokenavailable for import via remappings in the top level project, and must run a seperatedapp install ds-testto makeds-testavailable, even thoughds-testis installed as a dep ofds-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. -
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...
Exciting! I think this is a good approach to ameliorate the biggest pain point with a small change. Will investigate the consequences
oo love this
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).
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
sha256sumbased 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.
Added docs & changelog entry.
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...).
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 | ✅ | ✅ |
I was told issue #734 might be relevant to this PR.
lfg
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.