-
Notifications
You must be signed in to change notification settings - Fork 49
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 split code factory #1316
base: main
Are you sure you want to change the base?
Add split code factory #1316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a few minor comments; otherwise seems ready to go!
Perhaps we can also measure how much gas it takes to deploy the pool factory / pools now? It'd be great to confirm that we're now below the limit.
@@ -61,7 +67,7 @@ abstract contract BasePoolFactory is IBasePoolFactory, SingletonAuthentication, | |||
IVault vault, | |||
uint32 pauseWindowDuration, | |||
bytes memory creationCode | |||
) SingletonAuthentication(vault) FactoryWidePauseWindow(pauseWindowDuration) { | |||
) BaseSplitCodeFactory(creationCode) SingletonAuthentication(vault) FactoryWidePauseWindow(pauseWindowDuration) { | |||
_creationCode = creationCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete this; otherwise we're still using storage :)
_creationCode = creationCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L61 shall also be deleted with the reference to the private variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be used for computing the deployment address (which we didn't have in v2), but yes; we'd have to assemble it the same way vs. just storing it, or we'll have the same issue on Gnosis.
|
||
// From | ||
// https://github.com/Arachnid/solidity-stringutils/blob/b9a6f6615cf18a87a823cbc461ce9e140a61c305/src/strings.sol | ||
function _memcpy(uint256 dest, uint256 src, uint256 len) private pure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably use mcopy
opcode directly, but also fine to keep it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also questioned the storage; other than that, just some comment and standards things.
/** | ||
* @dev Returns the two addresses where the creation code of the contract crated by this factory is stored. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @dev Returns the two addresses where the creation code of the contract crated by this factory is stored. | |
*/ | |
/// @dev Returns the two addresses where the creation code of the contract created by this factory is stored. |
/** | ||
* @dev Returns the creation code of the contract this factory creates. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @dev Returns the creation code of the contract this factory creates. | |
*/ | |
/// @dev Returns the creation code of the contract this factory creates. |
@@ -61,7 +67,7 @@ abstract contract BasePoolFactory is IBasePoolFactory, SingletonAuthentication, | |||
IVault vault, | |||
uint32 pauseWindowDuration, | |||
bytes memory creationCode | |||
) SingletonAuthentication(vault) FactoryWidePauseWindow(pauseWindowDuration) { | |||
) BaseSplitCodeFactory(creationCode) SingletonAuthentication(vault) FactoryWidePauseWindow(pauseWindowDuration) { | |||
_creationCode = creationCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be used for computing the deployment address (which we didn't have in v2), but yes; we'd have to assemble it the same way vs. just storing it, or we'll have the same issue on Gnosis.
Description
The current factory implementation incurs high gas costs. Due to this inefficiency, some pool deployments now exceed 17M gas, making deployment on Gnosis Chain infeasible.
This PR implements a more gas-efficient alternative, directly based on Balancer V2, to reduce deployment costs.
Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution
Closes #1313