refactor: make bound internal virtual

[vminkov]

bound() needs to be virtual for Test to be compatible to be extended together with DSTestPlus of solmate

[mds1]

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

[ZeroEkkusu]

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.

[vminkov]

Sorry for leaving little context here

The need to use both is for fuse-flywheel

Pasting the context from a chat somewhere:

another suggestion to quickly fix the situation is to just change solmate/test/DSTestPlus.sol function bound() internal to public virtual which will make it compatible to be overridden together with the forge-std/Test.sol version of bound()

https://github.com/Rari-Capital/solmate/pull/279/files
https://github.com/foundry-rs/forge-std/pull/92/files

then in fuse-flywheel we can override all the clashing functions in a common test base BaseDSTest

https://github.com/fei-protocol/fuse-flywheel/pull/23/files#diff-cef27f927fef341c9a95838ad6d015729493778cc4758565922b34c3260d8e07

[mds1]

It sounds like a better solution here is that solmate's DSTestPlus should remove methods that are already in forge-std's Test, such as bound? cc @transmissions11

[ZeroEkkusu]

Some tests in Solmate rely on bound and it hasn't transitioned to Forge Std (transmissions11/solmate#214).

I still think using your own DSTestPlus is the simplest solution, if not we'll change bound to internal here (not to public in Solmate).

[transmissions11]

It sounds like a better solution here is that solmate's DSTestPlus should remove methods that are already in forge-std's Test, such as bound? cc @transmissions11

this is the plan for V7 yes, but for the master branch i still need those funcs rn

[ZeroEkkusu]

Alright, let's resolve this.

@transmissions11 are you down to make bound virtual in Solmate for the time being (transmissions11/solmate#279) so devs can use it if they really want to? This PR will make it internal virtual here.

@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);
    }

[vminkov]

Alright, let's resolve this.

@transmissions11 are you down to make bound virtual in Solmate for the time being (Rari-Capital/solmate#279) so devs can use it if they really want to? This PR will make it internal virtual here.

@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 public to internal. The tests with forge test are passing, not sure what else I can do to test the change?

Also, I have changed the PR to solmate to only make the bound() fn virtual and it stays internal (transmissions11/solmate#279)

[vminkov]

@ZeroEkkusu can you, please, approve a run of the github workflows so we can proceed to merge the change? Thanks

[ZeroEkkusu]

You still need Solmate tho.