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

dapp: package local remappings #719

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

d-xo
Copy link
Contributor

@d-xo d-xo commented Aug 2, 2021

As discussed in #715 (comment) , 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...

@MrChico
Copy link
Member

MrChico commented Aug 2, 2021

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

@transmissions11
Copy link
Contributor

oo love this

@d-xo
Copy link
Contributor Author

d-xo commented Aug 4, 2021

As discussed in #715 (comment), 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
Copy link
Contributor Author

d-xo commented Aug 4, 2021

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 🎉

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

d-xo commented Aug 5, 2021

Added docs & changelog entry.

@d-xo
Copy link
Contributor Author

d-xo commented Aug 5, 2021

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

d-xo commented Aug 5, 2021

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 🤔

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

@livnev
Copy link
Member

livnev commented Aug 11, 2021

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

--extract) export DAPP_BUILD_EXTRACT=1;;
--optimize) export DAPP_BUILD_OPTIMIZE=1;;
--legacy) export DAPP_BUILD_LEGACY=1;;
--allow-transitive-imports) export DAPP_ALLOW_TRANSITIVE_IMPORTS=1;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a shorter alias? --trans?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a shorter alias? --trans?

ayy my own flag

xD

@MrChico
Copy link
Member

MrChico commented Sep 8, 2021

lfg

@d-xo
Copy link
Contributor Author

d-xo commented Sep 9, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants