Skip to content

Conversation

@arwer13
Copy link
Contributor

@arwer13 arwer13 commented Jul 14, 2025

Only for the contracts updated/deployed on Triggerable Withdrawals upgrade.

@arwer13 arwer13 requested a review from a team as a code owner July 14, 2025 18:22
@github-actions
Copy link

github-actions bot commented Jul 14, 2025

badge

Hardhat Unit Tests Coverage Summary

Filename                                                       Stmts    Miss  Cover    Missing
-----------------------------------------------------------  -------  ------  -------  -------------------------
contracts/0.4.24/Lido.sol                                        212       0  100.00%
contracts/0.4.24/StETH.sol                                        72       0  100.00%
contracts/0.4.24/StETHPermit.sol                                  15       0  100.00%
contracts/0.4.24/interfaces/IStakingModule.sol                     0       0  100.00%
contracts/0.4.24/lib/Packed64x4.sol                                5       0  100.00%
contracts/0.4.24/lib/SigningKeys.sol                              36       0  100.00%
contracts/0.4.24/lib/StakeLimitUtils.sol                          37       0  100.00%
contracts/0.4.24/nos/NodeOperatorsRegistry.sol                   435       0  100.00%
contracts/0.4.24/oracle/LegacyOracle.sol                          72       0  100.00%
contracts/0.4.24/utils/Pausable.sol                                9       0  100.00%
contracts/0.4.24/utils/Versioned.sol                               5       0  100.00%
contracts/0.6.12/WstETH.sol                                       17       0  100.00%
contracts/0.8.25/ValidatorExitDelayVerifier.sol                   72      15  79.17%   202-210, 249-257, 316-366
contracts/0.8.4/WithdrawalsManagerProxy.sol                       61       0  100.00%
contracts/0.8.9/BeaconChainDepositor.sol                          21       2  90.48%   48, 51
contracts/0.8.9/Burner.sol                                        71       0  100.00%
contracts/0.8.9/DepositSecurityModule.sol                        128       0  100.00%
contracts/0.8.9/EIP712StETH.sol                                   16       0  100.00%
contracts/0.8.9/LidoExecutionLayerRewardsVault.sol                16       0  100.00%
contracts/0.8.9/LidoLocator.sol                                   20       0  100.00%
contracts/0.8.9/OracleDaemonConfig.sol                            28       0  100.00%
contracts/0.8.9/StakingRouter.sol                                305       0  100.00%
contracts/0.8.9/TriggerableWithdrawalsGateway.sol                 54       1  98.15%   212
contracts/0.8.9/WithdrawalQueue.sol                               88       0  100.00%
contracts/0.8.9/WithdrawalQueueBase.sol                          146       0  100.00%
contracts/0.8.9/WithdrawalQueueERC721.sol                         89       0  100.00%
contracts/0.8.9/WithdrawalVault.sol                               32       0  100.00%
contracts/0.8.9/WithdrawalVaultEIP7002.sol                        21       0  100.00%
contracts/0.8.9/lib/ExitLimitUtils.sol                            35       0  100.00%
contracts/0.8.9/lib/Math.sol                                       4       0  100.00%
contracts/0.8.9/lib/PositiveTokenRebaseLimiter.sol                22       0  100.00%
contracts/0.8.9/lib/UnstructuredRefStorage.sol                     2       0  100.00%
contracts/0.8.9/oracle/AccountingOracle.sol                      193       0  100.00%
contracts/0.8.9/oracle/BaseOracle.sol                             89       1  98.88%   348
contracts/0.8.9/oracle/HashConsensus.sol                         263       1  99.62%   1005
contracts/0.8.9/oracle/ValidatorsExitBus.sol                     138       1  99.28%   407
contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol                52       1  98.08%   156
contracts/0.8.9/proxy/OssifiableProxy.sol                         17       0  100.00%
contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol      232       0  100.00%
contracts/0.8.9/utils/DummyEmptyContract.sol                       0       0  100.00%
contracts/0.8.9/utils/PausableUntil.sol                           31       0  100.00%
contracts/0.8.9/utils/Versioned.sol                               11       0  100.00%
contracts/0.8.9/utils/access/AccessControl.sol                    23       0  100.00%
contracts/0.8.9/utils/access/AccessControlEnumerable.sol           9       0  100.00%
contracts/testnets/sepolia/SepoliaDepositAdapter.sol              21      21  0.00%    49-100
TOTAL                                                           3225      43  98.67%

Diff against master

Filename                                              Stmts    Miss  Cover
--------------------------------------------------  -------  ------  --------
contracts/0.4.24/interfaces/IStakingModule.sol            0       0  +100.00%
contracts/0.4.24/nos/NodeOperatorsRegistry.sol          -77       0  +100.00%
contracts/0.8.25/ValidatorExitDelayVerifier.sol         +72     +15  +79.17%
contracts/0.8.9/LidoLocator.sol                          +2       0  +100.00%
contracts/0.8.9/StakingRouter.sol                       -11       0  +100.00%
contracts/0.8.9/TriggerableWithdrawalsGateway.sol       +54      +1  +98.15%
contracts/0.8.9/WithdrawalVault.sol                     +11       0  +100.00%
contracts/0.8.9/WithdrawalVaultEIP7002.sol              +21       0  +100.00%
contracts/0.8.9/lib/ExitLimitUtils.sol                  +35       0  +100.00%
contracts/0.8.9/oracle/AccountingOracle.sol              +3      -2  +1.05%
contracts/0.8.9/oracle/ValidatorsExitBus.sol           +138      +1  +99.28%
contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol      -39      -1  +0.28%
TOTAL                                                  +209     +14  -0.37%

Results for commit: c841cdc

Minimum allowed coverage is 95%

♻️ This comment has been updated with latest results

F4ever
F4ever previously approved these changes Jul 14, 2025
Copy link
Member

@tamtamchik tamtamchik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets fix some issues ⏲️

package.json Outdated
"test:integration:fork:mainnet:custom": "MODE=forking hardhat test --network mainnet-fork",
"typecheck": "tsc --noEmit",
"prepare": "husky",
"check:interfaces": "bash -c 'forge inspect contracts/common/interfaces/IStakingModule.sol abi | diff -u - <(forge inspect contracts/0.4.24/interfaces/IStakingModule.sol abi)'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to hardhat tasks overrides maybe?

import {IStakingRouter} from "contracts/common/interfaces/IStakingRouter.sol";

contract StakingRouter_Mock is IStakingRouter {
contract StakingRouter_Mock {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should implement interface here.

import {IStakingRouter} from "contracts/common/interfaces/IStakingRouter.sol";

contract StakingRouter_Mock is IStakingRouter {
contract StakingRouter_Mock {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
contract StakingRouter_Mock {
contract StakingRouter__Mock {

import {IStakingRouter} from "contracts/0.8.9/oracle/AccountingOracle.sol";

contract StakingRouter__MockForAccountingOracle is IStakingRouter {
contract StakingRouter__MockForAccountingOracle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

import {StakingRouter} from "contracts/0.8.9/StakingRouter.sol";

contract StakingRouter__MockForDepositSecurityModule is IStakingRouter {
contract StakingRouter__MockForDepositSecurityModule {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we do not need this, can be changed to ABI comparacent

@arwer13
Copy link
Contributor Author

arwer13 commented Jul 16, 2025

Current output:

Checking interfaces...
✅ contracts/0.8.9/oracle/BaseOracle.sol:BaseOracle fully matches contracts/common/interfaces/IBaseOracle.sol:IBaseOracle
ℹ️  skipping 'contracts/common/interfaces/IBurner.sol:IBurner' and 'contracts/0.8.9/Burner.sol:Burner' (Burner is scheduled for V3 upgrade)
ℹ️  skipping 'contracts/common/interfaces/IEIP712StETH.sol:IEIP712StETH' and 'contracts/0.8.9/EIP712StETH.sol:EIP712StETH' (StETH is scheduled for V3 upgrade)
•  skipping 'contracts/common/interfaces/IGateSealFactory.sol:IGateSealFactory' - no other such interfaces of contracts found
ℹ️  skipping 'contracts/common/interfaces/IHashConsensus.sol:IHashConsensus' and 'contracts/0.4.24/oracle/LegacyOracle.sol:IHashConsensus' (LegacyOracle is obsolete)
✅ contracts/0.8.9/utils/PausableUntil.sol:PausableUntil fully matches contracts/common/interfaces/IPausableUntil.sol:IPausableUntil
✅ contracts/0.8.9/oracle/HashConsensus.sol:IReportAsyncProcessor is sub-interface of contracts/common/interfaces/IReportAsyncProcessor.sol:IReportAsyncProcessor
✅ contracts/0.4.24/StETH.sol:StETH fully matches contracts/common/interfaces/IStETH.sol:IStETH
✅ contracts/0.4.24/interfaces/IStakingModule.sol:IStakingModule fully matches contracts/common/interfaces/IStakingModule.sol:IStakingModule
✅ contracts/0.4.24/Lido.sol:IStakingRouter is sub-interface of contracts/common/interfaces/IStakingRouter.sol:IStakingRouter
✅ contracts/0.8.9/TriggerableWithdrawalsGateway.sol:TriggerableWithdrawalsGateway fully matches contracts/common/interfaces/ITriggerableWithdrawalsGateway.sol:ITriggerableWithdrawalsGateway
✅ contracts/0.8.9/oracle/ValidatorsExitBus.sol:ValidatorsExitBus fully matches contracts/common/interfaces/IValidatorsExitBus.sol:IValidatorsExitBus
✅ contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol:ValidatorsExitBusOracle fully matches contracts/common/interfaces/IValidatorsExitBusOracle.sol:IValidatorsExitBusOracle
ℹ️  skipping 'contracts/common/interfaces/IVersioned.sol:IVersioned' and 'contracts/0.4.24/utils/Versioned.sol:Versioned' (Versioned for 0.4.24 differs by design)
✅ contracts/0.4.24/Lido.sol:IWithdrawalVault is sub-interface of contracts/common/interfaces/IWithdrawalVault.sol:IWithdrawalVault

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

❌ ABI mismatch between:
   Common interface: contracts/common/interfaces/ILidoLocator.sol:ILidoLocator
   Other interface:  contracts/0.8.9/LidoLocator.sol:LidoLocator
   Match type:       Full match expected

📋 Directly importing contracts (9):
   contracts/0.4.24/Lido.sol
   contracts/0.4.24/nos/NodeOperatorsRegistry.sol
   contracts/0.4.24/oracle/LegacyOracle.sol
   contracts/0.8.25/ValidatorExitDelayVerifier.sol
   contracts/0.8.9/LidoLocator.sol
   contracts/0.8.9/TriggerableWithdrawalsGateway.sol
   contracts/0.8.9/oracle/AccountingOracle.sol
   contracts/0.8.9/oracle/ValidatorsExitBus.sol
   contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol

📋 Indirectly importing contracts (2):
   contracts/0.4.24/template/LidoTemplate.sol
   contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol

📋 Entries missing in common interface (3):
   error ZeroAddress();
   function coreComponents() view returns (address, address, address, address, address, address);
   function oracleReportComponentsForLido() view returns (address, address, address, address, address, address, address);

📋 Entries missing in other interface/contract (2):
   function coreComponents() view returns (address elRewardsVault, address oracleReportSanityChecker, address stakingRouter, address treasury, address withdrawalQueue, address withdrawalVault);
   function oracleReportComponentsForLido() view returns (address accountingOracle, address elRewardsVault, address oracleReportSanityChecker, address burner, address withdrawalQueue, address withdrawalVault, address postTokenRebaseReceiver);

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's settle on this:

  • use embedded interfaces, don't create standalone ones unless strictly needed
  • the whole PR effort should be focused on the tool to check them
  • for TW we suggest embedding ifaces to simplify the way forward

},
{
commonInterfaceFqn: "contracts/common/interfaces/IValidatorsExitBusOracle.sol:IValidatorsExitBusOracle",
additionalInterfaces: ["contracts/0.8.9/oracle/ValidatorsExitBus.sol:ValidatorsExitBus"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Base automatically changed from feat/triggerable-exits to develop August 18, 2025 12:17
@tamtamchik tamtamchik dismissed F4ever’s stale review August 18, 2025 12:17

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants