-
Notifications
You must be signed in to change notification settings - Fork 187
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
feat(world): add world factory #1385
Conversation
|
function deployWorld() public { | ||
bytes memory bytecode = type(World).creationCode; | ||
address worldAddress = Create2.create2Deploy(bytecode, worldCount); | ||
IBaseWorld world = IBaseWorld(worldAddress); | ||
world.installRootModule(coreModule, new bytes(0)); | ||
emit WorldDeployed(worldAddress); | ||
worldCount++; | ||
} |
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.
Should we transfer the ownership of the root module to the caller after installing the root module? (Or maybe we could make it optional and emit the selected option as part of the event? The interesting thing with a deploy function that does not transfer ownership of the root module is that users can be sure the World behaves exactly as the audited vanilla World)
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.
As it currently stands it would be the Factory address that would be the owner? I imagine the caller would probably assume they will be the owner so maybe that is best although I'm happy to take direction as I don't have a full understanding of the knock on effects.
address calculatedAddress = calculateAddress(address(create2Factory), bytes32(0), combinedBytes); | ||
// Confirm event for deployment | ||
vm.expectEmit(true, false, false, false); | ||
emit ContractDeployed(calculatedAddress, uint256(0)); |
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.
Is it possible to use emit Create2Factory.ContractDeployed
here (and in the other places) to avoid having to redefine the events in the test?
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.
I can't seem to make this work. I noticed in other tests the events are redefined, e.g. World.t.sol. Could you point me to somewhere I can use as an example if it is possible?
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.
seems to not be possible yet, but soon: ethereum/solidity#14274
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.
looks like it's part of the 0.8.21 release: https://github.com/ethereum/solidity/releases/tag/v0.8.21
Allow qualified access to events from other contracts.
@dev Helper Contract to facilitate create2 deployment of Contracts. | ||
*/ | ||
contract Create2Factory { | ||
event ContractDeployed(address addr, uint256 salt); |
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.
is this a standard somewhere? should we have an index on addr?
WorldFactory allows new worlds to be deployed (with a predefined CoreModule installed) using create2
Create2Factory is a stand alone helper contract that can be used to deploy any contract using create2. Eventually this could be used to deploy WorldFactory, CoreModule, Systems, etc.
Deploy script will be updated to use factory in a future PR after this and #1384 have been finalised.