fix(world): inline debug constants [L-11] #1976

ClaudeZsb · 2023-11-30T10:29:45Z

Description

The original type of ROOT_NAMESPACE_STRING defined in WorldResourceId.sol is

bytes16 constant ROOT_NAMESPACE_STRING = bytes16("ROOT_NAMESPACE");

Obviously its length is supposed to be 14. And then problem is coming when we use it in a conditional operator ? : and put it in the true condition. See code here:

bytes14 resourceNamespace = getNamespace(resourceId);
...
return string(
  abi.encodePacked(
    ...,
    resourceNamespace == ROOT_NAMESPACE ? ROOT_NAMESPACE_STRING : resourceNamespace,
    ...
  )
)

Although resourceNamespace is defined as bytes14, the type of the value packed will be converted to bytes16 just because the type of value at the true condition is bytes16. As a result, ResourceId.toString() will return a string of length 36=2+1+16+1+16 instead of 34=2+1+14+1+16.

Reproduction

Try the following test code

console.logBytes(abi.encodePacked(bytes16("true")));
console.logBytes(abi.encodePacked(bytes14("false")));
console.logBytes(abi.encodePacked(1 == 2 ? bytes16("true") : bytes14("false")));

And you'll get

Logs:
  0x74727565000000000000000000000000
  0x66616c7365000000000000000000
  0x66616c73650000000000000000000000

where bytes14("false") is packed as a bytes16.

changeset-bot · 2023-11-30T10:29:49Z

🦋 Changeset detected

Latest commit: ca84b88

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages

Name	Type
@latticexyz/world	Patch
@latticexyz/cli	Patch
@latticexyz/dev-tools	Patch
@latticexyz/store-sync	Patch
@latticexyz/world-modules	Patch
@latticexyz/store-indexer	Patch
@latticexyz/abi-ts	Patch
@latticexyz/block-logs-stream	Patch
@latticexyz/common	Patch
@latticexyz/config	Patch
create-mud	Patch
@latticexyz/ecs-browser	Patch
@latticexyz/faucet	Patch
@latticexyz/gas-report	Patch
@latticexyz/network	Patch
@latticexyz/noise	Patch
@latticexyz/phaserx	Patch
@latticexyz/protocol-parser	Patch
@latticexyz/react	Patch
@latticexyz/recs	Patch
@latticexyz/schema-type	Patch
@latticexyz/services	Patch
@latticexyz/solecs	Patch
solhint-config-mud	Patch
solhint-plugin-mud	Patch
@latticexyz/std-client	Patch
@latticexyz/std-contracts	Patch
@latticexyz/store-cache	Patch
@latticexyz/store	Patch
@latticexyz/utils	Patch

Not sure what this means? Click here to learn what changesets are.

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

holic · 2023-11-30T11:35:58Z

packages/world/src/WorldResourceId.sol

@@ -10,7 +10,7 @@ import { RESOURCE_NAMESPACE, MASK_RESOURCE_NAMESPACE } from "./worldResourceType
 uint256 constant NAMESPACE_BITS = 14 * 8;
 uint256 constant NAME_BITS = 16 * 8;

-bytes16 constant ROOT_NAMESPACE_STRING = bytes16("ROOT_NAMESPACE");
+bytes14 constant ROOT_NAMESPACE_STRING = bytes14("ROOT_NAMESPACE");
 bytes16 constant ROOT_NAME_STRING = bytes16("ROOT_NAME");


@alvrs A few notes/questions for us:

Since these strings are just for debugging, they don't need to be type-aligned with the actual namespace/name types, right? They could technically be any length?

I have been using an empty string for root namespace/name in ~all of the client code, and wonder if we should align on that here. Alternatively, I wonder if we should use a less likely string like <root> or something.

Should we inline this into toString (since that's the only place it's used) instead of defining up here, so we don't need to specify the type?

Should we use toTrimmedString for namespace and name inside toString?

Since these strings are just for debugging, they don't need to be type-aligned with the actual namespace/name types, right? They could technically be any length?

Correct (but kinda agree using something else than bytes14 can lead to confusion)

I have been using an empty string for root namespace/name in ~all of the client code, and wonder if we should align on that here. Alternatively, I wonder if we should use a less likely string like or something.

If we do that we don't even need the ternary operator here anymore since the actual namespace/name is already empty. The original reason why we replaced the empty string here with a more explicit one was that ns:: might be less easy to understand than ns:ROOT_NAMESPACE:ROOT_NAME. (No strong opinion on which string we use, also happy to make it ns:<root>:<root>)

Should we inline this into toString (since that's the only place it's used) instead of defining up here, so we don't need to specify the type?

Yeah that makes sense! (still need to specify the type tho because the ternary operator is unhappy if the result is string | bytes14)

Should we use toTrimmedString for namespace and name inside toString?

We could, but if it's logged as string the 0's are already not displayed, and if it's logged as raw bytes it might be a tiny little bit easier to figure out what the log means if it always has the same length (34 bytes). On the other hand advantage of using toTrimmedString would mean it's possible to compare the result with an inline string, which also might be useful.

How about the new changes? Use the inline and shorter string <root> for root namespace and name.

I like it!

Given that this method is meant to be for debug/human-readability purposes and comparison/parsing should be done against the resource ID itself, I am keen for this to be more string like and use toTrimmedString around the whole thing. But would be curious to see gas differences before making the final call.

