-
-
Notifications
You must be signed in to change notification settings - Fork 160
feat(config): support deterministic addresses in config #2593
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
base: main
Are you sure you want to change the base?
feat(config): support deterministic addresses in config #2593
Conversation
3b20684
to
e3cf68b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved, but see comments! you'll like some of the feedback I think
Note on the tests: macos is currently failing occasionally on the gas flag integration tests, I'm looking into it now.. As long the Ubunutu's pass, can merge
if network_name not in ecosystem.networks: | ||
raise ConfigError( | ||
f"Invalid network '{ecosystem_name}:{network_name}' in deployments config." | ||
) | ||
|
||
# NOTE: Merge deterministic deployments after we initializing `NetworkManager.ecosystems` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# NOTE: Merge deterministic deployments after we initializing `NetworkManager.ecosystems` | |
# NOTE: Merge deterministic deployments after we initialize `NetworkManager.ecosystems` |
if network_name not in ecosystem.networks: | ||
raise ConfigError( | ||
f"Invalid network '{ecosystem_name}:{network_name}' in deployments config." | ||
) | ||
|
||
# NOTE: Merge deterministic deployments after we initializing `NetworkManager.ecosystems` | ||
for ecosystem_name, ecosystem in self.network_manager.ecosystems.items(): | ||
if ecosystem_name not in self.deployment_data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do this neat trick:
self.deployment_data.setdefault(ecosystem_name, {})
It does exactly the same as what you're doing here:
for ecosystem_name, ecosystem in self.network_manager.ecosystems.items():
if ecosystem_name not in self.deployment_data:
@@ -297,6 +297,25 @@ def test_deployments(networks_connected_to_tester, owner, vyper_contract_contain | |||
assert deployment.address == instance.address | |||
|
|||
|
|||
def test_deterministic_deployments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_deterministic_deployments( | |
def test_deployments_deterministic( |
I know this sounds backwards, but it lets us do this in pytest
:
-k test_deployments
to run all deployments related tests at once, which is something i do a lot: run all related tests at once when sseeing if broke something
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days. |
This PR was closed because it has been inactive for 35 days. |
af2b8c5
to
cf5e573
Compare
What I did
Support deterministic address config
fixes: #2590
How I did it
Add deterministic
DeploymentConfig
objects to every networkHow to verify it
See issue
Checklist