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

[yonadaa]

A few suggestions were fixed in previous PR's.

#2007 fixed registerTable:

The registerTable function might register a namespace as well.

#2007 fixed registerSystem:

The registerSystem function might register a namespace

#2096 added unregisterDelegation:

Individual delegations must be removed by overwriting them using the regular registration function, which still leaves a stray record in the UserDelegationControl table.

[changeset-bot]

⚠️ No Changeset found

Latest commit: b673d76

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[holic]

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

[holic]

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

[holic]

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

[holic]

another reason for #2155

[yonadaa]

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

[alvrs]

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

[alvrs]

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

[holic]

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

[yonadaa]

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

[holic]

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)

[holic]

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());