Move mappings key to constants #261
Conversation
# Conflicts: # contracts/upgradeable_contracts/BasicForeignBridge.sol # contracts/upgradeable_contracts/ERC677Bridge.sol
| // Before store the contract we need to make sure that it is the block reward contract in actual fact, | ||
| // call a specific method from the contract that should return a specific value | ||
| bool isBlockRewardContract = false; | ||
| if (_blockReward.call(abi.encodeWithSignature("blockRewardContractId()"))) { |
There was a problem hiding this comment.
The compiler could prepare the method descriptor on the compilation time and store just 4 bytes in the code now but I am not sure. Could you investigate if it is so or it is worth (from the gas consumption point of view) to store abi.encodeWithSignature("blockRewardContractId()") in a constant as well?
There was a problem hiding this comment.
From docs about Constant State Variables:
The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective constant expression (which might be computed to a single value by the optimizer).
I tried 2 options:
Test 1
Defined:
bytes internal constant BLOCK_REWARD_CONTRACT_ID = abi.encodeWithSignature("blockRewardContractId()");And then using it as:
if (_blockReward.call(BLOCK_REWARD_CONTRACT_ID)) {Gas consumption results:
| Contract | Before | After | Diff |
|---|---|---|---|
| HomeBridgeErcToNative | 6110869 | 6110869 | 0 |
| FeeManagerErcToErcPOSDAO | 894232 | 894168 | -64 |
| HomeBridgeErcToNative.setBlockRewardContract | 34362 | 34362 | 0 |
Test 2
Defined the selector hash directly in the constant
bytes4 internal constant BLOCK_REWARD_CONTRACT_ID = 0x2ee57f8d; // blockRewardContractId() selectorGas consumption results:
| Contract | Before | After | Diff |
|---|---|---|---|
| HomeBridgeErcToNative | 6110869 | 6082606 | -28263 |
| FeeManagerErcToErcPOSDAO | 894232 | 860151 | -34081 |
| HomeBridgeErcToNative.setBlockRewardContract | 34362 | 34160 | -202 |
From test 1, it seems that it is not worth to extract the signature to a constant from gas perspective. However, it looks like if we define the constant as in test 2 we can save some considerable gas. We can apply this to similar other cases on the other contracts too. If we do this, I think we should have a comment on each constant to know the selector it is referring to
There was a problem hiding this comment.
Nice research!
Can the line
bytes internal constant BLOCK_REWARD_CONTRACT_ID = abi.encodeWithSignature("blockRewardContractId()");
consume more gas since there is no exact size of data?
I agree with putting just hexadecimal value of the method selector to optimize gas usage.
| // Before store the contract we need to make sure that it is the block reward contract in actual fact, | ||
| // call a specific method from the contract that should return a specific value | ||
| bool isBlockRewardContract = false; | ||
| if (_blockReward.call(abi.encodeWithSignature("blockRewardContractId()"))) { |
There was a problem hiding this comment.
Nice research!
Can the line
bytes internal constant BLOCK_REWARD_CONTRACT_ID = abi.encodeWithSignature("blockRewardContractId()");
consume more gas since there is no exact size of data?
I agree with putting just hexadecimal value of the method selector to optimize gas usage.
…geToken" This reverts commit 16b8105
|
Added the selector hexadecimal values as constants. Here are the main gas consumption changes
|
Moved mappings key to constant. This helps prevent issues caused by typos or erroneous copy-pastes, this also improves readability. Also some repeated code related to the constant was extracted to base contracts. I think the easiest way to review this PR is by reviewing commit per commit.