Code Monkey home page Code Monkey logo

Comments (10)

DaniPopes avatar DaniPopes commented on June 3, 2024

cc @klkvr

from foundry.

klkvr avatar klkvr commented on June 3, 2024

expectRevert expects the next frame to revert. delegatecalls to a library is a frame as well

In your case getNumeratorC512 is a public function thus it is getting dispatched as a delegatecall and expectRevert works correctly.

However, calculate_c512 is an internal function which is getting inlined into a contract and no delegatecall is performed when it invoked (which can be seen in traces), thus the first frame seen by expectRevert is invocation of mul512x256

This is interesting though as I would expect invocation of getNumeratorC512 in calculate_c512 to perform a delegatecall, but it seems that if public library function is invoked in internal function of the same library, it will cause public fn to get inlined as well, which is weird as it would cause duplicating bytecode being deployed in both library and contract using it

from foundry.

oscarsernarosero avatar oscarsernarosero commented on June 3, 2024

Hm. That's a good find because I had forgotten about the mixed visibilities I had there. However, the only reason I started to make the other functions public was because the issue was already there when there was only 1 function calculate_c512, and I was not able to spot the problem thinking the error was in my code, so I made the other helper functions and made the library a regular contract at some point. Anyways, long story short, I tried making all the functions in library_A internal to test your point, so now getNumeratorC512 and calculate_c512 are both internal. I still have the same issue.

from foundry.

klkvr avatar klkvr commented on June 3, 2024

@oscarsernarosero mind sharing logs?

from foundry.

oscarsernarosero avatar oscarsernarosero commented on June 3, 2024

sure!

Traces:
  [14421] CofN512FuzzTests::testEquations_CofN512_CalculateNumeratorFuzz(4136799322481705091955576 [4.136e24], 134193314344108 [1.341e14], 100000000000000000 [1e17], 0)
    ├─ [0] console::log("Bound Result", 4136799322481705091955576 [4.136e24]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Bound Result", 134193314344108 [1.341e14]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Bound Result", 100000000000000000 [1e17]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
    │   └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
    ├─ [505] MathLibraryAbstractionLayer::mul512x256(8556554317242547139921794348727436964993978745888 [8.556e48], 0, 134193314344108 [1.341e14]) [delegatecall]
    │   └─ ← [Return] 1148232383196163535505025695706415258810544877106422631122027904 [1.148e63], 0
    ├─ [0] VM::expectRevert(ResultBelowPMin())
    │   └─ ← [Return] 
    ├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
    │   └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
    └─ ← [Revert] call did not revert as expected

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.82s (2.81s CPU time)

This is calling getNumeratorC512

from foundry.

oscarsernarosero avatar oscarsernarosero commented on June 3, 2024 
Traces:
  [14456] CofN512FuzzTests::testEquations_CofN512_CalculateCFuzz(4136799322481705091955576 [4.136e24], 134193314344108 [1.341e14], 100000000000000000 [1e17], 0)
    ├─ [0] console::log("Bound Result", 4136799322481705091955576 [4.136e24]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Bound Result", 134193314344108 [1.341e14]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Bound Result", 100000000000000000 [1e17]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
    │   └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
    ├─ [505] MathLibraryAbstractionLayer::mul512x256(8556554317242547139921794348727436964993978745888 [8.556e48], 0, 134193314344108 [1.341e14]) [delegatecall]
    │   └─ ← [Return] 1148232383196163535505025695706415258810544877106422631122027904 [1.148e63], 0
    ├─ [0] VM::expectRevert(ResultBelowPMin())
    │   └─ ← [Return] 
    ├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
    │   └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
    └─ ← [Revert] call did not revert as expected

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.83s (2.82s CPU time)

This is calling calculate_c512.

So it looks like you are into something here because now both calls fail. This is still not the desired behavior, but the function visibility definitely was causing one to fail the test and the other to pass. At least now both fail the test.

from foundry.

klkvr avatar klkvr commented on June 3, 2024

ah so right now when both functions are internal they are both getting inlined, thus first frame created with invocation of those fn is a MathLibraryAbstractionLayer delegatecall which does not revert

to test this you can either make both functions public so that each of them creates a new frame when invoked

also you can create a wrapper around them for testing or just a helper function on a test contract

for example

function calculate_c512(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) internal pure returns (uint256 Cn) {
    Xn.getNumeratorC512(Sn, DnL, DnH);
}

function test(....) public {
    vm.expectRevert(...);
    this.calculate_c512(...)
}

from foundry.

oscarsernarosero avatar oscarsernarosero commented on June 3, 2024

Yeah that would totally work. I do think Forge test should be smart enough to handle this tho.

from foundry.

klkvr avatar klkvr commented on June 3, 2024

unfortunately after compiler optimizations it is really hard to tell whether certain bytecode corresponds to an internal library, handling this is non-trivial just as handling reverts in internal contract functions for example

from foundry.

oscarsernarosero avatar oscarsernarosero commented on June 3, 2024

Ok I see. Thank you for your time @klkvr. This definitely helped me understand my issue even though it seems to be now in my hands to handle this kind of situations.

from foundry.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.