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

expectCall cheatcode overloaded with msg.value assertion #1568

Closed
tynes opened this issue May 9, 2022 · 4 comments · Fixed by #1619
Closed

expectCall cheatcode overloaded with msg.value assertion #1568

tynes opened this issue May 9, 2022 · 4 comments · Fixed by #1619
Assignees
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-debug Command: forge run Cmd-forge-test Command: forge test D-easy Difficulty: easy good first issue Good for newcomers P-normal Priority: normal T-feature Type: feature

Comments

@tynes
Copy link
Contributor

tynes commented May 9, 2022

Component

Forge

Describe the feature you would like

Currently vm.expectCall operates on a target address and the calldata it expects

 function expectCall(address,bytes calldata) external;

When sending ETH through multiple contracts, it would be useful to have an overloaded expectCall that also accepts a value param that also asserts on call context's value (msg.value)

function expectCall(address,uint256 value,bytes calldata) external;

Didn't see this mentioned in any open issues, including #876

Additional context

No response

@tynes tynes added the T-feature Type: feature label May 9, 2022
@onbjerg onbjerg added this to Foundry May 9, 2022
@onbjerg onbjerg moved this to Todo in Foundry May 9, 2022
@onbjerg onbjerg added Cmd-forge-test Command: forge test C-forge Command: forge A-cheatcodes Area: cheatcodes Cmd-forge-debug Command: forge run good first issue Good for newcomers P-normal Priority: normal D-easy Difficulty: easy labels May 10, 2022
@AdithyaNarayan
Copy link
Contributor

Is this issue available? I would like to work on this as my first PR

@tynes
Copy link
Contributor Author

tynes commented May 13, 2022

I think overloading mockCall in the same way would be useful too.

Definitely feel free to open a PR, I don't know of anybody working on this now.

@AdithyaNarayan
Copy link
Contributor

AdithyaNarayan commented May 13, 2022

While overloading mockCall, I came across bug #1604. Is it ok if I do one PR to fix both of them? (Since #1604 is a one-line fix)

@onbjerg onbjerg moved this from Todo to In Progress in Foundry May 14, 2022
@AdithyaNarayan
Copy link
Contributor

Quick question, for the mockCall overload, which would take precedence, msg.value, or calldata?

vm.mockCall(address(target), 10, abi.encodeWithSelector(target.f.selector), abi.encode(10));
vm.mockCall(address(target), abi.encodeWithSelector(target.f.selector, 1), abi.encode(20));

assertEq(target.f{value: 10}(1) == ???);

What is the expected return data in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-debug Command: forge run Cmd-forge-test Command: forge test D-easy Difficulty: easy good first issue Good for newcomers P-normal Priority: normal T-feature Type: feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants