-
Notifications
You must be signed in to change notification settings - Fork 327
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
refactor: make bound
internal virtual
#92
Conversation
…together with DSTestPlus of solmate
What do you use from solmate's DSTestPlus that's not in forge-std? Wondering if there's more we should upstream instead of making this virtual as a patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this would require also making it internal
and making the one in Solmate virtual
. but then it would still require overriding them in your contract.
so, the best approach is as matt suggested or to create your own DSTestPlus
.
Sorry for leaving little context here The need to use both is for fuse-flywheel Pasting the context from a chat somewhere:
https://github.com/Rari-Capital/solmate/pull/279/files then in |
It sounds like a better solution here is that solmate's |
Some tests in Solmate rely on I still think using your own |
this is the plan for V7 yes, but for the master branch i still need those funcs rn |
Alright, let's resolve this. @transmissions11 are you down to make @vminkov I just looked at your code and wanted to point out you don't need to re-implement functions when overriding; do this instead: function bound(
uint256 x,
uint256 min,
uint256 max
) internal override(DSTestPlus, Test) returns (uint256) {
return Test.bound(x, min, max);
} |
Sounds like a solution to me. I changed it in this PR from Also, I have changed the PR to |
@ZeroEkkusu can you, please, approve a run of the github workflows so we can proceed to merge the change? Thanks |
You still need Solmate tho. |
bound
internal virtual
bound() needs to be virtual for Test to be compatible to be extended together with DSTestPlus of solmate