foundry-rs / compilers

Utilities for working with native solc and compiling projects.
Apache License 2.0
59 stars 36 forks source link

Inconsistent tailing slash in remapping name and path #47

Closed Troublor closed 1 week ago

Troublor commented 5 months ago

Hi, I notice that when formatting remappings in the compiler settings, a slash / is pushed if the remapped path does not end with /. https://github.com/foundry-rs/compilers/blob/6528e4ac0b8f17599fd4b6b1e091b21c690ad142/src/remappings.rs#L160-L162

This may lead to incorrect path remapping and cause compiler failure. For example, if the original remapping is @0x/contracts-utils=/home/cluracan/code/0x-monorepo/node_modules/@0x/contracts-utils, and one more / is added at the end. The remapping now becomes: @0x/contracts-utils=/home/cluracan/code/0x-monorepo/node_modules/@0x/contracts-utils/.

If the contract has one import like this: import "@0x/contracts-utils/contracts/src/v06/LibBytesV06.sol"; The imported path after remapping is /home/cluracan/code/0x-monorepo/node_modules/@0x/contracts-utils//contracts/src/v06/LibBytesV06.sol. Note that there is one redundant / in the remapped path and this leads to File not found errors in the compiler.

I encounter this issue when trying to compile mainnet contract 0xDef1C0ded9bec7F1a1670819833240f027b25EfF with the source code provided by Etherscan API.

mattsse commented 5 months ago

could you please prepare a simple repro for this?

Troublor commented 5 months ago

@mattsse Please check this repo for reproduce this issue: https://github.com/Troublor/foundry-rs-compilers-issue47

I put some comments in the src/main.rs to illustrate the situation.

mattsse commented 5 months ago

interesting, looks like this affects solc versions < 0.8.8

I think we should remove the trailing / then, in serialization. looks like pre and post missing trailing / is handled properly, so there's probably some join bug in solc that has been fixed via the base path/allow path features in 0.8.8

This makes me think that we actually want the inverse here? if it ends with a /, remove that

could you convert this issue into a test and open a PR so we can fix this?

Troublor commented 5 months ago

Sure I can have a PR, but just be sure, what is the reason for appending a trailing / in the first place? I feel there must be a reason (e.g., handle some corner cases) in the previous commit.

BlazeWasHere commented 3 months ago

adding to this issue, there are few more places where a trailing / is added (not sure if its removed later in the call frame)

https://github.com/foundry-rs/compilers/blob/342f713da41c9738c6e775b7daf0bf6827041cde/src/remappings.rs#L178-L181 https://github.com/foundry-rs/compilers/blob/342f713da41c9738c6e775b7daf0bf6827041cde/src/remappings.rs#L245-L263 https://github.com/foundry-rs/compilers/blob/342f713da41c9738c6e775b7daf0bf6827041cde/src/remappings.rs#L863-L866