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

ensure all functions are internal #55

Merged
merged 1 commit into from
Jul 30, 2021
Merged

ensure all functions are internal #55

merged 1 commit into from
Jul 30, 2021

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Jul 30, 2021

This means the library is an Embedded library, not a Linked library

https://medium.com/coinmonks/all-you-should-know-about-libraries-in-solidity-dd8bc953eae7

@geoknee geoknee self-assigned this Jul 30, 2021
@geoknee
Copy link
Contributor Author

geoknee commented Jul 30, 2021

We might be able to write a test that fails if we accidentally went back down the linked library route. For example, we could parse the transaction output of one of our tests and assert that there are no DELEGATECALL opcodes.

@geoknee geoknee requested a review from NiloCK July 30, 2021 13:14
@geoknee
Copy link
Contributor Author

geoknee commented Jul 30, 2021

...or we could ensure that our test suite records the gas consumed down enough execution pathways to detect the increased gas usage of DELEGATECALL. I think that is not currently the case because we only use the types, not the functions, exposed by the library.

Either strategy requires greater test coverage.

Copy link
Contributor

@NiloCK NiloCK left a comment

Choose a reason for hiding this comment

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

Hooray for deployable contracts.

Notable here is that WithdrawHelper and ERC20Interface still declare external functions and are consumed by the library. I understand this to be fine because they do not describe implementations that a consumer of exit-format would be required to run.

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.

None yet

2 participants