actually delete data in deleteRecord

latticexyz · Sep 23, 2023 · 004fe9c · 004fe9c
1 parent e452215
commit 004fe9c
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 34 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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",