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 11 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": 1438969
"gasUsed": 1439079
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallGas",
"name": "install keys in table module",
"gasUsed": 1438969
"gasUsed": 1439079
},
{
"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": 1438969
"gasUsed": 1439079
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookCompositeGas",
"name": "install keys in table module",
"gasUsed": 1438969
"gasUsed": 1439079
},
{
"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": 1438969
"gasUsed": 1439079
},
{
"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": 684371
"gasUsed": 684459
},
{
"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": 684371
"gasUsed": 684459
},
{
"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": 684371
"gasUsed": 684459
},
{
"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": 684371
"gasUsed": 684459
},
{
"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": 118175
"gasUsed": 118198
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -279,7 +279,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromSystemDelegation",
"name": "register a systembound delegation",
"gasUsed": 115728
"gasUsed": 115751
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -291,7 +291,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromTimeboundDelegation",
"name": "register a timebound delegation",
"gasUsed": 112651
"gasUsed": 112674
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -303,7 +303,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstall",
"name": "install unique entity module",
"gasUsed": 704240
"gasUsed": 704219
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand All @@ -315,7 +315,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstallRoot",
"name": "installRoot unique entity module",
"gasUsed": 673144
"gasUsed": 673145
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand Down
18 changes: 12 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": 12512902
"gasUsed": 12642949
},
{
"file": "test/World.t.sol",
Expand All @@ -81,7 +81,7 @@
"file": "test/World.t.sol",
"test": "testCallFromUnlimitedDelegation",
"name": "register an unlimited delegation",
"gasUsed": 47584
"gasUsed": 47607
},
{
"file": "test/World.t.sol",
Expand All @@ -105,7 +105,7 @@
"file": "test/World.t.sol",
"test": "testRegisterFunctionSelector",
"name": "Register a function selector",
"gasUsed": 84519
"gasUsed": 84542
},
{
"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": 80467
"gasUsed": 80445
},
{
"file": "test/World.t.sol",
"test": "testRegisterSystem",
"name": "register a system",
"gasUsed": 164385
"gasUsed": 164363
},
{
"file": "test/World.t.sol",
Expand All @@ -143,11 +143,17 @@
"name": "Write data to the table",
"gasUsed": 39047
},
{
"file": "test/World.t.sol",
"test": "testUnregisterNamespaceDelegation",
"name": "unregister a namespace delegation",
"gasUsed": 27062
},
{
"file": "test/World.t.sol",
"test": "testUnregisterUnlimitedDelegation",
"name": "unregister an unlimited delegation",
"gasUsed": 26173
"gasUsed": 26255
},
{
"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.

2 changes: 2 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.

10 changes: 6 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 @@ -141,11 +141,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 @@ -169,7 +170,7 @@ contract CoreModule is Module {
_registerRootFunctionSelector(BATCH_CALL_SYSTEM_ID, functionSignaturesBatchCall[i]);
}

string[13] memory functionSignaturesCoreRegistration = [
string[14] memory functionSignaturesCoreRegistration = [
// --- ModuleInstallationSystem ---
"installModule(address,bytes)",
// --- StoreRegistrationSystem ---
Expand All @@ -185,7 +186,8 @@ contract CoreModule is Module {
"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
21 changes: 20 additions & 1 deletion 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 @@ -53,7 +53,7 @@ contract AccessManagementSystem is System, LimitedCallContext {
* @param newOwner The address to which ownership should be transferred.
*/
function transferOwnership(ResourceId namespaceId, address newOwner) public virtual onlyDelegatecall {
// Require the namespace to be a valid namespace ID
// Require the namespace ID to be a valid namespace
requireNamespace(namespaceId);

// Require the namespace to exist
Expand All @@ -71,4 +71,23 @@ contract AccessManagementSystem is System, LimitedCallContext {
// 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 onlyDelegatecall {
// Require the namespace ID to be a valid namespace
requireNamespace(namespaceId);

// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());
yonadaa marked this conversation as resolved.
Show resolved Hide resolved

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

// Revoke access from old owner
ResourceAccess._deleteRecord(namespaceId, _msgSender());
}
Copy link
Member

Choose a reason for hiding this comment

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

should we just call transferOwnership with address(0) here? Similar to the Ownable lib: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/e5c63635e3508a8d9d0afed091578cc4bb59a9c7/contracts/access/Ownable.sol#L77

Copy link
Member

Choose a reason for hiding this comment

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

Could transfer to address 0 and then delete the resource access record. No strong opinion though.

Copy link
Member

Choose a reason for hiding this comment

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

that would basically be the same logic but a few extra writes, wouldn't it?

Copy link
Contributor Author

@yonadaa yonadaa Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting pattern, but it ~doubles the gas from 35488 to 74700. Plus, setting then deleting the ResourceAccess record seems potentially more confusing then not? Unless folks feel strongly i think the inlined code is fine

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 this is fine how it is, but I did make a code suggestion below for completeness with other audit fixes (adding namespace existence check for better reverts)

}
27 changes: 24 additions & 3 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 @@ -44,7 +44,7 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
* @param namespaceId The unique identifier for the new namespace
*/
function registerNamespace(ResourceId namespaceId) public virtual onlyDelegatecall {
// Require namespace to be a valid namespace ID
// Require namespace ID to be a valid namespace
requireNamespace(namespaceId);

// Require namespace to not exist yet
Expand Down Expand Up @@ -284,7 +284,12 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
}
}

function unregisterDelegation(address delegatee) public {
/**
* @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 onlyDelegatecall {
// Delete the delegation control contract address
UserDelegationControl.deleteRecord({ delegator: _msgSender(), delegatee: delegatee });
}
Expand All @@ -301,7 +306,7 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
ResourceId delegationControlId,
bytes memory initCallData
) public onlyDelegatecall {
// Require namespace to be a valid namespace ID
// Require namespace ID to be a valid namespace
requireNamespace(namespaceId);

// Require the delegation to not be unlimited
Expand Down Expand Up @@ -329,4 +334,20 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
});
}
}

/**
* @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 onlyDelegatecall {
// Require namespace ID to be a valid namespace
requireNamespace(namespaceId);

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

Choose a reason for hiding this comment

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

Suggested change
// Require namespace ID to be a valid namespace
requireNamespace(namespaceId);
// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());
// Require namespace ID to be a valid namespace
requireNamespace(namespaceId);
// Require the namespace to exist
AccessControl.requireExistence(namespaceId);
// Require the caller to own the namespace
AccessControl.requireOwner(namespaceId, _msgSender());


// Delete the delegation control
NamespaceDelegationControl.deleteRecord(namespaceId);
}
}
Loading
Loading