From 004fe9ce9ec3012429a498de0a121a32b9639841 Mon Sep 17 00:00:00 2001 From: alvrs <alvarius@lattice.xyz> Date: Sat, 23 Sep 2023 17:28:04 +0100 Subject: [PATCH] actually delete data in deleteRecord --- packages/store/gas-report.json | 28 ++++++++++++------------ packages/store/src/IStoreErrors.sol | 1 + packages/store/src/Storage.sol | 12 ++++++++++ packages/store/src/StoreCore.sol | 16 +++++++++++--- packages/world/gas-report.json | 34 ++++++++++++++--------------- 5 files changed, 57 insertions(+), 34 deletions(-) diff --git a/packages/store/gas-report.json b/packages/store/gas-report.json index c46032c970..616393654d 100644 --- a/packages/store/gas-report.json +++ b/packages/store/gas-report.json @@ -381,7 +381,7 @@ "file": "test/Mixed.t.sol", "test": "testSetGetDeleteExternal", "name": "delete record from Mixed (external)", - "gasUsed": 8424 + "gasUsed": 10987 }, { "file": "test/Mixed.t.sol", @@ -399,7 +399,7 @@ "file": "test/Mixed.t.sol", "test": "testSetGetDeleteInternal", "name": "delete record from Mixed (internal)", - "gasUsed": 7213 + "gasUsed": 9775 }, { "file": "test/PackedCounter.t.sol", @@ -597,25 +597,25 @@ "file": "test/StoreCoreDynamic.t.sol", "test": "testGetFieldSlice", "name": "get field slice (cold, 1 slot)", - "gasUsed": 8315 + "gasUsed": 8022 }, { "file": "test/StoreCoreDynamic.t.sol", "test": "testGetFieldSlice", "name": "get field slice (warm, 1 slot)", - "gasUsed": 2383 + "gasUsed": 2090 }, { "file": "test/StoreCoreDynamic.t.sol", "test": "testGetFieldSlice", "name": "get field slice (semi-cold, 1 slot)", - "gasUsed": 4386 + "gasUsed": 4093 }, { "file": "test/StoreCoreDynamic.t.sol", "test": "testGetFieldSlice", "name": "get field slice (warm, 2 slots)", - "gasUsed": 4612 + "gasUsed": 4319 }, { "file": "test/StoreCoreDynamic.t.sol", @@ -693,13 +693,13 @@ "file": "test/StoreCoreGas.t.sol", "test": "testAccessEmptyData", "name": "access slice of dynamic field of non-existing record", - "gasUsed": 1517 + "gasUsed": 1224 }, { "file": "test/StoreCoreGas.t.sol", "test": "testDeleteData", "name": "delete record (complex data, 3 slots)", - "gasUsed": 7733 + "gasUsed": 10293 }, { "file": "test/StoreCoreGas.t.sol", @@ -747,7 +747,7 @@ "file": "test/StoreCoreGas.t.sol", "test": "testHooks", "name": "delete record on table with subscriber", - "gasUsed": 18733 + "gasUsed": 18747 }, { "file": "test/StoreCoreGas.t.sol", @@ -771,7 +771,7 @@ "file": "test/StoreCoreGas.t.sol", "test": "testHooksDynamicData", "name": "delete (dynamic) record on table with subscriber", - "gasUsed": 19719 + "gasUsed": 22772 }, { "file": "test/StoreCoreGas.t.sol", @@ -1035,13 +1035,13 @@ "file": "test/tables/StoreHooks.t.sol", "test": "testTable", "name": "StoreHooks: delete record (warm)", - "gasUsed": 10147 + "gasUsed": 11840 }, { "file": "test/tables/StoreHooks.t.sol", "test": "testTable", "name": "StoreHooks: set field (warm)", - "gasUsed": 31220 + "gasUsed": 51121 }, { "file": "test/tables/StoreHooks.t.sol", @@ -1059,7 +1059,7 @@ "file": "test/tables/StoreHooksColdLoad.t.sol", "test": "testDelete", "name": "StoreHooks: delete record (cold)", - "gasUsed": 19005 + "gasUsed": 25337 }, { "file": "test/tables/StoreHooksColdLoad.t.sol", @@ -1071,7 +1071,7 @@ "file": "test/tables/StoreHooksColdLoad.t.sol", "test": "testGetItem", "name": "StoreHooks: get 1 element (cold)", - "gasUsed": 6575 + "gasUsed": 6282 }, { "file": "test/tables/StoreHooksColdLoad.t.sol", diff --git a/packages/store/src/IStoreErrors.sol b/packages/store/src/IStoreErrors.sol index 6bb184c188..46b19f181e 100644 --- a/packages/store/src/IStoreErrors.sol +++ b/packages/store/src/IStoreErrors.sol @@ -10,6 +10,7 @@ interface IStoreErrors { error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string resourceIdString); error Store_NotDynamicField(); + error Store_InvalidDynamicDataLength(uint256 expected, uint256 received); error Store_InvalidKeyNamesLength(uint256 expected, uint256 received); error Store_InvalidFieldNamesLength(uint256 expected, uint256 received); error Store_InvalidValueSchemaLength(uint256 expected, uint256 received); diff --git a/packages/store/src/Storage.sol b/packages/store/src/Storage.sol index d8745b3636..e558d23093 100644 --- a/packages/store/src/Storage.sol +++ b/packages/store/src/Storage.sol @@ -99,6 +99,18 @@ library Storage { } } + function zero(uint256 storagePointer, uint256 length) internal { + // Ceil division to round up to the nearest word + uint256 limit = storagePointer + (length + 31) / 32; + while (storagePointer < limit) { + /// @solidity memory-safe-assembly + assembly { + sstore(storagePointer, 0) + storagePointer := add(storagePointer, 1) + } + } + } + function load(uint256 storagePointer) internal view returns (bytes32 word) { assembly { word := sload(storagePointer) diff --git a/packages/store/src/StoreCore.sol b/packages/store/src/StoreCore.sol index 99c2b85927..d799e18cbf 100644 --- a/packages/store/src/StoreCore.sol +++ b/packages/store/src/StoreCore.sol @@ -456,10 +456,20 @@ library StoreCore { uint256 staticDataLocation = StoreCoreInternal._getStaticDataLocation(tableId, keyTuple); Storage.store({ storagePointer: staticDataLocation, offset: 0, data: new bytes(fieldLayout.staticDataLength()) }); - // If there are dynamic fields, delete the dynamic data length - if (fieldLayout.numDynamicFields() > 0) { + // If there are dynamic fields, delete the dynamic data + uint256 numDynamicFields = fieldLayout.numDynamicFields(); + if (numDynamicFields > 0) { uint256 dynamicDataLengthLocation = StoreCoreInternal._getDynamicDataLengthLocation(tableId, keyTuple); - Storage.store({ storagePointer: dynamicDataLengthLocation, data: bytes32(0) }); + PackedCounter dynamicDataLength = PackedCounter.wrap(Storage.load({ storagePointer: dynamicDataLengthLocation })); + + // Delete dynamic data + for (uint256 i; i < numDynamicFields; i++) { + uint256 dynamicDataLocation = StoreCoreInternal._getDynamicDataLocation(tableId, keyTuple, uint8(i)); + Storage.zero({ storagePointer: dynamicDataLocation, length: dynamicDataLength.atIndex(uint8(i)) }); + } + + // Set the dynamic data length to 0 + Storage.zero({ storagePointer: dynamicDataLengthLocation, length: 32 }); } // Call onAfterDeleteRecord hooks diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json index 7df8359bb2..e4babf0aae 100644 --- a/packages/world/gas-report.json +++ b/packages/world/gas-report.json @@ -75,7 +75,7 @@ "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookCompositeGas", "name": "delete a composite record on a table with keysInTableModule installed", - "gasUsed": 160569 + "gasUsed": 159704 }, { "file": "test/KeysInTableModule.t.sol", @@ -93,7 +93,7 @@ "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookGas", "name": "delete a record on a table with keysInTableModule installed", - "gasUsed": 86287 + "gasUsed": 86008 }, { "file": "test/KeysWithValueModule.t.sol", @@ -123,7 +123,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testInstall", "name": "set a record on a table with KeysWithValueModule installed", - "gasUsed": 136094 + "gasUsed": 137469 }, { "file": "test/KeysWithValueModule.t.sol", @@ -141,7 +141,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetAndDeleteRecordHook", "name": "delete a record on a table with KeysWithValueModule installed", - "gasUsed": 36264 + "gasUsed": 37803 }, { "file": "test/KeysWithValueModule.t.sol", @@ -153,43 +153,43 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetField", "name": "set a field on a table with KeysWithValueModule installed", - "gasUsed": 147302 + "gasUsed": 148677 }, { "file": "test/KeysWithValueModule.t.sol", "test": "testSetField", "name": "change a field on a table with KeysWithValueModule installed", - "gasUsed": 112061 + "gasUsed": 113593 }, { "file": "test/query.t.sol", "test": "testCombinedHasHasValueNotQuery", "name": "CombinedHasHasValueNotQuery", - "gasUsed": 103163 + "gasUsed": 100526 }, { "file": "test/query.t.sol", "test": "testCombinedHasHasValueQuery", "name": "CombinedHasHasValueQuery", - "gasUsed": 52885 + "gasUsed": 52006 }, { "file": "test/query.t.sol", "test": "testCombinedHasNotQuery", "name": "CombinedHasNotQuery", - "gasUsed": 128373 + "gasUsed": 123685 }, { "file": "test/query.t.sol", "test": "testCombinedHasQuery", "name": "CombinedHasQuery", - "gasUsed": 81580 + "gasUsed": 78943 }, { "file": "test/query.t.sol", "test": "testCombinedHasValueNotQuery", "name": "CombinedHasValueNotQuery", - "gasUsed": 83214 + "gasUsed": 80577 }, { "file": "test/query.t.sol", @@ -201,19 +201,19 @@ "file": "test/query.t.sol", "test": "testHasQuery", "name": "HasQuery", - "gasUsed": 18122 + "gasUsed": 17536 }, { "file": "test/query.t.sol", "test": "testHasQuery1000Keys", "name": "HasQuery with 1000 keys", - "gasUsed": 5938621 + "gasUsed": 5645621 }, { "file": "test/query.t.sol", "test": "testHasQuery100Keys", "name": "HasQuery with 100 keys", - "gasUsed": 554015 + "gasUsed": 524715 }, { "file": "test/query.t.sol", @@ -225,7 +225,7 @@ "file": "test/query.t.sol", "test": "testNotValueQuery", "name": "NotValueQuery", - "gasUsed": 46481 + "gasUsed": 45602 }, { "file": "test/StandardDelegationsModule.t.sol", @@ -237,7 +237,7 @@ "file": "test/StandardDelegationsModule.t.sol", "test": "testCallFromCallboundDelegation", "name": "call a system via a callbound delegation", - "gasUsed": 36694 + "gasUsed": 36701 }, { "file": "test/StandardDelegationsModule.t.sol", @@ -297,7 +297,7 @@ "file": "test/World.t.sol", "test": "testDeleteRecord", "name": "Delete record", - "gasUsed": 9911 + "gasUsed": 9918 }, { "file": "test/World.t.sol",