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

Testing on forked chains #1164

Conversation

Alderian
Copy link

Problem:

  • Current TestHelper creates various endpoints, but the are all created on the same network
  • The emulate message sending, but never leave the current network
  • There is no way to test omnichain applications using already deployed contracts on forked chains

Solution:

The solution we came up with @shankars99 have these advantages:

1- It let you create and configure one endpoints for each fork you want to test
2- ensures oapps wires up thru the forks
3- It will let ppl test messaging and interact with already deployed contracts

Example usage:

  • There is a MyOapp.t.sol that works
  • Production example could be if you want a messaging system and you need to use Oracles or usa Uniswap to exchange, or any other contract that is already deployed and you want to test with those contracts.

Next steps:

  1. Add something similar with hardhat (its possible to use the hardhat-switch-network plugin or similar
  2. Add documentation on how to use this
  3. Add the possibility to have more endpoints by fork

- One Price feed for each network
- Ensure using correct fork for endpoint setup and OApp wireup
- Reorder sendMessage test to ensure we get correct fork contracts
Copy link
Contributor

@ItsAdel ItsAdel left a comment

Choose a reason for hiding this comment

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

As discussed earlier not all changes in this PR reflect all the changes of the fork chains as done so in the branch. This PR seems moreso to address the priceFeeds and makes additional updates to the branch utilizing vm.selectFork which lgtm 🚢

I think this should be approved to be merged into the feature branch. And then another PR to merge this entire branch to main will be made which will reflect all the changes and give the bigger picture.

Also @DanL0 to address the worry about backward compatibility for other tests,
the older tests run as before.
(Review f39c9be )

function setUpEndpoints(uint8 _endpointNum, LibraryType _libraryType)
function createEndpoints(
        uint8 _endpointNum,
        LibraryType _libraryType,
        address[] memory nativeTokenAddresses
)
function createEndpoints(
        uint8 _endpointNum,
        LibraryType _libraryType,
        address[] memory nativeTokenAddresses,
        string[] memory forkUrls
)

are setup to overload the functions as necessary. Passing a default forkUrls array if not specified by old tests. Here it ensures the old tests run as they were before. Only new tests where forkUrls are passed will run the tests with this new fork feature.

@shankars99
Copy link
Contributor

lets keep this hanging another day. i need to look into the changes. i thought alderian was merging this into main and not into my branch lol

@Alderian
Copy link
Author

Alderian commented Feb 4, 2025

Hi... I forked on my github from LZ branch created by @shankars99 because I assumed I wouldn't have permission to commit here. Then I fixed @DanL0 comments and some little stuff and added the analysis of the solution to clarify.

I thought that the correct path of merge is first merge my changes to LZ branch created by Shankar and then merge to main if it's all ok...

Hope it's all.good! Lmk if I can be of help in any way

Side note... This new piece of code we cooked with Shankar helped our project to test some complex scenarios with existing contracts on forked Blockchain...

Thanks a lot!

@DanL0
Copy link
Contributor

DanL0 commented Feb 4, 2025

Assigning @shankars99 because this PR base branch is @shankars99 branch

@shankars99
Copy link
Contributor

Hi... I forked on my github from LZ branch created by @shankars99 because I assumed I wouldn't have permission to commit here. Then I fixed @DanL0 comments and some little stuff and added the analysis of the solution to clarify.

I thought that the correct path of merge is first merge my changes to LZ branch created by Shankar and then merge to main if it's all ok...

Hope it's all.good! Lmk if I can be of help in any way

Side note... This new piece of code we cooked with Shankar helped our project to test some complex scenarios with existing contracts on forked Blockchain...

Thanks a lot!

oh no you're all good it's just that I (mostly) forgot the changes that we made and want to take another look. and also have a look at your changes

@shankars99
Copy link
Contributor

shankars99 commented Feb 4, 2025

lgtm + thanks for the fix and cleanup on the eid

@shankars99 shankars99 merged commit 69715ff into LayerZero-Labs:feat/fork-evm-testing Feb 4, 2025
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants