Namespace storage #191
Namespace storage #191
Conversation
minghinmatthewlam
requested review from
iansuvak,
feuGeneA,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| } | ||
|
|
||
| // solhint-disable-next-line func-name-mixedcase | ||
| function __ERC20TokenRemote_init( |
There was a problem hiding this comment.
Also just for my own understanding: is the only benefit of creating the __ERC20TokenRemote_init internal function and calling it from initialize to support contracts that may inherit from this contract in the future?
There was a problem hiding this comment.
yup mostly it, and also to keep a convention for upgradeable contracts
| struct TokenRemoteStorage { | ||
| /// @notice The blockchain ID of the chain this contract is deployed on. | ||
| bytes32 _blockchainID; | ||
| /// @notice The blockchain ID of the TokenHome instance this contract receives tokens from. |
There was a problem hiding this comment.
Also spacing nit here, what do think of including the empty newline between these state variables like we had before? Makes it a bit easier to read IMO
There was a problem hiding this comment.
it's getting autoformatted being the types are in a struct, going to leave it for now.
| // solhint-disable no-console | ||
| import {console2} from "forge-std/console2.sol"; | ||
|
|
||
| contract ComputeSlotTest is Test { |
There was a problem hiding this comment.
Thoughts on making the storage slot variables public for the sake of both making them easy to unit test?
| @@ -205,6 +227,41 @@ abstract contract TokenRemote is ITokenRemote, TeleporterOwnerUpgradeable, SendR | |||
| // Right-shift by 5 bits to divide by 32. | |||
| return (payloadSize + 31) >> 5; | |||
| } | |||
|
|
|||
| function getIsCollateralized() public view returns (bool) { | |||
There was a problem hiding this comment.
Thoughts on remove the "get" prefix on some/all of these public getters? isCollateralized() in particular seems fully descriptive already. Removing "get" may make them feel a bit closer to standard public state variable getters, but I don't have strong feelings either way.
There was a problem hiding this comment.
Super excited to have this place.
I had a few questions and minor nits. Only high level question I have is if using namespace storage has any significant impact on the gas costs of the function calls? I wouldn't expect so, but I'm not 100% sure if those lookups/writes are handled differently under the hood.
Update: Thinking through it a bit more, it seems like state lookups will have one additional read (first read the STORAGE_SLOT variable value, then read the intended variable, rather than just reading the intended variable directly). Writes now require an additional read first also (need to lookup STORAGE_SLOT) prior to knowing where to write the value. These additional reads should hopefully be somewhat limited by the fact that the STORAGE_SLOT value can be reused for future reads/writes once it has been read into memory once in a function call.
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
minghinmatthewlam
merged commit 80cb00e
into
upgradeable-poc-merge
Why this should be merged
Fixes #185
How this works
Adds namespace storage to all contracts according to ERC7201. Namespace storage helps prevent collision between storage of different logic contracts when used in the context of upgradeable contracts and proxies.
Changes:
@custom:storage-locationaccording to erc7201$structHow this was tested
CI
TODO: should ideally have unit tests to make sure that the storage location are computed correctly.
How is this documented
TBD