synthetix-v3 icon indicating copy to clipboard operation
synthetix-v3 copied to clipboard

Improve storage namespace hash calculation in the deployer

Open eternauta1337 opened this issue 3 years ago • 3 comments

Atm, we calculate hashes "by hand" (e.g. using Remix) and hardcode them to the code, accompanied by a comment which shows how that hash was calculated.

E.g.

    function _electionStore() internal pure returns (ElectionStore storage store) {
        assembly {
            // bytes32(uint(keccak256("io.synthetix.election")) - 1)
            store.slot := 0x4a7bae7406c7467d50a80c6842d6ba8287c729469098e48fc594351749ba4b22
        }
    }

The deployer does have a validation step in which we check if two or more namespaces use the same hash, but we never check if that hash was calculated correctly.

We could either introduce this check (by looking at the comment), or try to find a way to calculate the hash in Solidity in a way that doesn't increase runtime gas costs badly. We do not want to calculate the hash each and every time that someone wants to access the namespace's storage. Maybe we could use Solidity's immutable keyword for this purpose, but not sure if this is trivial since our usage of proxies might complicate things.

eternauta1337 avatar Mar 24 '22 11:03 eternauta1337

@ajsantander after inspecting this issue, I would like to verify:

  • there appears to be duplicate namespace hash checking already. there are even tests. maybe you meant something else/similar?
  • for correct, do we need to verify that the comment prior to the slot matches the actual hash? Should we verify all storage uses the same namespace prefix (ex. io.synthetix.whatever)

also, I know its slightly less gas efficient, but couldn't the slot be computed outside the assembly by calling a utility library function with the bytes32 string like io.synthetix.synth, for example?

dbeal-eth avatar May 05 '22 21:05 dbeal-eth

  • there are even tests.
  1. Yes, I believe we already check for duplicate hashes.
  2. And yes, we want to check that we hash correctly now.
  3. I was highly tempted to do this as well, but it is highly inefficient. These storage namespaces/slots are accessed all over the place, and doing a keccak of a string each and every time would most definitely affect runtime gas usage. Perhaps we could use Solidity's immutable feature here?

eternauta1337 avatar May 07 '22 00:05 eternauta1337

Perhaps we could use Solidity's immutable feature here?

Immutable sounds like a great idea, but it would be a significant departure from how the slot has been calculated in the past. Are we ok with making some breaking changes for this, as long as the validation leads the user to the correct outcome?

could we create a library or something which exports an internal function like generateNamespaceHash(bytes32 name), and then have the validation ensure that this function is used to generate the namespace?

dbeal-eth avatar May 07 '22 21:05 dbeal-eth