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.

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

Proposal: move namespace/name out of the Store protocol #1148

Closed
holic opened this issue Jul 12, 2023 · 7 comments
Closed

Proposal: move namespace/name out of the Store protocol #1148

holic opened this issue Jul 12, 2023 · 7 comments
Assignees

Comments

@holic
Copy link
Member

holic commented Jul 12, 2023

Store currently attempts to be unopinionated about table IDs where World creates table IDs from a namespace and name, using the namespace in access control.

However, Store's config "leaks" the World's concept of namespace/name when defining tables, because it's more user-friendly to think of tables with human-readable names.

While working on new store sync code, I was finding myself carrying around the namespace/name, because that's what the Store config uses and it allows us to carry through strong types for tables. I attempted to revert back to a more "Store-pure" approach of just using table IDs everywhere, but then I had no way to carry strong types through (and converting namespace/name to table ID in pure TS sounds painful/slow).

I want to explore using namespace/name everywhere as a first-class concept, even if Store doesn't really make use of them for things like access control.

-event StoreSetRecord(bytes32 table, bytes32[] key, bytes data);
+event StoreSetRecord(bytes16 namespace, bytes16 tableName, bytes32[] key, bytes data);

-event StoreSetField(bytes32 table, bytes32[] key, uint8 schemaIndex, bytes data);
+event StoreSetField(bytes16 namespace, bytes16 tableName, bytes32[] key, uint8 schemaIndex, bytes data);

-event StoreDeleteRecord(bytes32 table, bytes32[] key);
+event StoreDeleteRecord(bytes16 namespace, bytes16 tableName, bytes32[] key);

