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

Add suggested remappings in readme #4440

Merged
merged 2 commits into from Jul 9, 2023
Merged

Add suggested remappings in readme #4440

merged 2 commits into from Jul 9, 2023

Conversation

allwin199
Copy link
Contributor

Added directions to remappings while working with foundry in README.md.

Fixes #4439

Added below lines to give directions about remappings


Note: Only while working with Foundry

Head to foundry.toml and add below line

remappings = ["@openzeppelin/=lib/openzeppelin-contracts/"]

Added directions to remappings while working with foundry in README.md.
@changeset-bot
Copy link

changeset-bot bot commented Jul 9, 2023

⚠️ No Changeset found

Latest commit: a5171eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

README.md Outdated Show resolved Hide resolved
Add `@openzeppelin/=lib/openzeppelin-contracts/` in `remappings.txt.`

Co-authored-by: Francisco <fg@frang.io>
@frangio frangio changed the title #4439 Update Remappings in README.md Add suggested remappings in readme Jul 9, 2023
@frangio frangio merged commit 2a4396c into OpenZeppelin:master Jul 9, 2023
16 of 17 checks passed
@gitpoap-bot
Copy link

gitpoap-bot bot commented Jul 9, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

GitPOAP: 2023 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@pcaversaccio
Copy link
Contributor

Just a quick point for discussion: I introduced as part of my PR #4134, the file remappings.txt:

openzeppelin/=contracts/

In this PR you start sticking to the @ convention:

@openzeppelin/=lib/openzeppelin-contracts/

So my recommendation is to either change remappings.txt to:

- openzeppelin/=contracts/
+ @openzeppelin/=contracts/

and adjust the remappings in the test files where used, or state in the README:

- Add `@openzeppelin/=lib/openzeppelin-contracts/` in `remappings.txt.`
+ Add `openzeppelin/=lib/openzeppelin-contracts/` in `remappings.txt.`

It's a consistency question.

@allwin199
Copy link
Contributor Author

Hey @pcaversaccio

Inside the package, I don't think @ is required.

What do you think?

@pcaversaccio
Copy link
Contributor

Inside the package, I don't think @ is required.

Actually, I never use @ in remappings; an example from my repo here:

utils/=lib/utils/
murky/=lib/murky/src/
solady/=lib/solady/src/
solmate/=lib/solmate/src/
prb/test/=lib/prb-test/src/
forge-std/=lib/forge-std/src/
erc4626-tests/=lib/erc4626-tests/
create-util/=lib/create-util/contracts/
openzeppelin/=lib/openzeppelin-contracts/contracts/
solidity-bytes-utils/=lib/solidity-bytes-utils/contracts/

Also, you might wanna consider openzeppelin/=lib/openzeppelin-contracts/contracts/ instead of openzeppelin/=lib/openzeppelin-contracts/.

@allwin199
Copy link
Contributor Author

Sure, I will use openzeppelin/=lib/openzeppelin-contracts/contracts/ instead of openzeppelin/=lib/openzeppelin-contracts/

openzeppelin/=lib/openzeppelin-contracts/contracts/

After adding this above line in my remappings.txt without @
If we use the below line, compiler will say file not found

import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";

to make our remappings to work, then we have to update import without @ as well

import {ERC721} from "openzeppelin/contracts/token/ERC721/ERC721.sol";

If we update our import statement we don't have to use @
But is it ok to remove @ from import, because It represents as scoped package
what do you think?

@pcaversaccio
Copy link
Contributor

Actually, if you use my remapping (including contracts/), it's enough to write:

import {ERC721} from "openzeppelin/token/ERC721/ERC721.sol";

Submodules are not like scoped packages since you can't target tags (branches are possible though). Thus, I don't think @ is necessary. @frangio what's your opinion on this?

@frangio
Copy link
Contributor

frangio commented Jul 10, 2023

Our documentation uses @openzeppelin/contracts in import statements following the npm package, and we would want those snippets to work in Foundry with the recommended remappings.

(Also note that OpenZeppelin builds things outside the Contracts library and simply openzeppelin/ doesn't reflect that.)

@allwin199 allwin199 deleted the #4439-update-remapping-readme branch July 21, 2023 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we update Remapping details while working with Foundry
3 participants