With test code:

function testToString() public { ResourceId resourceId = WorldResourceIdLib.encode({ typeId: RESOURCE_SYSTEM, namespace: "namespace", name: "name" }); startGasReport("convert resource ID to string"); string memory resourceIdString = resourceId.toString(); endGasReport(); // assertEq(resourceIdString, "sy:namespace:name"); }

It turns out that:

GAS REPORT: 460 convert resource ID to string

After using toTrimmedString inside of toString in the following form:

string( abi.encodePacked( resourceType, ":", resourceNamespace == bytes14("") ? "<root>" : WorldResourceIdLib.toTrimmedString(resourceNamespace), ":", resourceName == bytes16("") ? "<root>" : WorldResourceIdLib.toTrimmedString(resourceName) ) );

We got:

GAS REPORT: 3287 convert resource ID to string

toString is actually used in error message, so we might not care much about its gas consumption?

alvrs · 2024-01-02T12:38:39Z

packages/world/src/WorldResourceId.sol

+          resourceNamespace == bytes14("") ? "<root>" : WorldResourceIdLib.toTrimmedString(resourceNamespace),
          ":",
-          resourceName == ROOT_NAME ? ROOT_NAME_STRING : resourceName
+          resourceName == bytes16("") ? "<root>" : WorldResourceIdLib.toTrimmedString(resourceName)


small nit: let's use the ROOT_NAMESPACE and ROOT_NAME constants (imported from constants.sol) here instead of the inline bytes14

alvrs

Good to be merged and replace #2030. @yonadaaa can you take this over and finish it up (using the ROOT_NAMESPACE and ROOT_NAME constants and adding a short changeset)

holic · 2024-01-12T16:57:11Z

Thanks @ClaudeZsb! Sorry for the delay in getting back to this: we all took a long winter break :)

ClaudeZsb requested review from alvrs and holic as code owners November 30, 2023 10:29

holic reviewed Nov 30, 2023

View reviewed changes

ClaudeZsb requested a review from holic December 5, 2023 02:45

holic mentioned this pull request Dec 6, 2023

chore(world): change ROOT_NAMESPACE_STRING to bytes14 [L-11] #2030

Closed

ClaudeZsb added 4 commits December 11, 2023 10:16

correct the length of ROOT_NAMESPACE_STRING

2bb3aee

incline and short string for root ns&name

a912872

use '<root>' instead of 'root'

4a0100e

use toTrimmedString inside of ResourceId.toString

9b4f570

ClaudeZsb force-pushed the fix-resourceId-string branch from fbc0ac1 to 9b4f570 Compare December 11, 2023 02:16

alvrs reviewed Jan 2, 2024

View reviewed changes

holic changed the title ~~fix(world): fix the length of ROOT_NAMESPACE_STRING~~ fix(world): fix the length of ROOT_NAMESPACE_STRING [L-11] Jan 2, 2024

yonadaa added 3 commits January 7, 2024 23:00

refactor: use root namespace and name constants

7d631d9

chore: changeset

e799d6e

Merge remote-tracking branch 'origin/main' into fix-resourceId-string

f3ea63a

yonadaa requested a review from alvrs January 7, 2024 23:48

holic added 2 commits January 12, 2024 08:35

Update gold-wombats-add.md

2c9b56a

Merge remote-tracking branch 'origin/main' into fix-resourceId-string

ca84b88

holic changed the title ~~fix(world): fix the length of ROOT_NAMESPACE_STRING [L-11]~~ fix(world): inline debug constants [L-11] Jan 12, 2024

holic merged commit d00c4a9 into latticexyz:main Jan 12, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(world): inline debug constants [L-11] #1976

fix(world): inline debug constants [L-11] #1976

ClaudeZsb commented Nov 30, 2023

changeset-bot bot commented Nov 30, 2023 •

edited

Loading

holic Nov 30, 2023 •

edited

Loading

alvrs Nov 30, 2023

ClaudeZsb Dec 4, 2023 •

edited

Loading

holic Dec 6, 2023

ClaudeZsb Dec 8, 2023 •

edited

Loading

ClaudeZsb Dec 11, 2023

alvrs Jan 2, 2024

alvrs left a comment

holic commented Jan 12, 2024

fix(world): inline debug constants [L-11] #1976

fix(world): inline debug constants [L-11] #1976

Conversation

ClaudeZsb commented Nov 30, 2023

Description

Reproduction

changeset-bot bot commented Nov 30, 2023 • edited Loading

🦋 Changeset detected

holic Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

alvrs Nov 30, 2023

Choose a reason for hiding this comment

ClaudeZsb Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

holic Dec 6, 2023

Choose a reason for hiding this comment

ClaudeZsb Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

ClaudeZsb Dec 11, 2023

Choose a reason for hiding this comment

alvrs Jan 2, 2024

Choose a reason for hiding this comment

alvrs left a comment

Choose a reason for hiding this comment

holic commented Jan 12, 2024

changeset-bot bot commented Nov 30, 2023 •

edited

Loading

holic Nov 30, 2023 •

edited

Loading

ClaudeZsb Dec 4, 2023 •

edited

Loading

ClaudeZsb Dec 8, 2023 •

edited

Loading