feat(world-modules): add puppet module #1793
Conversation
🦋 Changeset detectedLatest commit: 4af069d The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| bytes32 eventSignature, | ||
| bytes32 topic1, | ||
| bytes32 topic2, | ||
| bytes32 topic3, |
There was a problem hiding this comment.
are these essentially the indexed args?
|
|
||
| ```solidity | ||
| import { PuppetModule } from "@latticexyz/world-modules/src/modules/puppet/PuppetModule.sol"; | ||
| import { createPuppet } from "@latticexyz/world-modules/src/modules/puppet/createPuppet.sol"; |
There was a problem hiding this comment.
Although the puppet terminology is fun, I wonder if we should just call this a proxy 🙈
There was a problem hiding this comment.
Thought about that too but I'm afraid proxy has too many preconceptions around the proxy being the authority (ie proxy holds the state, proxy switches its implementation, etc), whereas here the "proxy" is just the "puppet" of the World
| import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; | ||
| import { Systems } from "@latticexyz/world/src/codegen/tables/Systems.sol"; | ||
|
|
||
| contract Puppet { |
There was a problem hiding this comment.
pretty great to see how lean this is!
| contract PuppetMaster { | ||
| error PuppetMaster_NoPuppet(address systemAddress, ResourceId systemId); | ||
|
|
||
| function puppet() internal view returns (Puppet) { |
There was a problem hiding this comment.
does this mean there's only a 1:1 mapping between a system and a puppet?
if I want multiple tokens (e.g. ERC721) on my world, how would I go about that?
There was a problem hiding this comment.
does this mean there's only a 1:1 mapping between a system and a puppet
For now yes. Agree 1:N would be nicer but makes it much more complex, so I'd think about that later (either as a change to the puppet module, or with a new module)
if I want multiple tokens (e.g. ERC721) on my world, how would I go about that?
You'll have to register different systems (see #1844). Thought a lot about this too, but I feel like this approach is the simplest (one namespace per token, with a separate system and tables in that namespace). Otherwise everything gets more complicated (if it's just one system you'd need to pass a param to the system, which means the interface doesn't match anymore with the puppet, access management becomes more complex, etc)
| */ | ||
| function createPuppet(IBaseWorld world, ResourceId systemId) returns (address puppet) { | ||
| puppet = abi.decode( | ||
| world.call(PUPPET_FACTORY, abi.encodeCall(PuppetFactorySystem.createPuppet, (systemId))), |
There was a problem hiding this comment.
ooc why don't we use SystemSwitch here?
There was a problem hiding this comment.
Agree should probably use system switch here
There was a problem hiding this comment.
update: turns out using system switch here pushes ERC20Module over the bytecode limit, so reverting for now (meaning you can't register a puppet from a root system for now). We should find a better implementation for SystemSwitch that doesn't require inlining the libraries, maybe by deploying StoreCore as a public library.
There was a problem hiding this comment.
we should prob add a follow up issue to come back to 1) this specific instance and 2) the problem overall
StoreCore as a public lib seems bad for gas because it'd be a delegatecall to an external contract right? And iirc foundry doesn't have good support for linked libs
or do you mean public as in not-internal?
There was a problem hiding this comment.
yeah i mean public as in not-internal, which means delegatecall to an external contract
packages/world-modules/src/modules/puppet/PuppetDelegationControl.sol
Outdated
Show resolved
Hide resolved
| * This library is functionally equivalent with the AccessControl library from world, | ||
| * but uses StoreSwitch instead of always reading from own storage. | ||
| */ | ||
| library AccessControlLib { |
There was a problem hiding this comment.
Though I understand why it's in world-modules, would it maybe be better to have a single AccessControl lib with hasAccess/_hasAccess etc, to have all the logic in 1 place and let it mirror the table methods?
There was a problem hiding this comment.
agree! unfortunately can't touch the code in world while it's being audited but should note this down for later
| function puppet() internal view returns (Puppet) { | ||
| ResourceId systemId = SystemRegistry.getSystemId(address(this)); | ||
| address puppetAddress = PuppetRegistry.get(PUPPET_TABLE_ID, systemId); | ||
| if (puppetAddress == address(0)) revert PuppetMaster_NoPuppet(address(this), systemId); |
There was a problem hiding this comment.
does address(this) still work in the context of a delegatecall?
There was a problem hiding this comment.
There could only be a single puppet for root systems (because they all have the world address). There would also have to be a namespace delegation in the root namespace, which feels like a bad idea. So all in all i'd say you shouldn't use puppets with root systems.
Co-authored-by: dk1a <[email protected]>
No description provided.