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

perf: use staticcall instead of call #383

Merged
merged 1 commit into from
May 24, 2023

Conversation

PaulRBerg
Copy link
Contributor

Unless I'm missing something, staticcall should be used instead of call for querying the token balances.

Cc @brockelmore.

uint256 prevBal = abi.decode(balData, (uint256));

// update balance
stdstore.target(token).sig(0x00fdd58e).with_key(to).with_key(id).checked_write(give);

// update total supply
if (adjust) {
(, bytes memory totSupData) = token.call(abi.encodeWithSelector(0xbd85b039, id));
(, bytes memory totSupData) = token.staticcall(abi.encodeWithSelector(0xbd85b039, id));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this PR seems like it should be ok: ERC-20 requires that balanceOf and totalSupply are view methods, and ERC-1155 requires that balanceOf is view also

Interestingly I didn't see the totalSupply specified in 1155 though—@wirew0lf any idea on if using a staticcall for the 1155 methods here might be problematic for some reason?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! yeah I agree it's better to use staticcall, good call. About the ERC115 @mds1 ERC1155 doesn't have a total supply by default, it is an extension ERC1155Supply.

On the cheatcode I acocunted for this and revert if adjust is set to true but contract is no ERC115Supply with the message target contract is not ERC1155Supply. But yeah on ERC1155Supply totalSupply() is also a view function so no problem by using staticcall.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah perfect, thanks @wirew0lf for the info and thanks @PaulRBerg for the PR!

@mds1 mds1 merged commit b490ef2 into foundry-rs:master May 24, 2023
@PaulRBerg PaulRBerg deleted the perf/static-call branch May 24, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants