Fix accounting tests for vaults #912
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: ade6c03 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
folkyatina
reviewed
Jan 3, 2025
vp4242
force-pushed
the
feat/fix-accounting-tests-vaults
branch
from
58f9dc9 to
d456680
Compare
folkyatina
reviewed
Jan 14, 2025
folkyatina
requested changes
Jan 14, 2025
test/deploy/dao.ts
Outdated
| @@ -79,7 +79,14 @@ export async function deployLidoDao({ rootAccount, initialized, locatorConfig = | |||
| await lido.initialize(locator, eip712steth, { value: ether("1.0") }); | |||
| } | |||
|
|
|||
| return { lido, dao, acl }; | |||
| const locator = await lido.getLidoLocator(); | |||
There was a problem hiding this comment.
Looks like these lines add accounting deployment to the dozen of different test suites. Do we really need it this way? It will slow down all the test pipeline. Maybe, we need a separate function for it?
| @@ -125,14 +125,6 @@ contract OracleReportSanityCheckerStub { | |||
| uint256 _reportTimestamp | |||
| ) external view {} | |||
|
|
|||
| function checkSimulatedShareRate( | |||
There was a problem hiding this comment.
Seems like this contract is not used at all and may be deleted safely. Probably, even the whole file.
folkyatina
reviewed
Jan 16, 2025
|
|
||
| let elRewardsVault: LidoExecutionLayerRewardsVault__MockForLidoAccounting; | ||
| let withdrawalVault: WithdrawalVault__MockForLidoAccounting; | ||
| let stakingRouter: StakingRouter__MockForLidoAccounting; | ||
| let oracleReportSanityChecker: OracleReportSanityChecker__MockForAccounting; |
There was a problem hiding this comment.
Lido don't use sanity checker anymore, so we can skip this part of initialization.
|
|
||
| let lido: Lido; | ||
| let acl: ACL; | ||
| // let locator: LidoLocator; | ||
| let postTokenRebaseReceiver: IPostTokenRebaseReceiver; |
There was a problem hiding this comment.
Lido don't use postTokenRebaseReceiver too, so we can skip it.
| let locator: LidoLocator; | ||
|
|
||
| let lido: Lido__MockForAccounting; | ||
| let elRewardsVault: LidoExecutionLayerRewardsVault__MockForLidoAccounting; |
There was a problem hiding this comment.
We can remove excess dependencies if testing only Accounting: elRewardsVault and withdrawalVault
| it("Reverts if the `checkAccountingOracleReport` sanity check fails", async () => { | ||
| await oracleReportSanityChecker.mock__checkAccountingOracleReportReverts(true); | ||
|
|
||
| await expect(accounting.handleOracleReport(report())).to.be.reverted; |
There was a problem hiding this comment.
Need to check the exact reason
| withdrawalFinalizationBatches: [1n], | ||
| }), | ||
| ), | ||
| ).to.be.reverted; |
|
|
||
| /// NOTE: This test is not applicable to the current implementation (Accounting's _checkAccountingOracleReport() checks for checkWithdrawalQueueOracleReport() | ||
| /// explicitly in case _report.withdrawalFinalizationBatches.length > 0 | ||
| // it("Does not revert if the `checkWithdrawalQueueOracleReport` sanity check fails but `withdrawalQueue` is paused", async () => { |
There was a problem hiding this comment.
It maybe a bug in a new Accounting, BTW. Need to check.
There was a problem hiding this comment.
Could be. Previous implementation have explicit check for pause state
core/contracts/0.4.24/Lido.sol
Line 879 in 901c0e1
folkyatina
approved these changes
Jan 19, 2025
| }), | ||
| ), | ||
| ) | ||
| .and.to.emit(lido, "TransferShares") |
There was a problem hiding this comment.
Looks like formatter troubles
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix accounting tests for vaults