fix: slot-overwrite bug #133

wei3erHase · 2022-06-30T12:48:36Z

Fixes #118

wei3erHase · 2022-07-01T06:59:24Z

package.json

+    "mytest:nocompile": "cross-env mocha 'test/unit/mock/mytest.spec.ts'",
    "test": "yarn compile && yarn test:nocompile",
+    "mytest": "yarn compile && yarn mytest:nocompile",


please remove extra scripts from package.json

wei3erHase · 2022-07-01T06:59:32Z

package.json

@@ -83,7 +84,6 @@
    "@types/node": "15.12.2",
    "@types/semver": "^7.3.8",
    "chai": "4.3.4",
-    "chai-as-promised": "7.1.1",


wei3erHase · 2022-07-01T07:00:08Z

src/factories/ethers-interface.ts

-      if (isMaybeJsonObject(spec)) {
-        return await ethersInterfaceFromAbi(spec);
-      } else {
-        return await ethersInterfaceFromContractName(spec);
-      }
-    } catch (err) {
-      throw err;
-    }
+      return new ethers.utils.Interface(spec);
+    } catch {}
+
+    try {
+      return (await (hre as any).ethers.getContractFactory(spec)).interface;
+    } catch {}
+
+    try {
+      return (await (hre as any).ethers.getContractAt(spec, ethers.constants.AddressZero)).interface;
+    } catch {}
+
+    throw new Error(`unable to generate smock spec from string`);


i believe this should not be here, please make sure to pull dev up

wei3erHase · 2022-07-01T07:00:26Z

src/logic/editable-storage-logic.ts

+    // for (const slot of slots) {
+    //   await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val));
+    // }


wei3erHase · 2022-07-01T07:01:42Z

src/logic/editable-storage-logic.ts

-      await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(slot.val));
+      let prevStorageValue = await this.vmManager.getContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key));
+      let stringValue = remove0x(toHexString(prevStorageValue));
+      if (stringValue && (slot.type === 'address' || slot.type === 'bool' || slot.type.startsWith('contract'))) {


hmmm interesting code here, is there any way of generalizing this?
i don't think it will cover all use cases, let's say you have an address, bool, bool pack, would it work?
did you perform tests on multiple scenarios?

only address and bool has same slot value
so if they have same slot, previous storage value will be overwritten
i have tested in all case
so i have done this action

wei3erHase · 2022-07-01T07:01:50Z

src/logic/editable-storage-logic.ts

+        // console.log('_______________________________', stringValue, slot.val);
+        let lastResult = slot.val.slice(0, slot.val.length - stringValue.length).concat(stringValue);
+        await this.vmManager.putContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key), fromHexString(lastResult));
+        // console.log('>>>>>>>>>>>>>>>>>>>>>>>', slot.key, lastResult);


remove comments

wei3erHase · 2022-07-01T07:02:01Z

src/utils/storage.ts

+  // let storageLayout = (output as any).storageLayout;
+  // console.log('___ storage ___', (output as any).storageLayout);
+  // for (let i = 0; i < storageLayout.storage.length; i++) {
+  //   storageLayout.storage[i].slot = i.toString();
+  // }
+
+  // return storageLayout;


wei3erHase · 2022-07-01T07:02:10Z

src/utils/storage.ts

