Skip to content
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

test: Fork Integration Testing refactor #433

Closed
wants to merge 30 commits into from
Closed

Conversation

8sunyuan
Copy link
Collaborator

With the different upgrade paths between Goerli and Mainnet, this is an attempt at having more upgrade testing by using the utils parser ExistingDeploymentParser and deploying in IntegrationDeployer.setUp() depending on the chainid that tests are being run on.

To run upgrade integration tests

  1. Setup anvil node
    anvil -f $RPC_GOERLI
  2. Run integration tests
    forge test --match-contract Integration -f http://127.0.0.1:8545

ypatil12 and others added 27 commits January 2, 2024 16:45
* Fix: flaky integration tests (#384)

* feat: strat settings

* Fix: flaky integration tests (#384)

* feat: strat settings

* feat: withdrawalDelayBlocks per strategy

* fix: set deprecated storage to private

* fix: pr review changes

* fix: require string

* docs: updated

* refactor: rename creditTransfersDisabled

* fix: doc typos

* docs: add new methods and fix formatting

* fix: nits and getWithdrawals view

* docs: add link

---------

Co-authored-by: Yash Patil <40046473+ypatil12@users.noreply.github.com>
Co-authored-by: wadealexc <pragma-services@proton.me>
* fix: add contract size check

* fix: ignore harness build size
* refactor: initial draft

* fix: revert require chages

* fix: small nits
* test: added back avsRegistration tests

* fix: fuzz runs 4096

* fix: broken fuzz test
* init commit

* updated testFullWithdrawalFlow to deneb spec

* added two proof paths

* added both capella and deneb testS

* added testFullWithdrawalFlowCapellaWithdrawalAgainstDenebRoot

* added event

* fixed storage gap

* uncommented testsg

* fix: remove line

* fixed tesst

* added a setter in the EPM for deneForkTimetamp

* tests still broken

* cleanup

* added modifier

* fixing tests

* tests working

* added tests

* comments

* fixed failing test

* fix flaky test

* removed modifier

---------

Co-authored-by: gpsanant <gpsanant@gmail.com>
* feat: simplify fork timestamp setting logic

* test: fix tests to account for only setting timestamp once

---------

Co-authored-by: wadealexc <pragma-services@proton.me>
* Create GoerliUpgrade2.s.sol

* preprod deploy

* Update GV2_preprod_deployment_2024_30_1.json

* nit: comments

* avs directory already deployed

* preprod deploy
warnings were for unused or shadowed variables, or functions that could have stricter mutability
This reverts commit c3d7bff.
* fix: fixed comment

* fix: removed dead space

* fix: removed extraneous the

* fix: edited another comment
* fix: try installing solc-select

* fix: addShares selector
This provides additional signature replay protection
for the `StrategyManager.depositIntoStrategyWithSignature` method

Specifically, it addresses the issue outlined in
https://mirror.xyz/curiousapple.eth/pFqAdW2LiJ-6S4sg_u1z08k4vK6BCJ33LcyXpnNb8yU
where some ERC1271 wallets might be vulnerable to "replays" of signatures

While the theoretical "damage" would be ~zero
(allowing someone to deposit and credit the deposit to a user),
adding this field to the typehash seems to be best practice, at least.
* fix: updated doc

* fix: changed more incorrect references fo verifyBalanceUpdates

* fix: changed more incorrect references fo verifyBalanceUpdates

* fix: fixed image

* fix: fixed incorrect comment

* docs: fix formatting

---------

Co-authored-by: wadealexc <pragma-services@proton.me>
* feat: slight refactor to make better use of strategybase hooks

* docs: add clarifying comment
this should be MIT licensed; looks like this was missed
@8sunyuan 8sunyuan marked this pull request as draft February 12, 2024 19:41
Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

Would be nice to have something that can target mainnet, if possible

@wadealexc
Copy link
Collaborator

wadealexc commented Feb 12, 2024

Would be nice to have something that can target mainnet, if possible

Wait nvm there's a setupMainnet method...

edit: oh, but it's not impled yet. okay!

seems like a great starting point!

} else if (block.chainid == 31337) {
_setUpLocal();
} else {
revert();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably good to have a message here, otherwise this would be a meganightmare to debug

@8sunyuan
Copy link
Collaborator Author

Would be nice to have something that can target mainnet, if possible

Wait nvm there's a setupMainnet method...

Yeah not sure if this is the best place/method to do the upgrade testing so haven't added that in yet. But it made the most sense as there are no mocks setup(outside of beacon stuff) and this will make us take more advantage of our integration testing.

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Awesome start! I think there are two parts to this idea of integration tests post-upgrade

  1. Normal interaction on M1 -> Upgrade -> Continue interaction on M2
  2. Upgrade, and ensure that all interaction on M1 can be done on M2

I think this framework does 2, but not necessarily 1? Couple of example scenarios:

  • queueing a withdrawal on M1 and completing on M2 (tested via WithdrawalMigration.t.sol)
  • depositing on M1 and then doing the entire LST lifecycle on M2 (delegate, queue, complete)
  • creating a pod in M1 and pointing WC and then going through the entire pod lifecycle on M2 (verifying WC, balance updates, partial/full withdrawals)
  • setting an admin var in M1 and then resetting on M2

For 2, I think the main things we should test are normal interaction, which we can easily do with this framework and the already written integration tests, and setting all the admin variables, which could catch the setting of withdrawalDelayBlocks

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Also, think we should update the CI to run the integration and off of a fork in addition to the local execution!

Base automatically changed from m2-mainnet-fixes to m2-mainnet February 16, 2024 16:37
@olarithecreator
Copy link

just read through this!!, very awesome

@8sunyuan 8sunyuan closed this Mar 26, 2024
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.

7 participants