synthetix-v3
synthetix-v3 copied to clipboard
Improve storage namespace hash calculation in the deployer
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.
@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?
- there are even tests.
- Yes, I believe we already check for duplicate hashes.
- And yes, we want to check that we hash correctly now.
- 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
immutablefeature here?
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?