optimism icon indicating copy to clipboard operation
optimism copied to clipboard

Use file-relative paths instead of project-relative

Open adraffy opened this issue 1 year ago • 6 comments

Is your feature request related to a problem? Please describe.

These project relative paths:

https://github.com/ethereum-optimism/optimism/blob/a4c47fe1cdb8b83bfc27482e0e9fc84d3b62b65c/packages/contracts-bedrock/src/libraries/Encoding.sol#L4

Describe the solution you'd like

Should be changed to file relative paths:

import { Types } from "./Types.sol";

Additional context

For example, these files cannot be used in Remix as the npm imports cannot disambiguate "src/"

This also causes issues with Foundry, where the same file is imported twice causing T != T errors.

adraffy avatar Apr 18 '24 04:04 adraffy

Can you give a reproducible case that shows this error? Imports using this style are part of our style guide. They make refactoring much easier internally to the project. Perhaps we could raise this as an issue with remix?

tynes avatar Apr 18 '24 18:04 tynes

Yeah, I can submit a foundry repo and a remix project example.

IMO, that's a weird style guide recommendation, as the file is only refactored once. Every importer pays the price of translating project-relative includes.

For example, all OpenZeppelin and forge-std includes are file-relative.

adraffy avatar Apr 18 '24 20:04 adraffy

https://github.com/adraffy/optimism-project-relative-issue/blob/main/src/Contract.sol

  1. foundryup
  2. forge build

Compiler run failed: Error (9553): Invalid type for argument in function call. Invalid implicit conversion from struct Types.OutputRootProof memory to struct Types.OutputRootProof memory requested.

adraffy avatar Apr 21 '24 09:04 adraffy

Thank you for the repro case

tynes avatar Apr 23 '24 05:04 tynes

You can use the following one-liner to repro in Remix:

import "@eth-optimism/contracts-bedrock/src/libraries/Encoding.sol";

This will correctly import via NPM then fail with with: Error: not found src/libraries/rlp/RLPWriter.sol

adraffy avatar Apr 23 '24 05:04 adraffy

I see, so relative paths are going to be more portable. I am curious if the following comment helps for the foundry case: https://github.com/foundry-rs/foundry/issues/3440#issuecomment-1472396987

tynes avatar Apr 24 '24 03:04 tynes