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

Addressing removal of mocks from forge-std v1.9.6 #181

Open
2 tasks done
Dhruv-Mishra opened this issue Feb 7, 2025 · 6 comments
Open
2 tasks done

Addressing removal of mocks from forge-std v1.9.6 #181

Dhruv-Mishra opened this issue Feb 7, 2025 · 6 comments
Labels
bug Something isn't working

Comments

@Dhruv-Mishra
Copy link
Contributor

Dhruv-Mishra commented Feb 7, 2025

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

  • This repo
  • Any dependencies (according to the package.json)

What command(s) is the bug in?

run pnpm test after running pnpm up

Operating System

Any

Describe the bug

As part of v.1.9.6 through this PR, forge-std deprecate MockERC20.sol and MockERC721 (among other things) which we leverage in some tests

@Dhruv-Mishra Dhruv-Mishra added the bug Something isn't working label Feb 7, 2025
@Dhruv-Mishra
Copy link
Contributor Author

@kopy-kat to address this, I think we can integrate OpenZeppelin's or Solady's ERC20Mock by including these mocks in packages or by adding sources locally/writing our own contract within modulekit. What do you suggest?

@kopy-kat
Copy link
Member

kopy-kat commented Feb 8, 2025

I think going with solady would make sense

@Dhruv-Mishra
Copy link
Contributor Author

I think going with solady would make sense

It looks like Solady’s published package does not include mocks by default, and other similar packages based on Solady also exclude them.
To work around this, I installed Solady using Forge with: forge install Vectorized/solady

This method works, but it pulls in the entire Solady repository into the lib folder, even though we only need the mock contracts. This significantly increases the project’s size.
I’ve implemented this approach and can submit a PR if you approve. However, I’d recommend directly adding only the source files for the two mock tokens we require instead, as it would keep the project small while ensuring we have exactly what we need. Let me know what you think!

@kopy-kat
Copy link
Member

yeah good point. I agree, copying over the mock erc20 seems like the best idea in that case

@Dhruv-Mishra
Copy link
Contributor Author

yeah good point. I agree, copying over the mock erc20 seems like the best idea in that case

Sounds good, pushing this

@Dhruv-Mishra
Copy link
Contributor Author

Raised PR here: #182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants