search

cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

Deployment of Solidity Libraries with Internal Functions Only #1

Closed mudgen closed 1 year ago

mudgen commented 2 years ago

I briefly looked at the general implementation using EIP2535 Diamonds and it looks good. It is a good example of using Diamond Storage and Solidity libraries correctly in the code, in the implementation. Well done.

I noticed in the deployment that some Solidity libraries are being deployed and there is attempt to link them to some facets. For example this deployment file deploys the "LibClaimsMask" library and attempts to link it to several facets: https://github.com/cartesi/rollups/blob/main/onchain/rollups/deploy/03_facets.ts

However Solidity libraries that contain only internal functions do not need to be deployed and linked to contracts. Because internal functions of Solidity libraries are automatically added directly to the bytecode of contracts/facets that use them. Only Solidity libraries that contain external functions need to be deployed independently and linked. Attempting to deploy and link Solidity libraries with only internal functions is probably not doing anything. Some more information about this is here: https://eip2535diamonds.substack.com/p/how-to-share-functions-between-facets

guidanoli commented 2 years ago

Hello, Nick. I am very happy to know we're using the Diamond pattern correctly in the Rollups project! Having said that, I agree that linking to libraries that contain only internal functions is not necessary, and so we hope to address this issue shortly. Thank you for the valuable input!

guidanoli commented 1 year ago

This issue has been addressed in release 0.6.0 and as a result, gas costs for some function calls have been reduced as some costly DELEGATECALL opcodes are now simple JUMP opcodes.

Thanks again, Nick! :-)

mudgen commented 1 year ago

Great!