mario-eth / soldeer

Solidity Package Manager written in rust
MIT License
241 stars 26 forks source link

`remappings_regenerate` Config Not Respected and Incorrect Remapping Prefix Handling in `soldeer` #170

Closed icanvardar closed 2 months ago

icanvardar commented 3 months ago

Component

Soldeer

Have you ensured that all of these are up to date?

What version of Foundry are you on?

forge 0.2.0 (98ab45e 2024-08-30T00:20:35.025550372Z)

What command(s) is the bug in?

Both forge soldeer install, forge soldeer update

Operating System

Pop!_OS 22.04 LTS

Describe the bug

After installing the latest versions of both Foundry and Soldeer, I encountered an issue with the [soldeer] configuration in my foundry.toml file. The relevant section of my foundry.toml is as follows:

[dependencies]
 forge-std = { version = "1.9.1" }

[soldeer]
 remappings_generated = false
 remappings_regenerate = false
 remappings_version = false
 remappings_prefix = ""
 remappings_location = "txt"
 recursive_deps = false

The problem is that regardless of whether the remappings_regenerate setting is set to true or false, the remappings.txt file is always altered after running forge soldeer install.

For example, my remappings.txt file initially contained: forge-std=dependencies/forge-std-1.9.1/src

After running the forge soldeer install command, the file was modified to: forge-std/=dependencies/forge-std-1.9.1/

I also tried downloading Soldeer externally and ran soldeer install, but the result was the same.

Additionally, if a library starts with the @ sign (e.g., @openzeppelin-contracts), and remappings_prefix is set to "@", Soldeer adds another @ sign to the prefix in the remappings.txt file, resulting in @@openzeppelin-contracts.

cc: @whisskey

beeb commented 3 months ago

Hey! Thanks for the report. Regarding your second issue (prefix), it's actually the desired behavior. It serves to add an additional prefix compared to what the package name already is. Think of it as namespacing.

The remappings_regenerate option indicates whether the existing remappings should be completely ignored or not. In case it's false, then existing remappings for a dependency should NOT be modified, so there might be a bug there.

On a side note, the readme was fixed because there was a typo for remappings_generated -> remappings_generate

beeb commented 3 months ago

I think the problem lies here:

https://github.com/mario-eth/soldeer/blob/6162a5e29b74d9681173f523aba127ce62f72d08/src/config.rs#L652

The remapping is only kept intact if the right side matches the dependency path. Since the right side was modified, this condition fails and it gets overwritten. The fix might be to compare the prefix of the path instead of the full path.

mario-eth commented 2 months ago

This should be solved in the next foundry release, Please do foundryup tomorrow!