Add onlyNamespace modifier and improve tests

packages/store-consumer/src/experimental/WithWorld.sol

@@ -18,6 +18,7 @@ abstract contract WithWorld is WithStore, System {
   error WithWorld_RootNamespaceNotAllowed();
   error WithWorld_NamespaceAlreadyExists();
   error WithWorld_NamespaceDoesNotExists();
+  error WithWorld_CallerHasNoNamespaceAccess();
   error WithWorld_CallerIsNotWorld();
 
   modifier onlyWorld() {
@@ -27,6 +28,14 @@ abstract contract WithWorld is WithStore, System {
     _;
   }
 
+  modifier onlyNamespace() {
+    // We use WorldContextConsumer directly as we already know the world is the caller
+    if (!_callerIsWorld() || !ResourceAccess.get(getNamespaceId(), WorldContextConsumer._msgSender())) {
+      revert WithWorld_CallerHasNoNamespaceAccess();
+    }
+    _;
+  }
+
   constructor(IBaseWorld _world, bytes14 _namespace, bool registerNamespace) WithStore(address(_world)) {
     if (_namespace == bytes14(0)) {
       revert WithWorld_RootNamespaceNotAllowed();

packages/store-consumer/test/StoreConsumer.t.sol

@@ -52,7 +52,11 @@ contract MockWithWorld is WithWorld, MockStoreConsumer {
     getWorld().transferOwnership(getNamespaceId(), to);
   }
 
+  function callableByAnyone() external view {}
+
   function onlyCallableByWorld() external view onlyWorld {}
+
+  function onlyCallableByNamespace() external view onlyNamespace {}
 }
 
 contract StoreConsumerTest is Test, GasReporter {
@@ -78,7 +82,8 @@ contract StoreConsumerTest is Test, GasReporter {
     assertTrue(ResourceIds.getExists(namespaceId), "Namespace not registered");
   }
 
-  function testOnlyWorld() public {
+  // Test internal MUD access control
+  function testAccessControl() public {
     IBaseWorld world = createWorld();
     StoreSwitch.setStoreAddress(address(world));
 
@@ -89,16 +94,71 @@ contract StoreConsumerTest is Test, GasReporter {
     MockWithWorld mock = new MockWithWorld(world, namespace, true);
     mock.transferNamespaceOwnership(address(this));
 
-    // Register the mock as a system
+    // Register the mock as a system with PRIVATE access
     world.registerSystem(systemId, mock, false);
 
     address alice = address(0x1234);
 
     vm.prank(alice);
     vm.expectRevert();
-    mock.onlyCallableByWorld();
+    world.call(systemId, abi.encodeCall(mock.callableByAnyone, ()));
 
+    // After granting access to namespace, it should work
     world.grantAccess(namespaceId, alice);
+    vm.prank(alice);
+    world.call(systemId, abi.encodeCall(mock.callableByAnyone, ()));
+  }
+
+  function testOnlyWorld() public {
+    IBaseWorld world = createWorld();
+    StoreSwitch.setStoreAddress(address(world));
+
+    bytes16 systemName = "mySystem";
+    bytes14 namespace = "myNamespace";
+    ResourceId systemId = WorldResourceIdLib.encode(RESOURCE_SYSTEM, namespace, systemName);
+    MockWithWorld mock = new MockWithWorld(world, namespace, true);
+    mock.transferNamespaceOwnership(address(this));
+
+    // Register the mock as a system with PUBLIC access
+    world.registerSystem(systemId, mock, true);
+
+    address alice = address(0x1234);
+
+    vm.prank(alice);
+    vm.expectRevert();
+    mock.onlyCallableByWorld();
+
+    vm.prank(alice);
     world.call(systemId, abi.encodeCall(mock.onlyCallableByWorld, ()));
   }
+
+  function testOnlyNamespace() public {
+    IBaseWorld world = createWorld();
+    StoreSwitch.setStoreAddress(address(world));
+
+    bytes16 systemName = "mySystem";
+    bytes14 namespace = "myNamespace";
+    ResourceId namespaceId = WorldResourceIdLib.encodeNamespace(namespace);
+    ResourceId systemId = WorldResourceIdLib.encode(RESOURCE_SYSTEM, namespace, systemName);
+    MockWithWorld mock = new MockWithWorld(world, namespace, true);
+    mock.transferNamespaceOwnership(address(this));
+
+    // Register the mock as a system with PUBLIC access
+    world.registerSystem(systemId, mock, true);
+
+    address alice = address(0x1234);
+
+    vm.prank(alice);
+    vm.expectRevert();
+    mock.onlyCallableByNamespace();
+
+    vm.prank(alice);
+    vm.expectRevert();
+    world.call(systemId, abi.encodeCall(mock.onlyCallableByNamespace, ()));
+
+    // After granting access to namespace, it should work
+    world.grantAccess(namespaceId, alice);
+    vm.prank(alice);
+    world.call(systemId, abi.encodeCall(mock.onlyCallableByNamespace, ()));
+  }
 }

packages/world-module-erc20/src/examples/ERC20WithWorld.sol

@@ -23,22 +23,22 @@ contract ERC20WithWorld is WithWorld, MUDERC20, ERC20Pausable, ERC20Burnable {
     world.transferOwnership(getNamespaceId(), _msgSender());
   }
 
-  function mint(address to, uint256 value) public onlyWorld {
+  function mint(address to, uint256 value) public onlyNamespace {
     _mint(to, value);
   }
 
-  function pause() public onlyWorld {
+  function pause() public onlyNamespace {
     _pause();
   }
 
-  function unpause() public onlyWorld {
+  function unpause() public onlyNamespace {
     _unpause();
   }
 
   // The following functions are overrides required by Solidity.
 
   function _msgSender() public view override(Context, WithWorld) returns (address sender) {
-    return super._msgSender();
+    return WithWorld._msgSender();
   }
 
   function _update(address from, address to, uint256 value) internal override(MUDERC20, ERC20Pausable) {

packages/world-module-erc20/src/experimental/ERC20Module.sol

@@ -44,8 +44,8 @@ contract ERC20Module is Module {
     // Grant access to the token so it can write to tables after transferring ownership
     world.grantAccess(namespaceId, address(token));
 
-    // Register token as a system so `onlyWorld` functions can be called through the world
-    world.registerSystem(systemId, token, false);
+    // Register token as a system so its functions can be called through the world
+    world.registerSystem(systemId, token, true);
 
     // The token should have transferred the namespace ownership to this module in its constructor
     world.transferOwnership(namespaceId, _msgSender());

packages/world-module-erc20/test/ERC20BaseTest.t.sol

@@ -55,6 +55,11 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
 
   function createToken() internal virtual returns (MockERC20Base);
 
+  // Used for validating the addresses in fuzz tests
+  function validAddress(address addr) internal view returns (bool) {
+    return addr != address(0) && addr != StoreSwitch.getStoreAddress();
+  }
+
   // TODO: startGasReport should be marked virtual so we can override
   function startGasReportWithPrefix(string memory name) internal {
     startGasReport(reportNameWithPrefix(name));
@@ -213,7 +218,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testMint(address to, uint256 amount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
 
     vm.expectEmit(true, true, true, true);
     emit Transfer(address(0), to, amount);
@@ -224,7 +229,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testBurn(address from, uint256 mintAmount, uint256 burnAmount) public {
-    vm.assume(from != address(0));
+    vm.assume(validAddress(from));
     vm.assume(burnAmount <= mintAmount);
 
     token.__mint(from, mintAmount);
@@ -237,15 +242,15 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testApprove(address to, uint256 amount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
 
     assertTrue(token.approve(to, amount));
 
     assertEq(token.allowance(address(this), to), amount);
   }
 
   function testTransfer(address to, uint256 amount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
     token.__mint(address(this), amount);
 
     vm.expectEmit(true, true, true, true);
@@ -262,9 +267,9 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testTransferFrom(address spender, address from, address to, uint256 approval, uint256 amount) public {
-    vm.assume(from != address(0));
-    vm.assume(to != address(0));
-    vm.assume(spender != address(0));
+    vm.assume(validAddress(from));
+    vm.assume(validAddress(to));
+    vm.assume(validAddress(spender));
     vm.assume(amount <= approval);
 
     token.__mint(from, amount);
@@ -294,7 +299,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testBurnInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 burnAmount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
     vm.assume(mintAmount < type(uint256).max);
     vm.assume(burnAmount > mintAmount);
 
@@ -304,7 +309,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testTransferInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 sendAmount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
     vm.assume(mintAmount < type(uint256).max);
     vm.assume(sendAmount > mintAmount);
 
@@ -314,7 +319,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testTransferFromInsufficientAllowanceReverts(address to, uint256 approval, uint256 amount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
     vm.assume(approval < type(uint256).max);
     vm.assume(amount > approval);
 
@@ -330,7 +335,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er
   }
 
   function testTransferFromInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 sendAmount) public {
-    vm.assume(to != address(0));
+    vm.assume(validAddress(to));
     vm.assume(mintAmount < type(uint256).max);
     vm.assume(sendAmount > mintAmount);