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 cloning functionality #15

Closed
wants to merge 17 commits into from
Closed

Add cloning functionality #15

wants to merge 17 commits into from

Conversation

salieflewis
Copy link
Contributor

Just opening this for visibility, still a lot to do on the testing front...

@0xTranqui
Copy link
Member

@salieflewis this is super cool! im not familiar with clone/proxy set ups, so my initial question is is there a reason you went with this clone architecture, rather than the ERC71967Proxy implementation that Iain has used in both the CuratorFactory.sol as well as zora-drops-contracts?

@salieflewis
Copy link
Contributor Author

I was trying to figure out the simplest way to configure a factory, while being gas effective, and easy to implement. Clones follow almost all the same rules as the ERC1967Proxy impl, but they're different in that they're non-upgradeable. I didn't think that that was a necessary feature for this use case, but I could be wrong and am eager to hear your thoughts. Would also be nice to hear what Iain and Rohan would suggest.

@salieflewis salieflewis marked this pull request as draft November 7, 2022 15:56
@0xTranqui
Copy link
Member

@salieflewis so im still not sure what the answer is here and I havent dug into it yet. but I believe well want to be able to deploy one factory base, and then update the implementation of the factory / other contracts it points to -- without deploying a new factory base (so the address stays the same). that makes me think we want to go the 1967 route but not sure and I could be confusing thigns

@salieflewis
Copy link
Contributor Author

I'd like to say that I'll have time to work on this over the weekend! Going the UUPS/1967 route.

@salieflewis
Copy link
Contributor Author

This is a big PR, so going to articulate what the changes are in these comments, as well as remind myself what I've done. Kinda trying to again familiarize myself before moving away from Clones like discussed.

Changes to CustomPricingMinter.sol

  1. ReentrancyGuard has been swapped for the upgradeable version of the contract. The main difference is that the upgradeable version does not include a constructor.
  2. Ownable has been swapped for the version written by Rohan, which is essentially the OZ upgradeable version with a few tweaks.
  3. The constructor has been replaced with an initialize function which has the modifier called initializer that prevents this function from being called repeatedly. The other difference now is that the contract owner is declared when this initialize function is called, rather than configured when the contract is deployed.

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

@salieflewis salieflewis closed this Dec 9, 2022
@salieflewis salieflewis deleted the add-factory branch March 28, 2023 16:38
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