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

fix(contract): depositToAave gas optimization #15

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

@aodhgan
Copy link
Contributor

aodhgan commented Jun 29, 2021

Oh so the gas savings with return values is only for external/public functions? Can we run the gas reporter on this?

@aodhgan
Copy link
Contributor

aodhgan commented Jun 29, 2021

ACK

@PierrickGT
Copy link
Contributor Author

PierrickGT commented Jun 30, 2021

Oh so the gas savings with return values is only for external/public functions? Can we run the gas reporter on this?

Yes, that's what we recommend in our doc: https://docs.pooltogether.com/contributing/smart-contract-guidelines#optimizations

On the main branch, supplyTokenTo consumes on average 157,718 units of gas and on this branch, it consumes on average 132,990 units of gas.

We should setup some CI integrations on our repos to see how gas consumption changes between PRs. Here is a setup guide: https://github.com/cgewecke/eth-gas-reporter/blob/master/docs/codechecks.md

@PierrickGT PierrickGT changed the base branch from main to fixes/c4-audit June 30, 2021 13:18
@PierrickGT PierrickGT merged commit 5abdc08 into fixes/c4-audit Jul 5, 2021
@PierrickGT PierrickGT deleted the fixes/shw-122 branch July 5, 2021 08:26
@kamescg
Copy link
Contributor

kamescg commented Jul 8, 2021

Oh so the gas savings with return values is only for external/public functions? Can we run the gas reporter on this?

The gas savings is only for external/public functions which are called from another smart contract. There is no gas savings when calling from an EOA - in fact it might actually be a bit more for an EOA if returning a variable, instead of nothing.

The PR looks good to me.

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