-
Notifications
You must be signed in to change notification settings - Fork 178
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.
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
feat(world): allow registration of function selectors in the World, split out RegisterSystem from World #481
Conversation
// TODO: autogenerate / auto-include all system interfaces | ||
import { IRegistrationSystem } from "./systems/IRegistrationSystem.sol"; | ||
|
||
interface IWorld is IStore, IWorldCore, IRegistrationSystem {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dk1a what do you think about adding an autogen function to create interfaces for systems that are registered in the World (defined via mud.config.mts
, then registered via registerFunctionSelector
/ registerRootFunctionSelector
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a limit to how many inheritance a contract can have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is, but haven't found explicit documentation about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dk1a what do you think about adding an autogen function to create interfaces for systems that are registered in the World (defined via
mud.config.mts
, then registered viaregisterFunctionSelector
/registerRootFunctionSelector
)?
Something like worldgen
. I think it's necessary for being able to interact with systems on-chain, and maybe helps the client too. Doing it manually would be a huge hassle with lots of room for error.
I don't think systems should actually implement their own interface, it'd just be generated along with World
based on manually defined public/external funcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just some high level questions, but nothing blocking. I didn't look super closely at all the Solidity, I trust you + your tests will enforce correctness.
As a small note, it would have been nice to review this as two separate PRs (e.g. RegisterSystem
in its own PR) to keep scope limited (both in terms of code and having to potentially revert later, and also for easier reviewing)
packages/world/src/Errors.sol
Outdated
error SystemExists(address system); | ||
error FunctionSelectorExists(bytes4 functionSelector); | ||
error FunctionSelectorNotFound(bytes4 functionSelector); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just saw a tweet about this:
https://twitter.com/PaulRBerg/status/1587041303015165952
sounds like errors and events in libraries won't have their ABI included in the contracts that use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, I guess we could add them to the IWorldCore
interface instead and use them from there in RegistrationSystem
(or inherit from a shared error interface? 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared error interface seems fine to me!
import { Bytes } from "@latticexyz/store/src/Bytes.sol"; | ||
|
||
bytes16 constant ROOT_NAMESPACE_STRING = bytes16("ROOT_NAMESPACE"); | ||
bytes16 constant ROOT_FILE_STRING = bytes16("ROOT_FILE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any guard rails to keep folks from registering using these constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, but also they're only used for logging, mostly in errors. Not worth the gas cost of adding checks to prevent them from being used as actual namespaces/files imo, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, I missed that this was only used in logging!
string( | ||
abi.encodePacked( | ||
namespace == ROOT_NAMESPACE ? ROOT_NAMESPACE_STRING : namespace, | ||
"/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any guard rails to keep folks from using /
in their namespace or filename and clobbering these "paths"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to the q above: we don't have checks that prevent folks from having /
in their namespace or file name on-chain right now, but since there is no security concern + this is only used for logging / error messages + the CLI prevents the use of special characters in namespace/file name, I think again not worth the gas cost of preventing this on-chain. The only one who gets hurt by using a /
is the person using it (because their logs might be harder to read), and it requires explicit effort to go around the CLI and use a /
uint256 length; | ||
for (; length < 16; length++) if (Bytes.slice1(selector, length) == 0) break; | ||
bytes memory packedSelector = abi.encodePacked(selector); | ||
return string(Bytes.setLength(packedSelector, length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just thinking aloud:
since we'll probably want to do more validating other than just null chars (e.g. slashes, underscores, non-alphanumeric, etc.), I wonder if its worth moving this logic to the client, and a Solidity function to just validate correctness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason we need this function is to create a custom function selector (<namespace>_<file>_<func>()
) in registerFunctionSelector
: the interface requires namespace and file to be bytes16
, and it's possible to pass any string shorter than 16 bytes as argument, but the remaining bytes will be 0. But to create the function selector we need to trim those 0s, because they influence the hash and therefore the function selector. As an example: assume namespace
is bytes16("mud")
, file
is bytes16("increment")
, then bytes4(keccak256(abi.encodePacked(namespace, "_", file, "_func()")))
isn't the same as bytes4(keccak256(abi.encodePacked("mud_increment_func()")))
, (but bytes4(keccak256(abi.encodePacked(trim(namespace), "_", trim(file), "_func()")))
is correct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words - we wouldn't need this if registerFunctionSelector
accepted a dynamic length bytes argument for namespace
and file
instead of a fixed bytes16
, but that would add more friction to consumers and more calldata (due to 32 bytes of length for dynamic length types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a binary search for reference (would need to be modified for bytes16
):
function findNull(uint256 _str) internal pure returns (uint256 result) {
result = 32;
// safe because result = 32, and max sum of all subtractions = 32
unchecked {
// binary search for the \0 terminator
if (_str & type(uint128).max == 0) {
result -= 16;
_str >>= 128;
}
if (_str & type(uint64).max == 0) {
result -= 8;
_str >>= 64;
}
if (_str & type(uint32).max == 0) {
result -= 4;
_str >>= 32;
}
if (_str & type(uint16).max == 0) {
result -= 2;
_str >>= 16;
}
if (_str & type(uint8).max == 0) {
result -= 1;
_str >>= 8;
}
// Another check is needed in case the string is not \0 terminated
if (_str & type(uint8).max == 0) {
result -= 1;
}
}
return result;
}
bytes16 file, | ||
bytes4 worldFunctionSelector, | ||
bytes4 systemFunctionSelector | ||
) external returns (bytes4 functionSelector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does access control use the full World method path ({namespace}_{file}_{functionName}
) as part of its access control? or does it just look up the function by name to get the registration info (namespace, file) and does access control based on that?
If the latter, I wonder if we could allow for any kind of function name registered on the world (maybe excluding a _
prefix for internals), not just a namespaced one, mostly to make it a little more user friendly? We're already enforcing uniqueness.
(I realize we could autogen around this but maybe we don't need to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the latter, so we could certainly allow for other formats of function selectors to be registered, like we do in registerRootFunctionSelector
, but I'd prefer to leave those fully flexible function selectors exclusive to the owner of the root namespace to avoid "squatting" on important function selectors. We could allow a custom function prefixed by only the namespace though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However I kind of like that with the verbose function selector it's immediately visible where the selector maps to (namespace, file and function selector within the system), while with custom ones it might be harder to know where to look for the source code
Is there an example of what the difference is between |
The difference is in |
Good point! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go, other than maybe moving errors to a shared interface for now so we can get ABI types from them.
This PR adds two new methods to
IWorld
(registerFunctionSelector
,registerRootFunctionSelector
) and refactors the World contract to move all registration methods to an internalRegistrationSystem
, for which we then register root function selectors in the World.The two new methods in detail:
1.:
This function allows the registration of a world function selector for a system function at the given namespace/file/function. The resulting world function selector is
world.<namespace>_<file>_<systemFunctionName><systemFunctionArguments>
. (Note -systemFunctionParameters
needs to include parentheses).Registering such a function selector only requires ownership of the
namespace
.2.:
This function allows the registration of a custom
worldFunctionSelector
for the given namespace/file/systemFunctionSelector.Registering such a function selector requires ownership of the ROOT_NAMESPACE (automatically granted to the deployer of the World).
Note: both functions allow the registration of an empty system function selector, in which case the call is routed to the system's fallback function.
Moving the internal registration methods into its own ResourceSystem, which is then registered in the World (including root function selectors) has multiple advantages:
RegistrationSystem
is stateless (writes on the state of it's caller) and can therefore be deployed once and then just linked to the World during the deploy process (this is not set up yet, currently we deploy a newRegistrationSystem
every time)callFrom
entry point (not implemented yet, see Proposal: World framework v2 #393 for details)It slightly increases the gas cost of all calls to registration methods, but this is fine because they are only called once during deployment, so the runtime cost stays the same.