@@ -82,6 +90,7 @@ export function computeStorageSlots(storageLayout: SolidityStorageLayout, variab
  let slots: StorageSlotPair[] = [];
  for (const [variableName, variableValue] of Object.entries(variables)) {
    // Find the entry in the storage layout that corresponds to this variable name.
+    // console.log('___ variableName, variableValue ___', variableName, variableValue);


wei3erHase · 2022-07-01T07:02:18Z

src/utils/storage.ts

+    // console.log('___ storageObj ___', storageObj);
+    // console.log('___ slots before ___', slots);


rrrrrrrrrrrrmmmmmmmmm

wei3erHase · 2022-07-01T07:03:53Z

test/contracts/mock/Bug.sol

+pragma solidity ^0.8.4;
+
+contract Bug {
+  address public myAddress;
+  bool public myBool;
+}


please generalize to different cases

wei3erHase

ey wang! let me generalize some comments:

i see a lot of extra comments that shouldn't be there, please rm
also, there are changes in dev that you seem to be rolling back, please correct
i like the solution, but i'm not convinced it could address all possible scenarios, please convince us

thanks for contributing to smock! 🚀
i see lot of potential here

wei3erHase

please, go into Files changed on the Pull Request and make sure that you commit to all the changes that appear there, this doesn't seem to be mergeable without removing other features.

wei3erHase · 2022-07-01T16:21:31Z

package.json

@@ -74,7 +74,6 @@
    "@typechain/ethers-v5": "7.0.1",
    "@typechain/hardhat": "2.0.2",
    "@types/chai": "4.2.18",
-    "@types/chai-as-promised": "7.1.5",


why are we removing this import?

wei3erHase · 2022-07-01T16:22:22Z

test/unit/mock/mytest.spec.ts

+      console.log('____myAddress____', await mock.myAddress());
+      console.log('____myBool____', await mock.myBool());
+      console.log('____newAddress____', await mock.newAddress());
+      console.log('____newAddress2____', await mock.newAddress2());
+      console.log('____newBool____', await mock.newBool());
+      console.log('____newBool2____', await mock.newBool2());


please, remove all console.logs...

wei3erHase · 2022-07-01T16:30:12Z

test/unit/mock/mytest.spec2.ts

@@ -0,0 +1,25 @@
+import { MockContract, MockContractFactory, smock } from '@src';


why is this file named mytest.spec2.ts??

wei3erHase · 2022-07-01T16:30:56Z

test/unit/mock/mytest.spec2.ts

+  });
+
+  describe('setVariable', () => {
+    it('should be able to fix bug', async () => {


don't reference the bug, this will stay in documentation for the future

wei3erHase · 2022-07-04T08:32:35Z

test/contracts/watchable-function-logic/Storage.sol

- * @title Storage
- * @dev Store & retrieve value in a variable
- */
-contract Storage {


emmm, should we keep this one?

wei3erHase · 2022-07-04T08:34:13Z

test/utils/constants.ts

+export const ADDRESS_EXAMPLE2 = '0x4fd96172C40913c8100a47337f21DBf25C9e2fC6';
+
+export const ADDRESS_EXAMPLE3 = '0x62eA762d9f2672f82d26b94f56849881831B9eaB';


i prefer you added a _ before the numbers

0xGorilla · 2022-07-04T08:38:17Z

test/unit/mock/slot-overwrite.spec.ts

+
+  describe('setVariable', () => {
+    it('should not be able to overwrite slot', async () => {
+      await mock.setVariable('myAddress', ADDRESS_EXAMPLE);


No need to use any ADDRESS_EXAMPLEX, let just use generated random addresses, I believe we can achieve that with makeRandomAddress()

What do you think about using setVariables instead of so many setVariable

0xGorilla · 2022-07-04T08:45:01Z

test/unit/fake/initialization.spec.ts

-
-  it('should throw error for invalid json string abi', async () => {
-    await expect(smock.fake(`{invalid}`)).to.be.rejectedWith(
-      Error,
-      `unable to generate smock spec from abi string.\nUnexpected token i in JSON at position 1`
-    );
-  });
-
-  it('should throw error for non existent contract', async () => {
-    let regex: RegExp = /unable to generate smock spec from contract name*/;
-    await expect(smock.fake('NonExistentContract')).to.be.rejectedWith(Error, regex);
-  });
-
-  it('should throw error if contract name is ambiguous', async () => {
-    let regex: RegExp = /unable to generate smock spec from contract name*/;
-    await expect(smock.fake('Storage')).to.be.rejectedWith(Error, regex);
-  });


why deleting this??

0xGorilla · 2022-07-04T08:45:49Z

test/contracts/watchable-function-logic/StorageDuplicate.sol

@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: MIT


This was being used for the test: should throw error if contract name is ambiguous. Why deleting it?

0xGorilla · 2022-07-04T08:46:16Z

test/contracts/watchable-function-logic/Storage.sol

@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: MIT


I suppose you are now using SlotOverwrite instead of this file, right?

0xGorilla · 2022-07-04T08:52:18Z

test/unit/mock/slot-overwrite.spec.ts

+  });
+
+  describe('setVariable', () => {
+    it('should not be able to overwrite slot', async () => {


let's please try more types, bytes, utint, etc... Maybe add some more tests

0xGorilla · 2022-07-04T08:53:32Z

src/logic/editable-storage-logic.ts

+      let prevStorageValue = await this.vmManager.getContractStorage(toFancyAddress(this.contractAddress), fromHexString(slot.key));
+      let stringValue = remove0x(toHexString(prevStorageValue));
+      if (stringValue && (slot.type === 'address' || slot.type === 'bool' || slot.type.startsWith('contract'))) {
+        let lastResult = slot.val.slice(0, slot.val.length - stringValue.length).concat(stringValue);


how come this works if you are always slicing starting in index 0? What if the variable packed slot starts from the middle?

wei3erHase · 2022-07-06T01:35:37Z

Checked until here.
Needs to test more data types.

wei3erHase · 2022-07-06T07:34:54Z

provided test does not replicate bug behaviour

wei3erHase · 2022-07-06T09:32:21Z

Closing PR due to misunderstanding of task scope.

fix: slot-overwrite bug

e6a2101

0xGorilla marked this pull request as draft June 30, 2022 12:56

wei3erHase commented Jul 1, 2022

View reviewed changes

fix: slot-overwrite bug

e53b89b

wei3erHase commented Jul 1, 2022

View reviewed changes

fix: slot-overwrite bug

2b47051

wei3erHase commented Jul 4, 2022

View reviewed changes

0xGorilla reviewed Jul 4, 2022

View reviewed changes

fix: slot-overwrite bug

7f7fdff

fix: slot-overwrite bug

ddc39d7

wei3erHase closed this Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: slot-overwrite bug #133

fix: slot-overwrite bug #133

wei3erHase commented Jun 30, 2022

wei3erHase Jul 1, 2022

wang-jundong Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wang-jundong Jul 1, 2022 •

edited

Loading

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase left a comment

wei3erHase left a comment

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 1, 2022

wei3erHase Jul 4, 2022

wei3erHase Jul 4, 2022

0xGorilla Jul 4, 2022

0xGorilla Jul 4, 2022

0xGorilla Jul 4, 2022

0xGorilla Jul 4, 2022

0xGorilla Jul 4, 2022 •

edited

Loading

0xGorilla Jul 4, 2022 •

edited

Loading

0xGorilla Jul 4, 2022

wei3erHase commented Jul 6, 2022

wei3erHase commented Jul 6, 2022

wei3erHase commented Jul 6, 2022

		// console.log('___ storageObj ___', storageObj);
		// console.log('___ slots before ___', slots);

		@@ -0,0 +1,25 @@
		import { MockContract, MockContractFactory, smock } from '@src';

		export const ADDRESS_EXAMPLE2 = '0x4fd96172C40913c8100a47337f21DBf25C9e2fC6';

		export const ADDRESS_EXAMPLE3 = '0x62eA762d9f2672f82d26b94f56849881831B9eaB';

fix: slot-overwrite bug #133

fix: slot-overwrite bug #133

Conversation

wei3erHase commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wang-jundong Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase left a comment

Choose a reason for hiding this comment

wei3erHase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGorilla Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

0xGorilla Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase commented Jul 6, 2022

wei3erHase commented Jul 6, 2022

wei3erHase commented Jul 6, 2022

wang-jundong Jul 1, 2022 •

edited

Loading

0xGorilla Jul 4, 2022 •

edited

Loading

0xGorilla Jul 4, 2022 •

edited

Loading