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

fork tests on other chains + fork test bug fix #556

Closed
wants to merge 6 commits into from

Conversation

anna-carroll
Copy link
Member

Motivation

Solution

PR Checklist

  • Added Tests
  • Updated Documentation
  • Updated CHANGELOG.md for the appropriate package

@anna-carroll anna-carroll requested a review from a team as a code owner November 29, 2022 17:54
@anna-carroll anna-carroll self-assigned this Nov 29, 2022
Copy link
Member Author

@anna-carroll anna-carroll left a comment

Choose a reason for hiding this comment

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

2 annoying bits = RPC & block number

@@ -7,7 +7,7 @@ import {NomadTest} from "@nomad-xyz/contracts-core/contracts/test/utils/NomadTes

contract RebootTest is RebootLogic, NomadTest {
string remote;
string constant _domain = "ethereum";
string constant testDomain = "ethereum";

function setUpReboot(string memory testName) public {
// ALL
Copy link
Member Author

Choose a reason for hiding this comment

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

we could get RPC from config instead of .env, that would make it easier to switch between chains quickly, but the config RPC situation is grim because those are all public endpoints that are usually quite frail

Copy link
Member Author

Choose a reason for hiding this comment

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

additionally, we could remove the pinned block number in order to make it more seamlessly compatible with all chains, but then the tests wouldn't cache the block which would make them even slower

Copy link
Member

Choose a reason for hiding this comment

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

don't get RPC from config, we don't want to keep good RPCs in those

@anna-carroll anna-carroll requested a review from a team as a code owner November 29, 2022 18:21
@anna-carroll anna-carroll changed the title make fork tests more easily configured to other chains fork tests on other chains + misc fork test touchup Nov 29, 2022
@anna-carroll anna-carroll changed the title fork tests on other chains + misc fork test touchup fork tests on other chains + fork test bug fix Nov 29, 2022
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

should be multiple PRs

  • test not updater sig
  • config changes
  • fix writeJson in reboot setup
  • refactor reboots to allow non-ethereum runs

@anna-carroll anna-carroll deleted the anna/fork-all-chains branch November 29, 2022 21:52
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.

2 participants