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

evm-contracts: Use forge-std instead of excerpted files #315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matejos
Copy link
Contributor

@matejos matejos commented Mar 12, 2024

No description provided.

forge install: forge-std

v1.7.4
Comment on lines +1 to +3
[submodule "packages/contracts/evm-contracts/lib/forge-std"]
path = packages/contracts/evm-contracts/lib/forge-std
url = https://github.com/foundry-rs/forge-std
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no other way to do this? Submodules are always an absolute nightmare to work with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default way forge is installed, but... I guess we could replace the submodule with the files directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any downside to the approach we had before where things are inline? I would prefer that over having to update every build tool going forward to support submodules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've ran into a few missing functions in the inline files, and I wasn't able to get them working just by adding them there. That's why I wanted to refactor into properly using forge-std. But in the end it is not a must-have, and if this would produce more problems than benefits, I'm fine with sunsetting this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants