-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(zkevm-integration): implement draft-impl
#140
base: feature/zkevm-integration
Are you sure you want to change the base?
feat(zkevm-integration): implement draft-impl
#140
Conversation
f0cdb07
to
8f3de79
Compare
9d49a13
to
218c373
Compare
Slither reportTHIS CHECKLIST IS NOT COMPLETE. Use
reentrancy-ethImpact: High
dpos-contract/src/ronin/validator/CoinbaseExecution.sol Lines 219 to 271 in 232d73a
dpos-contract/src/ronin/validator/CoinbaseExecution.sol Lines 112 to 175 in 232d73a
dpos-contract/src/ronin/validator/CoinbaseExecution.sol Lines 112 to 175 in 232d73a
dpos-contract/src/ronin/staking/CandidateStaking.sol Lines 120 to 155 in 232d73a
dpos-contract/src/ronin/validator/CoinbaseExecution.sol Lines 112 to 175 in 232d73a
dpos-contract/src/ronin/validator/CoinbaseExecution.sol Lines 112 to 175 in 232d73a
shadowing-stateImpact: High
uninitialized-stateImpact: High
|
218c373
to
9acc56e
Compare
Storage Layout Change ReportLayout Changes for ProfileHandler--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/profile/ProfileHandler.sol .464099646 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/profile/ProfileHandler.sol .464099646 +0000
@@ -3,4 +3,5 @@
2 _consensus2Id mapping(TConsensus => address)
3 _profileChangeCooldown uint256
4 _vrfKeyHash2Id mapping(bytes32 => address)
- 5 __gap bytes32[46]
+🏴 5 _rollupId2Id mapping(uint32 => address)
+🏴 6 __gap bytes32[45]
Layout Changes for ProfileStorage--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/profile/ProfileStorage.sol .533100945 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/profile/ProfileStorage.sol .533100945 +0000
@@ -3,4 +3,5 @@
2 _consensus2Id mapping(TConsensus => address)
3 _profileChangeCooldown uint256
4 _vrfKeyHash2Id mapping(bytes32 => address)
- 5 __gap bytes32[46]
+🏴 5 _rollupId2Id mapping(uint32 => address)
+🏴 6 __gap bytes32[45]
Layout Changes for SlashingExecution--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/SlashingExecution.sol .100262167 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/SlashingExecution.sol .100262167 +0000
@@ -32,5 +32,6 @@
173 _lockedConsensusList address[]
174 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
175 _lockedFundReleased mapping(address => bool)
- 176 ______gap uint256[44]
-
+🏴 176 _validatorL2MiningReward mapping(address => uint256)
+🏴 177 _delegatorL2MiningReward mapping(address => uint256)
+🏴 178 ______gap uint256[42]
Layout Changes for CoinbaseExecution--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/CoinbaseExecution.sol .577271143 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/CoinbaseExecution.sol .577271143 +0000
@@ -42,5 +42,6 @@
230 _lockedConsensusList address[]
231 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
232 _lockedFundReleased mapping(address => bool)
- 233 ______gap uint256[44]
-
+🏴 233 _validatorL2MiningReward mapping(address => uint256)
+🏴 234 _delegatorL2MiningReward mapping(address => uint256)
+🏴 235 ______gap uint256[42]
Layout Changes for EmergencyExit--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/EmergencyExit.sol .214264312 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/EmergencyExit.sol .214264312 +0000
@@ -38,5 +38,6 @@
226 _lockedConsensusList address[]
227 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
228 _lockedFundReleased mapping(address => bool)
- 229 ______gap uint256[44]
-
+🏴 229 _validatorL2MiningReward mapping(address => uint256)
+🏴 230 _delegatorL2MiningReward mapping(address => uint256)
+🏴 231 ______gap uint256[42]
Layout Changes for CoinbaseExecutionDependant--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/CoinbaseExecutionDependant.sol .282265592 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/CoinbaseExecutionDependant.sol .282265592 +0000
@@ -42,5 +42,6 @@
230 _lockedConsensusList address[]
231 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
232 _lockedFundReleased mapping(address => bool)
- 233 ______gap uint256[44]
-
+🏴 233 _validatorL2MiningReward mapping(address => uint256)
+🏴 234 _delegatorL2MiningReward mapping(address => uint256)
+🏴 235 ______gap uint256[42]
Layout Changes for RoninValidatorSetConstructor--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/RoninValidatorSetConstructor.sol .897277165 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/RoninValidatorSetConstructor.sol .897277165 +0000
@@ -44,5 +44,6 @@
231 _lockedConsensusList address[]
232 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
233 _lockedFundReleased mapping(address => bool)
- 234 ______gap uint256[44]
-
+🏴 234 _validatorL2MiningReward mapping(address => uint256)
+🏴 235 _delegatorL2MiningReward mapping(address => uint256)
+🏴 236 ______gap uint256[42]
Layout Changes for SlashingExecutionDependant--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/SlashingExecutionDependant.sol .315266213 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/SlashingExecutionDependant.sol .315266213 +0000
@@ -32,5 +32,6 @@
173 _lockedConsensusList address[]
174 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
175 _lockedFundReleased mapping(address => bool)
- 176 ______gap uint256[44]
-
+🏴 176 _validatorL2MiningReward mapping(address => uint256)
+🏴 177 _delegatorL2MiningReward mapping(address => uint256)
+🏴 178 ______gap uint256[42]
Layout Changes for CommonStorage--- https://github.com/ronin-chain/dpos-contract/blob/8f3de799bd122622d3f96abb01db226cd4a58b31/src/ronin/validator/storage-fragments/CommonStorage.sol .351248071 +0000
+++ https://github.com/ronin-chain/dpos-contract/blob/03b1613b9f1f9d6e7a224b30800677c2ebf39d1d/src/ronin/validator/storage-fragments/CommonStorage.sol .351248071 +0000
@@ -30,5 +30,6 @@
171 _lockedConsensusList address[]
172 _exitInfo mapping(address => struct ICommonInfo.EmergencyExitInfo)
173 _lockedFundReleased mapping(address => bool)
- 174 ______gap uint256[44]
-
+🏴 174 _validatorL2MiningReward mapping(address => uint256)
+🏴 175 _delegatorL2MiningReward mapping(address => uint256)
+🏴 176 ______gap uint256[42]
|
uint32 rollupId | ||
) external view returns (address) { | ||
(bool found, address id) = _tryGetRollupId2Id(rollupId); | ||
if (!found) revert ErrLookUpIdFromRollupIdFailed(rollupId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why dont we just return address zero when not found? Reverting in external view function doesn't make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have try function for that. This protects other contracts from querying null data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which other contracts? And should we revert when not found in both methods getId2Aggregator
and getId2Sequencer
too?
function changeSequencerAddr(address id, address sequencer) external onlyAdmin { | ||
CandidateProfile storage _profile = _getId2ProfileHelper(id); | ||
|
||
_requireNonZeroAndNonDuplicated(RoleAccess.SEQUENCER, sequencer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the logic, aggregator address must be differential from sequencer address. Is it as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intended.
function execCreateRollup(address id, uint32 rollupId) external onlyContract(ContractType.ZK_ROLLUP_MANAGER) { | ||
CandidateProfile storage _profile = _getId2ProfileHelper(id); | ||
|
||
_requireNotOnRenunciation(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have: bring it to modifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will bring later
|
||
_requireNotOnRenunciation(id); | ||
|
||
if (_registry[rollupId]) revert ErrRollupIdAlreadyRegistered(rollupId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the rollup ID does not exist but accidentally matches any registered address, this tx will be reverted. Is this as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rollupId in range 2**32 so it cannot match
/** | ||
* @dev Checks whether the candidate has created a rollup before. | ||
*/ | ||
function _isRollupOwner( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function _isRollupOwner( | |
function _isRollupOwned( |
// TODO: remove this line in the next upgrade | ||
if (_firstTrackedPeriodEnd == 0) { | ||
_firstTrackedPeriodEnd = _lastUpdatedPeriod; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain the context for this change
delete _delegatorL2MiningReward[vId]; | ||
delete _validatorL2MiningReward[vId]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change do not follow CEI pattern
Description
PR to merge from implement-feature/zkevm-integration/draft-impl to feature/zkevm-integration.