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

Add a test harness to ensure library is embedded, not linked #57

Closed
geoknee opened this issue Jul 30, 2021 · 2 comments · Fixed by #64
Closed

Add a test harness to ensure library is embedded, not linked #57

geoknee opened this issue Jul 30, 2021 · 2 comments · Fixed by #64

Comments

@geoknee
Copy link
Contributor

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.

...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.

Originally posted by @geoknee in #55 (comment)

@geoknee
Copy link
Contributor Author

geoknee commented Aug 5, 2021

This shouldn't be necessary, as the following lines will throw an error if linking is necessary:

before(async () => {
testConsumer = await (
await ethers.getContractFactory("TestConsumer")
).deploy();
await testConsumer.deployed();

The reason why we didn't catch this before (see #55), is that the linking or embedding decision is made by solidity not just by inspecting the library, but by inspecting which bits of the library are referenced in the consumer. So we ended up with an embedded library in this repo, since the consumer doesn't reference all of the functions in the library (some of which could be non-internal). But in other consumers (i.e. nitro-protocol) we did reference those functions and ended up with an embedded library.

@geoknee
Copy link
Contributor Author

geoknee commented Aug 5, 2021

I think referencing all of the functions of the library in the consumer is probably sufficient to close this issue, for now.

@geoknee geoknee closed this as completed Aug 6, 2021
@geoknee geoknee reopened this Aug 6, 2021
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 a pull request may close this issue.

1 participant