Closed mattsse closed 1 year ago
TIL context:
Is anyone actively working on this issue? Working on a project that needs multiple solidity versions on different dependencies and blocked on this feature
@fredlacs can you share a repro case which illustrates teh bug?
Here is a minimal repro of what I'm running into https://github.com/fredlacs/multiple-solc-repro
If you switch the commented out imports in src/*Dependencies.sol
you can't make it compile unless you change the packages imported in the submodules
I'm trying to manage 2 dependencies, one using solc 0.6 and another one using 0.8
I've seen other projects just manage multiple OZ versions locally (as in https://github.com/Phission-Finance/Phission-Finance/tree/master/lib).
Now that I think of it a bit more maybe the solution here isn't the context:
feature but rather a "deep remapping" to specify what to link @openzeppelin/contracts
to in each dependency
affected by this as well. importing thirdweb and oz contracts.
Same when building around existing contracts. Having transitive dependency resolution would be great.
Can you guys show some minimal repros for us to better understand the problem?
See #2693 for an example
See #2693 for an example
@mattsse can u ptal
Context-specific imports would be very helpful to avoid version incompatibilities in nested dependencies (and thus increase submodule composability)
Hey @mattsse, any plans to implement this?
We've got an issue @MorphoLabs which we can illustrate with the following, smallest git setup:
Let repository dependency
depend on openzeppelin-contracts
& have remappings.txt
:
@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts
Let repository module
depend on dependency
Then module
MUST have openzeppelin-contracts
as dependency, but no commit restriction is applied, which allows module
to install any version of openzeppelin-contracts
, including ones that may not be compatible with dependency
!
One way of solving this could be to have forge
compile with nested submodules instead of module
's root submodules (thus, the commit restriction would be applied by each git repository)
@Rubilmax
I'll take a closer look now.
I think adding this should be doable with medium effort, since solc should do the heavy lifting anyways.
perhaps we start by making this an opt-in setting in forge.
It would be awesome thx!
I ran into this issue while trying to implement opensea's blocklist. Installing operator-filter-registry as a submodule breaks OpenZepplin imports by expecting lib/openzeppelin-contracts/contracts
, when our project uses lib/openzeppelin-contracts/
Affected by this as well. We are creating a repository similar to our learn-evm-attacks project.
The main thing is that we are not writing many contracts but we do write a lot of tests. When we add a new repository-to-test, the ideal workflow would be to simply
$ forge install [project-to-test]
Unfortunately due to this problem, this is near impossible: different projects will use different versions and remappings. Context aware remappings are a must here for us.
Do you happen to have any updates on this, @mattsse? Sorry to tag you, but I've just bumped into a similar scenario to @joaquinlpereyra.
It seems to me that the more successful Foundry becomes, the more important this feature becomes because Foundry projects become more intertwined (both depth-wise and breadth-wise).
Our project sablier-labs/v2-periphery has many remappings:
Because of that and the lack of context-aware remappings in Foundry, V2 Periphery itself cannot be consumed by any third-party Foundry repo (without a patch). I installed v2-periphery
in my foundry-template, trying to import something from src/types/Permit2.sol
, and got this error:
Failed to resolve file: "/Users/prb/Workspace/templates/foundry-template/lib/v2-periphery/lib/permit2/interfaces/IAllowanceTransfer.sol": No such file or directory (os error 2).
Check configured remappings..
--> "/Users/prb/Workspace/templates/foundry-template/lib/v2-periphery/src/types/Permit2.sol"
"permit2/interfaces/IAllowanceTransfer.sol"
Then, I ran forge remappings
, and got this:
```text @openzeppelin/contracts/=lib/v2-periphery/lib/openzeppelin-contracts/contracts/ @prb/math/=lib/v2-periphery/lib/v2-core/lib/prb-math/src/ @prb/proxy-test/=lib/v2-periphery/lib/prb-proxy/test/ @prb/proxy/=lib/v2-periphery/lib/prb-proxy/src/ @prb/test/=lib/prb-test/src/ @sablier/v2-core-script/=lib/v2-periphery/lib/v2-core/script/ @sablier/v2-core-test/=lib/v2-periphery/lib/v2-core/test/ @sablier/v2-core/=lib/v2-periphery/lib/v2-core/src/ @sablier/v2-periphery/=lib/v2-periphery/src/ ds-test/=lib/forge-std/lib/ds-test/src/ erc4626-tests/=lib/v2-periphery/lib/openzeppelin-contracts/lib/erc4626-tests/ forge-gas-snapshot/=lib/v2-periphery/lib/permit2/lib/forge-gas-snapshot/src/ forge-std/=lib/forge-std/src/ openzeppelin-contracts/=lib/v2-periphery/lib/openzeppelin-contracts/ openzeppelin/=lib/v2-periphery/lib/openzeppelin-contracts/contracts/ permit2-test/=lib/v2-periphery/lib/permit2/test/ permit2/=lib/v2-periphery/lib/permit2/ prb-math/=lib/v2-periphery/lib/v2-core/lib/prb-math/src/ prb-proxy/=lib/v2-periphery/lib/prb-proxy/ prb-test/=lib/prb-test/src/ solady/=lib/v2-periphery/lib/solady/ solarray/=lib/v2-periphery/lib/v2-core/lib/solarray/src/ solmate/=lib/v2-periphery/lib/permit2/lib/solmate/src/ src/=src/ v2-core/=lib/v2-periphery/lib/v2-core/ v2-periphery/=lib/v2-periphery/ ```
Notice the permit2
remapping, which is the problem here. Foundry does not pick our own permit2
remapping, and as a result, the inferred remapping lacks the /src
suffix (and I get the error shared above).
We can hot-fix this by asking consuming repos to add an explicit re-remapping, like this:
permit2/=lib/v2-periphery/lib/permit2/src/
But this is a chore - for us and our users - and it's not scalable either (what happens when we have another dependency?).
Another hot-fix would be to rename the permit2
remapping to smth like @uniswap/permit2
. However, this would be another non-scalable approach because it assumes that all dependencies in the lib
tree have remappings that do not match the submodule names. Requiring users to do this in perpetuity would be a chore and a footgun.
this would definitely be a big QoL upgrade and lacking support is becoming more and more painful.
let's tackle this shortly @Evalir
Hi guys,
I am looking for up to date minimal repros to validate #5397. I've tested the repository in #3440.
Lmk
I am looking for up to date minimal repros to validate #5397.
You could try sablier-labs/examples
at this commit: eae781627e15d963a62c1545fe0944ad94d5519c.
Is this a correct way to override mappings from /lib/creator-token-contracts/
context?
lib/creator-token-contracts:@openzeppelin/=lib/openzeppelin-contracts/
here's the remappings in /lib/creator-token-contracts/remappings.txt
@openzeppelin/=node_modules/@openzeppelin/
ds-test/=lib/forge-std/lib/ds-test/src/
forge-std/=lib/forge-std/src/
hardhat/=node_modules/hardhat/
murky/=lib/murky/src
we want the creator-token-contracts
to use our higher level remapping of OpenZeppelin
higher level remappings
ERC721A-Upgradeable/=lib/ERC721A-Upgradeable/contracts/
ERC721A/=lib/ERC721A/contracts/
erc721a/=lib/ERC721A/
ds-test/=lib/forge-std/lib/ds-test/src/
erc4626-tests/=lib/openzeppelin-contracts/lib/erc4626-tests/
forge-std/=lib/forge-std/src/
ERC721C/=lib/creator-token-contracts/contracts/
openzeppelin-contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/
openzeppelin-contracts/=lib/openzeppelin-contracts/contracts/
@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/
@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/
QUESTION: IS THIS MAPPING CORRECT?
lib/creator-token-contracts:@openzeppelin/=lib/openzeppelin-contracts/
works locally, but, in Github Actions we see
@SweetmanTech see https://github.com/foundry-rs/foundry/issues/5447, it may be related to your issue.
Component
Forge
Describe the feature you would like
Currently forge picks up remappings from imported libraries as well, if a nested library and the project use the same remapping but for different targets (different version of the same dependency for example), the current behaviour is that the project's remapping overrides the nested lib's remapping, which may not be intended.
solc remappings support a
context:
prefix to circumvent this:https://docs.soliditylang.org/en/latest/path-resolution.html#import-remapping
context:prefix=target
where
context
must match the beginning of the source unit name of the file containing the import.so that solc can be invoked like
This would require some upstream changes in ethers-rs:
Remapping
this would also require us to track where the Remapping is coming from so we can derive the
context
(which should be the relative pathlib/<dep>/
I think)Additional context
No response