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.

Sign up for GitHub

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

refactor(world): encapsulate functionality [N-01] #2157

Merged
merged 15 commits into from
Jan 19, 2024
Merged

refactor(world): encapsulate functionality [N-01] #2157

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
15d6026
feat(world): add renounceOwnership function
yonadaa Jan 18, 2024
dcaff7b
feat: unregister namespace delegations
yonadaa Jan 18, 2024
06f2ca2
feat: add unregisterSystem
yonadaa Jan 18, 2024
d88c6a7
Merge remote-tracking branch 'origin/main' into yonadaaa/encapsulate-…
holic Jan 18, 2024
73b5ded
add namespace check
holic Jan 18, 2024
4659afa
use new namespace check in unregisterNamespaceDelegation
holic Jan 18, 2024
0d23f3b
Merge remote-tracking branch 'origin/main' into yonadaaa/encapsulate-…
yonadaa Jan 19, 2024
c7e270b
chore: remove unregisterSystem
yonadaa Jan 19, 2024
eaea087
fix: onlyDelegatecall
yonadaa Jan 19, 2024
201ca27
test: add unregisterNamespaceDelegation test
yonadaa Jan 19, 2024
2cbc4fd
comment
yonadaa Jan 19, 2024
d45070d
test: add gas report for renounce
yonadaa Jan 19, 2024
1860bbc
fix: require existance on renounce
yonadaa Jan 19, 2024
a2c92af
chore: gas report
yonadaa Jan 19, 2024
b673d76
fix: check existence in namespace
yonadaa Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions packages/world-modules/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallComposite",
"name": "install keys in table module",
"gasUsed": 1435380
"gasUsed": 1435490
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallGas",
"name": "install keys in table module",
"gasUsed": 1435380
"gasUsed": 1435490
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -93,13 +93,13 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallSingleton",
"name": "install keys in table module",
"gasUsed": 1435380
"gasUsed": 1435490
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookCompositeGas",
"name": "install keys in table module",
"gasUsed": 1435380
"gasUsed": 1435490
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -117,7 +117,7 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookGas",
"name": "install keys in table module",
"gasUsed": 1435380
"gasUsed": 1435490
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -135,7 +135,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testGetKeysWithValueGas",
"name": "install keys with value module",
"gasUsed": 681695
"gasUsed": 681783
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -153,7 +153,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testInstall",
"name": "install keys with value module",
"gasUsed": 681695
"gasUsed": 681783
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -165,7 +165,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testSetAndDeleteRecordHook",
"name": "install keys with value module",
"gasUsed": 681695
"gasUsed": 681783
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -183,7 +183,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testSetField",
"name": "install keys with value module",
"gasUsed": 681695
"gasUsed": 681783
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand Down Expand Up @@ -267,7 +267,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromCallboundDelegation",
"name": "register a callbound delegation",
"gasUsed": 118138
"gasUsed": 118179
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -279,7 +279,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromSystemDelegation",
"name": "register a systembound delegation",
"gasUsed": 115691
"gasUsed": 115732
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -291,7 +291,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromTimeboundDelegation",
"name": "register a timebound delegation",
"gasUsed": 112614
"gasUsed": 112655
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -303,7 +303,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstall",
"name": "install unique entity module",
"gasUsed": 702723
"gasUsed": 702743
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand All @@ -315,7 +315,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstallRoot",
"name": "installRoot unique entity module",
"gasUsed": 671743
"gasUsed": 671762
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand Down
12 changes: 6 additions & 6 deletions packages/world/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"file": "test/Factories.t.sol",
"test": "testWorldFactory",
"name": "deploy world via WorldFactory",
"gasUsed": 12514303
"gasUsed": 12709748
},
{
"file": "test/World.t.sol",
Expand All @@ -81,7 +81,7 @@
"file": "test/World.t.sol",
"test": "testCallFromUnlimitedDelegation",
"name": "register an unlimited delegation",
"gasUsed": 47532
"gasUsed": 47561
},
{
"file": "test/World.t.sol",
Expand All @@ -105,7 +105,7 @@
"file": "test/World.t.sol",
"test": "testRegisterFunctionSelector",
"name": "Register a function selector",
"gasUsed": 83123
"gasUsed": 83146
},
{
"file": "test/World.t.sol",
Expand All @@ -117,13 +117,13 @@
"file": "test/World.t.sol",
"test": "testRegisterRootFunctionSelector",
"name": "Register a root function selector",
"gasUsed": 80424
"gasUsed": 80425
},
{
"file": "test/World.t.sol",
"test": "testRegisterSystem",
"name": "register a system",
"gasUsed": 164348
"gasUsed": 164344
},
{
"file": "test/World.t.sol",
Expand All @@ -147,7 +147,7 @@
"file": "test/World.t.sol",
"test": "testUnregisterUnlimitedDelegation",
"name": "unregister an unlimited delegation",
"gasUsed": 26191
"gasUsed": 26209
},
{
"file": "test/WorldDynamicUpdate.t.sol",
Expand Down
2 changes: 2 additions & 0 deletions packages/world/src/codegen/interfaces/IAccessManagementSystem.sol
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/world/src/codegen/interfaces/IWorldRegistrationSystem.sol
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 7 additions & 4 deletions packages/world/src/modules/core/CoreModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,12 @@ contract CoreModule is Module {
* @dev Iterates through known function signatures and registers them.
*/
function _registerFunctionSelectors() internal {
string[3] memory functionSignaturesAccessManagement = [
string[4] memory functionSignaturesAccessManagement = [
// --- AccessManagementSystem ---
"grantAccess(bytes32,address)",
"revokeAccess(bytes32,address)",
"transferOwnership(bytes32,address)"
"transferOwnership(bytes32,address)",
"renounceOwnership(bytes32)"
];
for (uint256 i = 0; i < functionSignaturesAccessManagement.length; i++) {
_registerRootFunctionSelector(ACCESS_MANAGEMENT_SYSTEM_ID, functionSignaturesAccessManagement[i]);
Expand All @@ -168,7 +169,7 @@ contract CoreModule is Module {
_registerRootFunctionSelector(BATCH_CALL_SYSTEM_ID, functionSignaturesBatchCall[i]);
}

string[13] memory functionSignaturesCoreRegistration = [
string[15] memory functionSignaturesCoreRegistration = [
// --- ModuleInstallationSystem ---
"installModule(address,bytes)",
// --- StoreRegistrationSystem ---
Expand All @@ -180,11 +181,13 @@ contract CoreModule is Module {
"registerSystemHook(bytes32,address,uint8)",
"unregisterSystemHook(bytes32,address)",
"registerSystem(bytes32,address,bool)",
"unregisterSystem(bytes32)",
"registerFunctionSelector(bytes32,string)",
"registerRootFunctionSelector(bytes32,string,bytes4)",
"registerDelegation(address,bytes32,bytes)",
"unregisterDelegation(address)",
"registerNamespaceDelegation(bytes32,bytes32,bytes)"
"registerNamespaceDelegation(bytes32,bytes32,bytes)",
"unregisterNamespaceDelegation(bytes32)"
];
for (uint256 i = 0; i < functionSignaturesCoreRegistration.length; i++) {
_registerRootFunctionSelector(CORE_REGISTRATION_SYSTEM_ID, functionSignaturesCoreRegistration[i]);
Expand Down
16 changes: 16 additions & 0 deletions packages/world/src/modules/core/implementations/AccessManagementSystem.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,20 @@ contract AccessManagementSystem is System {
// Grant access to new owner
ResourceAccess._set(namespaceId, newOwner, true);
}

/**
* @notice Renounces ownership of the given namespace
* @dev Requires the caller to own the namespace. Revoke ResourceAccess for previous owner
* @param namespaceId The ID of the namespace to transfer ownership.
*/
function renounceOwnership(ResourceId namespaceId) public virtual {
holic marked this conversation as resolved.
Show resolved Hide resolved
// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());

// Delete namespace owner
NamespaceOwner._deleteRecord(namespaceId);

// Revoke access from old owner
ResourceAccess._deleteRecord(namespaceId, _msgSender());
}
}
57 changes: 57 additions & 0 deletions packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,40 @@ contract WorldRegistrationSystem is System, IWorldErrors {
ResourceAccess._set(namespaceId, address(system), true);
}

/**
* @notice Unregisters a system
* @dev Unregisters the system at the given ID
* @param systemId The unique identifier for the system
*/
function unregisterSystem(ResourceId systemId) public virtual {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we intentionally don't allow for unregistering systems? cc @alvrs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from @alvrs via discord

yeah it's this weird situation where other contracts that interact with yours might break if you unregister function selectors or unregister systems, so I think we should only encourage upgrading systems in a non-breaking way
if someone really wants to remove a system or function selector they can do it by editing the tables, but i don't think it should be something we encourage

// Require the provided system ID to have type RESOURCE_SYSTEM
if (systemId.getType() != RESOURCE_SYSTEM) {
revert World_InvalidResourceType(RESOURCE_SYSTEM, systemId, systemId.toString());
}

// Require the system's namespace to exist
ResourceId namespaceId = systemId.getNamespaceId();
AccessControl.requireExistence(namespaceId);

// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());

// Require the name to not be the namespace's root name
if (systemId.getName() == ROOT_NAME) revert World_InvalidResourceId(systemId, systemId.toString());

// Get the system at this system ID
address existingSystem = Systems._getSystem(systemId);

// Systems = mapping from system ID to system address and public access flag
Systems._deleteRecord(systemId);

// SystemRegistry = mapping from system address to system ID
SystemRegistry._deleteRecord(address(existingSystem));

// Delete the system access to its namespace
ResourceAccess._deleteRecord(namespaceId, address(existingSystem));
}

/**
* @notice Registers a new World function selector
* @dev Creates a mapping between a World function and its associated system function
Expand Down Expand Up @@ -260,6 +294,11 @@ contract WorldRegistrationSystem is System, IWorldErrors {
}
}

/**
* @notice Unregisters a delegation
* @dev Deletes the new delegation from the caller to the specified delegatee
* @param delegatee The address of the delegatee
*/
function unregisterDelegation(address delegatee) public {
// Delete the delegation control contract address
UserDelegationControl.deleteRecord({ delegator: _msgSender(), delegatee: delegatee });
Expand Down Expand Up @@ -307,4 +346,22 @@ contract WorldRegistrationSystem is System, IWorldErrors {
});
}
}

/**
* @notice Unregisters a delegation for a namespace
* @dev Deletes the delegation control for a specific namespace
* @param namespaceId The ID of the namespace
*/
function unregisterNamespaceDelegation(ResourceId namespaceId) public {
// Require the namespaceId to be a valid namespace ID
if (namespaceId.getType() != RESOURCE_NAMESPACE) {
revert World_InvalidResourceType(RESOURCE_NAMESPACE, namespaceId, namespaceId.toString());
}

// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());

// Register the delegation control
NamespaceDelegationControl.deleteRecord(namespaceId);
}
}
33 changes: 31 additions & 2 deletions packages/world/test/World.t.sol
View file
Open in desktop
yonadaa marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,12 @@ contract WorldTest is Test, GasReporter {
CoreRegistrationSystem coreRegistrationSystem = CoreRegistrationSystem(
Systems.getSystem(CORE_REGISTRATION_SYSTEM_ID)
);
bytes4[19] memory coreFunctionSignatures = [
bytes4[23] memory coreFunctionSignatures = [
// --- AccessManagementSystem ---
AccessManagementSystem.grantAccess.selector,
AccessManagementSystem.revokeAccess.selector,
AccessManagementSystem.transferOwnership.selector,
AccessManagementSystem.renounceOwnership.selector,
// --- BalanceTransferSystem ---
BalanceTransferSystem.transferBalanceToNamespace.selector,
BalanceTransferSystem.transferBalanceToAddress.selector,
Expand All @@ -247,10 +248,13 @@ contract WorldTest is Test, GasReporter {
coreRegistrationSystem.registerSystemHook.selector,
coreRegistrationSystem.unregisterSystemHook.selector,
coreRegistrationSystem.registerSystem.selector,
coreRegistrationSystem.unregisterSystem.selector,
coreRegistrationSystem.registerFunctionSelector.selector,
coreRegistrationSystem.registerRootFunctionSelector.selector,
coreRegistrationSystem.registerDelegation.selector,
coreRegistrationSystem.registerNamespaceDelegation.selector
coreRegistrationSystem.unregisterDelegation.selector,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof, we didn't have this before, eh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another reason for #2155

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this was forgotten. Although this is a test file so whatever automation we add needs to apply here too.

coreRegistrationSystem.registerNamespaceDelegation.selector,
coreRegistrationSystem.unregisterNamespaceDelegation.selector
];

for (uint256 i; i < coreFunctionSignatures.length; i++) {
Expand Down Expand Up @@ -426,6 +430,31 @@ contract WorldTest is Test, GasReporter {
world.transferOwnership(namespaceId, address(1));
}

function testRenounceNamespace() public {
bytes14 namespace = "testRenounce";
ResourceId namespaceId = WorldResourceIdLib.encodeNamespace(namespace);

world.registerNamespace(namespaceId);

// Expect the new owner to not be namespace owner before transfer
assertFalse(
(NamespaceOwner.get(namespaceId)) == address(0),
"new owner should not be namespace owner before transfer"
);

world.renounceOwnership(namespaceId);

// Expect the new owner to be zero address
assertEq(NamespaceOwner.get(namespaceId), address(0), "zero address should be namespace owner");

// Expect previous owner to no longer have access
assertEq(ResourceAccess.get(namespaceId, address(this)), false, "caller should no longer have access");

// Expect revert if caller is not the owner
_expectAccessDenied(address(this), namespace, 0, RESOURCE_NAMESPACE);
world.renounceOwnership(namespaceId);
}

function testRegisterTable() public {
FieldLayout fieldLayout = FieldLayoutEncodeHelper.encode(1, 32, 1);
Schema valueSchema = SchemaEncodeHelper.encode(SchemaType.BOOL, SchemaType.UINT256, SchemaType.STRING);
Expand Down
Loading