safe-smart-account icon indicating copy to clipboard operation
safe-smart-account copied to clipboard

Pad storage addresses passed to `getStorageAt()` to 66 chars

Open cameel opened this issue 2 years ago • 6 comments

Hardhat 2.9.5 released yesterday introduced a requirement for storage addresses passed to getStorageAt() to be padded to full 32 bytes. Currently the Safe has one test that uses an unpadded argument and that test is failing with latest Hardhat.

This PR adds the missing padding. Note that Hardhat has a bug that makes it still reject the properly padded address (https://github.com/NomicFoundation/hardhat/issues/2709) but once that's fixed, this change will be enough to solve the issue.

Failing test

hardhat test test/libraries/Migration.spec.ts
  Migration
    constructor
      ✓ can not use 0 Address
    migrate
      ✓ can only be called from Safe itself

      1) can migrate


  2 passing (3s)
  1 failing

  1) Migration
       migrate
         can migrate:
     InvalidArgumentsError: Errors encountered in param 1: Storage slot argument must have a length of 66 ("0x" + 32 bytes), but '0x6' has a length of 3
      at validateParams (node_modules/hardhat/src/internal/core/jsonrpc/types/input/validation.ts:64:13)
      at EthModule._getStorageAtParams (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:695:26)
      at EthModule.processRequest (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:185:49)
      at HardhatNetworkProvider._send (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:195:31)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at runNextTicks (node:internal/process/task_queues:65:3)
      at listOnTimeout (node:internal/timers:528:9)
      at processTimers (node:internal/timers:502:7)
      at HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)

cameel avatar May 13 '22 16:05 cameel

would it be possible to rebase your PR?

We added a CLA and adjusted the workflow to allow external contributions

rmeissner avatar May 16 '22 17:05 rmeissner

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions[bot] avatar May 16 '22 18:05 github-actions[bot]

Done.

cameel avatar May 16 '22 18:05 cameel

Could you agree to the CLA by commenting I have read the CLA Document and I hereby sign the CLA ;) then I can merge the PR :). Thank you very much :raised_hands:

rmeissner avatar May 17 '22 07:05 rmeissner

Pull Request Test Coverage Report for Build 2333914764

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.6%) to 98.891%

Totals Coverage Status
Change from base Build 2333530112: 1.6%
Covered Lines: 301
Relevant Lines: 301

💛 - Coveralls

coveralls avatar May 17 '22 07:05 coveralls

I have read the CLA Document and I hereby sign the CLA

cameel avatar May 18 '22 18:05 cameel