refactor: hardcode key/value schema in table libraries #2328
Conversation
🦋 Changeset detectedLatest commit: 39fa542 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
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 |
|
seems to help with bytecode? --- a/packages/world/sizes.txt
+++ b/packages/world/sizes.txt
@@ -25,7 +25,7 @@
| HookInstance | 0.045 | 24.531 |
| HookLib | 0.045 | 24.531 |
| Hooks | 0.045 | 24.531 |
-| InitModule | 19.677 | 4.899 |
+| InitModule | 18.258 | 6.318 |
| InitModuleAddress | 0.045 | 24.531 |
| InstalledModules | 0.045 | 24.531 |
| LayoutOffsets | 0.045 | 24.531 | |
| _keySchema[3] = SchemaType.INT120; | ||
|
|
||
| return SchemaLib.encode(_keySchema); | ||
| return Schema.wrap(0x0034040003601f2e000000000000000000000000000000000000000000000000); |
There was a problem hiding this comment.
Neat! Is there any gas impact to declaring these as constants, ie. Schema constant KEY_SCHEMA = ...?
Wondering if we can go even further and remove the functions (getKeySchema), leaving only the constants. Seems more fitting if all the functions do is return a hardcoded value.
There was a problem hiding this comment.
yep, that'd be the next step (see OP) but wanted to wait for the other PR to land first
holic
changed the title
|
|
||
| return SchemaLib.encode(_valueSchema); | ||
| } | ||
| // Hex-encoded key schema of () |
There was a problem hiding this comment.
() is kinda cursed but what can ya do
There was a problem hiding this comment.
I could add a conditional for this but seems messy
| @@ -29,28 +26,10 @@ library ERC20Metadata { | |||
| FieldLayout constant _fieldLayout = | |||
There was a problem hiding this comment.
Would be interesting to have a similar // Hex-encoded .... comment explaining how the field layout is generated!
There was a problem hiding this comment.
I considered that but didn't have an obvious way to generate a human-readable comment for it so I left it (it never had one)
I guess major if you're leaning on internals but that doesn't seem like a very supported path? I went for |
| // Hex-encoded key schema of (${keyTuple.map((field) => field.internalTypeId).join(", ")}) | ||
| Schema constant _keySchema = Schema.wrap(${keySchemaToHex( | ||
| Object.fromEntries(keyTuple.map((field) => [field.name, field.internalTypeId])) as KeySchema | ||
| )}); | ||
| // Hex-encoded value schema of (${fields.map((field) => field.internalTypeId).join(", ")}) | ||
| Schema constant _valueSchema = Schema.wrap(${valueSchemaToHex( | ||
| Object.fromEntries(fields.map((field) => [field.name, field.internalTypeId])) as ValueSchema | ||
| )}); |
There was a problem hiding this comment.
I generally agree with this change but am a little wary of this being a change to an audited code path. Are there any tests we can add to really make sure the typescript encoding matches the expected encoding? maybe a couple forge test cases where we encode with the new method and confirm it matches what would be produced by SchemaLib.encode?
| @@ -93,7 +93,7 @@ | |||
| "file": "test/World.t.sol", | |||
| "test": "testDeleteRecord", | |||
| "name": "Delete record", | |||
| "gasUsed": 8957 | |||
| "gasUsed": 8958 | |||
similar to #2166
looks like decent gas savings but only on register/install methods and some bytecode savings when using table libs