Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

Allow test L1 IP addresses #58

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Allow test L1 IP addresses #58

merged 2 commits into from
Aug 30, 2022

Conversation

aarshkshah1992
Copy link
Contributor

Allow user to configure a list of test L1 IP Addresses to connect to.

@aarshkshah1992
Copy link
Contributor Author

@juliangruber Merging this. As always, please feel free to leave comments.

@aarshkshah1992 aarshkshah1992 merged commit 9c8a3fc into main Aug 30, 2022
@aarshkshah1992 aarshkshah1992 deleted the feat/allow-test-l1s branch August 30, 2022 11:06
@@ -103,6 +110,8 @@ var (

// DNS Hostname of Saturn L1 Nodes
saturn_l1_hostName = "strn.pl"

defaultOrchestratorURL = "https://orchestrator.strn.pl/nodes/nearby"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The orchestrator url is https://orchestrator.strn.pl, the discovery url is https://orchestrator.strn.pl/nodes/nearby. Therefore I think either we rename this variables to defaultL1DiscoveryUrl or change the env var to not need the path suffix. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the discovery URL for the test net? Is it a different hostname with the same path, e.g. something like https://orchestrator.test.strn.pl/nodes/nearby? In that case providing a host name only may work.

In general, I feel it's better to avoid magic in configuration. If we need to configure discovery URL, then the config option should specify the full URL.

This way it's much easier to mock the discovery service for testing, as the mock does not need to replicate the path structure of the real one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants