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

feat(world): allow registration of function selectors in the World, split out RegisterSystem from World #481

Merged
merged 12 commits into from
Mar 14, 2023
Merged

feat(world): allow registration of function selectors in the World, split out RegisterSystem from World #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
194a5af
feat(world): add support for fallback function registration, add Regi…
alvrs Mar 9, 2023
b185818
feat(store): improve gas performance of setBytes4
alvrs Mar 10, 2023
b580108
chore(world): clean up
alvrs Mar 10, 2023
eeb0343
test(world): add gas-report
alvrs Mar 10, 2023
b3dd86a
feat(store): add Bytes.setLength util
alvrs Mar 10, 2023
19cf507
fix(world): trim selectors to create expected function selector, add …
alvrs Mar 10, 2023
f522663
docs(world): update comment
alvrs Mar 10, 2023
c24ba17
chore: update gas-report
alvrs Mar 10, 2023
a81bf8f
chore: self-review
alvrs Mar 10, 2023
9bb01e6
refactor(world): update argument names
alvrs Mar 10, 2023
31e1d7f
chore: update gas-report
alvrs Mar 10, 2023
4232dce
refactor(world): use IErrors instead of Error library
alvrs Mar 14, 2023
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
4 changes: 2 additions & 2 deletions packages/cli/src/utils/deploy-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { MUDConfig } from "../config/index.js";
import { MUDError } from "./errors.js";
import { getOutDirectory, getScriptDirectory, cast, forge } from "./foundry.js";
import { BigNumber, ContractInterface, ethers } from "ethers";
import { World } from "@latticexyz/world/types/ethers-contracts/World.js";
import { IWorld } from "@latticexyz/world/types/ethers-contracts/IWorld.js";
import { abi as WorldABI, bytecode as WorldBytecode } from "@latticexyz/world/abi/World.json";
import { ArgumentsType } from "vitest";
import chalk from "chalk";
Expand Down Expand Up @@ -64,7 +64,7 @@ export async function deploy(mudConfig: MUDConfig, deployConfig: DeployConfig):
);

// Create World contract instance from deployed address
const WorldContract = new ethers.Contract(await contractPromises.World, WorldABI, signer) as World;
const WorldContract = new ethers.Contract(await contractPromises.World, WorldABI, signer) as IWorld;

// Register namespace
if (namespace) await fastTxExecute(WorldContract, "registerNamespace", [toBytes16(namespace)]);
Expand Down
1 change: 1 addition & 0 deletions packages/store/gas-report.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
(test/Bytes.t.sol) | set bytes1 in bytes32 [Bytes.setBytes1(input, 8, 0xff)]: 7
(test/Bytes.t.sol) | set bytes2 in bytes32 [Bytes.setBytes2(input, 8, 0xffff)]: 7
(test/Bytes.t.sol) | set bytes4 in bytes32 [Bytes.setBytes4(input, 8, 0xffffffff)]: 7
(test/Bytes.t.sol) | set bytes4 in bytes memory [bytes memory result = Bytes.setBytes4(input, 0, overwrite)]: 43
(test/Bytes.t.sol) | slice bytes3 with offset 1 [bytes3 b = Bytes.slice3(a, 1)]: 77
(test/Bytes.t.sol) | slice bytes32 with offset 10 [bytes32 output = Bytes.slice32(input, 10)]: 74
(test/Bytes.t.sol) | create bytes32 from bytes memory with offset 0 [bytes32 output = Bytes.toBytes32(input, 0)]: 22
Expand Down
43 changes: 28 additions & 15 deletions packages/store/src/Bytes.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity >=0.8.0;

import { console } from "forge-std/console.sol";
import { SliceLib } from "./Slice.sol";

library Bytes {
/**
Expand Down Expand Up @@ -30,6 +31,16 @@ library Bytes {
return keccak256(a) == keccak256(b);
}

/**
* In-place set the length of a `bytes` memory value.
*/
function setLength(bytes memory input, uint256 length) internal pure returns (bytes memory) {
assembly {
mstore(input, length)
}
return input;
}

/************************************************************************
*
* SET
Expand All @@ -39,11 +50,7 @@ library Bytes {
/**
* Overwrite a single byte of a `bytes32` value and return the new value.
*/
function setBytes1(
bytes32 input,
uint256 index,
bytes1 overwrite
) internal pure returns (bytes32 output) {
function setBytes1(bytes32 input, uint256 index, bytes1 overwrite) internal pure returns (bytes32 output) {
bytes1 mask = 0xff;
assembly {
mask := shr(mul(8, index), mask) // create a mask by shifting 0xff right by index bytes
Expand All @@ -56,11 +63,7 @@ library Bytes {
/**
* Overwrite two bytes of a `bytes32` value and return the new value.
*/
function setBytes2(
bytes32 input,
uint256 index,
bytes2 overwrite
) internal pure returns (bytes32 output) {
function setBytes2(bytes32 input, uint256 index, bytes2 overwrite) internal pure returns (bytes32 output) {
bytes2 mask = 0xffff;
assembly {
mask := shr(mul(8, index), mask) // create a mask by shifting 0xffff right by index bytes
Expand All @@ -73,11 +76,7 @@ library Bytes {
/**
* Overwrite four bytes of a `bytes32` value and return the new value.
*/
function setBytes4(
bytes32 input,
uint256 index,
bytes4 overwrite
) internal pure returns (bytes32 output) {
function setBytes4(bytes32 input, uint256 index, bytes4 overwrite) internal pure returns (bytes32 output) {
bytes4 mask = 0xffffffff;
assembly {
mask := shr(mul(8, index), mask) // create a mask by shifting 0xffffffff right by index bytes
Expand All @@ -87,6 +86,20 @@ library Bytes {
return output;
}

/**
* In-place overwrite four bytes of a `bytes memory` value.
*/
function setBytes4(bytes memory input, uint256 offset, bytes4 overwrite) internal pure returns (bytes memory) {
bytes4 mask = 0xffffffff;
assembly {
let value := mload(add(add(input, 0x20), offset)) // load 32 bytes from input starting at offset
value := and(value, not(mask)) // zero out the first 4 bytes
value := or(value, overwrite) // set the bytes at the offset
mstore(add(add(input, 0x20), offset), value) // store the new value
}
return input;
}

/************************************************************************
*
* SLICE
Expand Down
18 changes: 18 additions & 0 deletions packages/store/test/Bytes.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,22 @@ contract BytesTest is Test {
bytes32(0x0000006d000a0000000000000000000000000000000000000000000000000000)
);
}

function testSetBytes4Memory() public {
bytes4 first = bytes4(0x1000000a);
bytes32 remainder = bytes32(keccak256("some data"));
bytes4 overwrite = bytes4(0x2000000b);

bytes memory input = abi.encodePacked(first, remainder);

// !gasreport set bytes4 in bytes memory
bytes memory result = Bytes.setBytes4(input, 0, overwrite);

// First 4 bytes should be overwritten
assertEq(bytes4(result), overwrite);
assertEq(keccak256(result), keccak256(abi.encodePacked(overwrite, remainder)));

// Operation happened in-place
assertEq(keccak256(input), keccak256(result));
}
}
11 changes: 11 additions & 0 deletions packages/world/gas-report.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
(test/World.t.sol) | Delete record [world.deleteRecord(namespace, file, singletonKey)]: 16038
(test/World.t.sol) | Push data to the table [world.pushToField(namespace, file, keyTuple, 0, encodedData)]: 96365
(test/World.t.sol) | Register a fallback system [bytes4 funcSelector1 = world.registerFunctionSelector(namespace, file, "", "")]: 80896
(test/World.t.sol) | Register a root fallback system [bytes4 funcSelector2 = world.registerRootFunctionSelector(namespace, file, worldFunc, 0)]: 72123
(test/World.t.sol) | Register a function selector [bytes4 functionSelector = world.registerFunctionSelector(namespace, file, "msgSender", "()")]: 101493
(test/World.t.sol) | Register a new namespace [world.registerNamespace("test")]: 151546
(test/World.t.sol) | Register a root function selector [bytes4 functionSelector = world.registerRootFunctionSelector(namespace, file, worldFunc, sysFunc)]: 96029
(test/World.t.sol) | Register a new table in the namespace [bytes32 tableSelector = world.registerTable(namespace, table, schema, defaultKeySchema)]: 252073
(test/World.t.sol) | Write data to a table field [world.setField(namespace, file, singletonKey, 0, abi.encodePacked(true))]: 44704
(test/World.t.sol) | Set metadata [world.setMetadata(namespace, file, tableName, fieldNames)]: 277121
(test/World.t.sol) | Write data to the table [Bool.set(tableId, world, true)]: 42576
12 changes: 12 additions & 0 deletions packages/world/mud.config.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ const config: StoreUserConfig = {
resourceType: "Resource",
},
},
FunctionSelectors: {
fileSelector: "funcSelectors",
primaryKeys: {
functionSelector: SchemaType.BYTES4,
},
schema: {
namespace: SchemaType.BYTES16,
file: SchemaType.BYTES16,
systemFunctionSelector: SchemaType.BYTES4,
},
dataStruct: false,
},
// Bool: {
// TODO: This table is only used for testing, move it to `test/tables` via the directory config once supported
// primaryKeys: {},
Expand Down
51 changes: 51 additions & 0 deletions packages/world/src/AccessControl.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0;

import { ResourceSelector } from "./ResourceSelector.sol";
import { IErrors } from "./interfaces/IErrors.sol";

import { ResourceAccess } from "./tables/ResourceAccess.sol";
import { NamespaceOwner } from "./tables/NamespaceOwner.sol";

library AccessControl {
using ResourceSelector for bytes32;

/**
* Returns true if the caller has access to the namespace or file, false otherwise.
*/
function hasAccess(bytes16 namespace, bytes16 file, address caller) internal view returns (bool) {
return
ResourceAccess.get(ResourceSelector.from(namespace, 0), caller) || // First check access based on the namespace
ResourceAccess.get(ResourceSelector.from(namespace, file), caller); // If caller has no namespace access, check access on the file
}

/**
* Check for access at the given namespace or file.
* Returns the resourceSelector if the caller has access.
* Reverts with AccessDenied if the caller has no access.
*/
function requireAccess(
bytes16 namespace,
bytes16 file,
address caller
) internal view returns (bytes32 resourceSelector) {
resourceSelector = ResourceSelector.from(namespace, file);

// Check if the given caller has access to the given namespace or file
if (!hasAccess(namespace, file, msg.sender)) {
revert IErrors.AccessDenied(resourceSelector.toString(), caller);
}
}

function requireOwner(
bytes16 namespace,
bytes16 file,
address caller
) internal view returns (bytes32 resourceSelector) {
resourceSelector = ResourceSelector.from(namespace, file);

if (NamespaceOwner.get(namespace) != msg.sender) {
revert IErrors.AccessDenied(resourceSelector.toString(), caller);
}
}
}
10 changes: 0 additions & 10 deletions packages/world/src/ISystemHook.sol
View file
Open in desktop

This file was deleted.

26 changes: 25 additions & 1 deletion packages/world/src/ResourceSelector.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0;
import { ROOT_NAMESPACE, ROOT_FILE } from "./constants.sol";
import { Bytes } from "@latticexyz/store/src/Bytes.sol";

bytes16 constant ROOT_NAMESPACE_STRING = bytes16("ROOT_NAMESPACE");
bytes16 constant ROOT_FILE_STRING = bytes16("ROOT_FILE");
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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!


library ResourceSelector {
/**
Expand Down Expand Up @@ -44,7 +49,26 @@ library ResourceSelector {
* Convert a selector to a string for more readable logs
*/
function toString(bytes32 resourceSelector) internal pure returns (string memory) {
return string(abi.encodePacked(getNamespace(resourceSelector), "/", getFile(resourceSelector)));
bytes16 namespace = getNamespace(resourceSelector);
bytes16 file = getFile(resourceSelector);
return
string(
abi.encodePacked(
namespace == ROOT_NAMESPACE ? ROOT_NAMESPACE_STRING : namespace,
"/",
Copy link
Member

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"?

Copy link
Member Author

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 /

file == ROOT_FILE ? ROOT_FILE_STRING : file
)
);
}

/**
* Convert a selector to a trimmed string (no trailing `null` ASCII characters)
*/
function toTrimmedString(bytes16 selector) internal pure returns (string memory) {
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));
Copy link
Member

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?

Copy link
Member Author

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)

Copy link
Member Author

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)

Copy link
Contributor

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;
}

}

/**
Expand Down
Loading