Some advantages of this approach:

  • Strong types through the entire stack
  • Namespace can become a first-class citizen in config everywhere (see Add support for multiple namespaces in one config #994)
  • Simplifies a lot of code that currently wraps/unwraps table ID into namespace/name
  • Allows for indexing on namespace, so events could be filtered by namespace
@holic holic added this to the Contracts stable milestone Jul 12, 2023
@holic holic changed the title Proposal: use table namespace/name everywhere Proposal: use namespace/name everywhere Jul 12, 2023
@dk1a
Copy link
Contributor

dk1a commented Jul 12, 2023

TLDR: I think it's fine to embrace namespaces in StoreCore for the benefit of World, config and client sync; and possibly even people who use pure Store, since they might want access control too

As you mention, we already effectively separate tableId into namespace/name in Store's typescript, only StoreCore remains unopinionated and treats tableId as monolithic
With #1114 (or a local-types version) the separation would only be stronger
Trying to backtrack and make Store's typescript side forget about namespace/name would complicate the config (and things that depend on it) for questionable benefit
Separating tableId into namespace/name in StoreCore would create a bunch of extra code, tableId is ubiquitous there
But down the line it would simplify World, which has to do indirection right now - setRecord(tableId, ...) is overridden to call setRecord(namespace, name, ...), so we have 2x more Store methods than we really need
Finally, just because namespaces aren't used directly by StoreCore, doesn't mean they don't make any sense in there. We can avoid being opinionated about access control, but still provide tools for it out of the box

@alvrs
Copy link
Member

alvrs commented Jul 12, 2023

Thought some more about this (and discussed some more with @holic and @ludns), here is a writeup of those thoughts

Current situation

  • Store does not use namespace and name because it is not necessary for the protocol
  • World uses namespace and name as a mechanism for access control.
    • This is implemented by via two tables:
      • NamespaceOwner stores the owner (address) of a namespace (bytes16)
      • ResourceAccess stores whether an address has access to a resource selector (bytes32)
    • AccessControl.hasAccess checks whether a given address has access to either the namespace or the resource selector (namespace+name) via the ResourceAccess table
    • AccessControl.requireOwnerOrSelf checks whether a given address owns a namespace via the NamespaceOwner table or is this

Why open this box

  • The fact that Store doesn’t use namespace and name leads to some complexity when naming tables: in World we use cleartext strings casted to bytes16 for both namespace and name, but Store doesn’t enforce this.
  • We want to make our state sync stack compatible with Store without assuming it to be coupled with World.
    • This lowers the barrier to entry for MUD, since developers can start by using Store as a replacement for Solidity storage in any of their contracts while benefiting from the whole sync stack and while not having to opt in to the more involved World framework.
  • One idea is to make namespace and name a concept of Store as well (instead of the less opinionated bytes32 tableId). This would reduce some of the complexity of converting between the World concept of namespace / name and the Store concept of tableId.
  • BUT: It’s unclear if namespace / name is the best approach for access control long term. Enshrining it in both World and Store makes it really hard to change in the future. It would require a full reaudit of both Store and World, while in the current situation only World would have to be reaudited.

Modular audits

  • What if instead of enshrining it in both World and Store, we enshrine it in neither?
  • If access control is modular in the sense that it is not hardcoded in the World contract but registered dynamically, it can be changed after a contract has been deployed, and also we can get the World code audited without access control (or a very simple version of it), and only need to audit the changes to access control when creating a new access control module.

How might this look like?

  • Instead of an internal hardcoded AccessControl library, we use dynamically registered AccessControl system.
  • For now all access control logic stays the same, we keep namespace and name as a concept of the AccessControl system.
  • BUT: we change all function selectors to use a single bytes32 resourceSelector instead of bytes16 namespace and bytes16 name. The developer experience would be that instead of passing namespace and name as individual strings, you pass them through a simple util function exported by the AccessControl module (ie. ResourceSelector(namespace, name))
  • We keep namespace and name a first class citizen of the MUD config. For now this is the best solution for access control we have, we can use it for strong types, and changing the format of the config is not a change that requires a reaudit of the contracts.
  • Our sync stack does not assume the table id to consist of namespace and name. In queries we can use a similar ResourceSelector(namespace, name) util to get to the raw table id.
  • Namespace and name is mostly abstracted from users anyway - Systems are called via function selectors, systems write to tables via codegenerated table libraries.

Conclusion

  • Audits are expensive.
  • We should optimize for modularity as a way to not have to reaudit everything when changes are necessary.
  • There are many ways to solve access control, namespace+name is one of them, but it’s reasonable to assume that smart developers with come up with different ones. By making our protocol flexible enough to support these users we can avoid forks and the necessity to reaudit the full stack.

@dk1a
Copy link
Contributor

dk1a commented Jul 12, 2023

instead of passing namespace and name as individual strings, you pass them through a simple util function exported by the AccessControl module (ie. ResourceSelector(namespace, name))

seems like the key idea I didn't consider, which completes the unopinionated version. This'd reconcile World and Store, and avoid needless StoreCore complexity

Instead of an internal hardcoded AccessControl library, we use dynamically registered AccessControl system

this'd be my main concern, dynamically registering stuff on-chain can be expensive

Allows for indexing on namespace, so events could be filtered by namespace

we lose this, though an indexed tableId can achieve the same end with a wrapper and some more filtering

@alvrs
Copy link
Member

alvrs commented Jul 13, 2023

One more thought about function selectors: we currently use namespace / name to register non-root function selectors

function registerFunctionSelector(
bytes16 namespace,
bytes16 name,
string memory systemFunctionName,
string memory systemFunctionArguments
) public returns (bytes4 worldFunctionSelector) {
// Require the caller to own the namespace
AccessControl.requireOwnerOrSelf(namespace, name, _msgSender());
// Compute global function selector
string memory namespaceString = ResourceSelector.toTrimmedString(namespace);
string memory nameString = ResourceSelector.toTrimmedString(name);
worldFunctionSelector = bytes4(
keccak256(abi.encodePacked(namespaceString, "_", nameString, "_", systemFunctionName, systemFunctionArguments))
);
// Require the function selector to be globally unique
bytes16 existingNamespace = FunctionSelectors.getNamespace(worldFunctionSelector);
bytes16 existingName = FunctionSelectors.getName(worldFunctionSelector);
if (existingNamespace != 0 || existingName != 0) revert FunctionSelectorExists(worldFunctionSelector);
// Register the function selector
bytes memory systemFunctionSignature = abi.encodePacked(systemFunctionName, systemFunctionArguments);
bytes4 systemFunctionSelector = systemFunctionSignature.length == 0
? bytes4(0) // Save gas by storing 0x0 for empty function signatures (= fallback function)
: bytes4(keccak256(systemFunctionSignature));
FunctionSelectors.set(worldFunctionSelector, namespace, name, systemFunctionSelector);
}

If we want to stop engraining namespace/name deep in the protocol to be able to switch it out, we can't use namespace/name here. A possible solution to this would be:

  • make "function selector prefix" a resource that can be owned like tables and systems (= function selector prefix is a bytes32 resource id)
  • use the access same dynamic access control for registering function selectors as for other resources like systems/tables
  • since the function selector prefix is now independent from the system it points to, the function signature of registerFunctionSelector would change to something like
function registerFunctionSelector(
    bytes32 functionPrefixResourceSelector,
    bytes32 systemResourceSelector,
    string memory systemFunctionSignature
  ) public returns (bytes4 worldFunctionSelector)

@alvrs alvrs self-assigned this Jul 14, 2023
@alvrs alvrs changed the title Proposal: use namespace/name everywhere Proposal: move namespace/name out of the core protocol Jul 14, 2023
@alvrs
Copy link
Member

alvrs commented Jul 14, 2023

One more place where we currently implicitly use namespaces is for calling systems via call or delegatecall (regular system vs root system). I propose unbundling this: instead of implicitly calling via delegatecall if the system is registered in the root namespace, we add a root flag to the SystemRegistry table. We define 0x00 as the root resource selector, and for registering a system with root = true we perform an ownership check on the root resource selector.

@alvrs alvrs moved this from Spec to Implementation in MUD v2 stable release (deprecated) Jul 14, 2023
@alvrs alvrs moved this from Implementation to Pause in MUD v2 stable release (deprecated) Jul 17, 2023
@holic holic changed the title Proposal: move namespace/name out of the core protocol Proposal: move namespace/name out of the Store protocol Aug 10, 2023
@holic
Copy link
Member Author

holic commented Aug 17, 2023

Is this considered done now that #1208 has landed?

@dk1a
Copy link
Contributor

dk1a commented Aug 17, 2023

Is this considered done now that #1208 has landed?

I think so, at least on the solidity side of things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants