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

Refactor Configuration #654

Closed
wants to merge 5 commits into from
Closed

Refactor Configuration #654

wants to merge 5 commits into from

Conversation

tilacog
Copy link
Contributor

@tilacog tilacog commented Apr 27, 2023

Note:

#659 refactors some changes from this PR, most notably the input validation module.
Reviewing #659 instead of this PR for the latest refactorings is advised.

  1. Refactors configuration to accept the following options as lists instead of single values:

    • --network-provider (new name for ethereum, which is now an alias)
    • --epoch-subgraph
    • --network-subgraph-endpoint
    • --network-subgraph-deployment
  2. Introduces:

    • a validation function validateNetworkOptions to sanitize the new cardinality of the network options.
    • a input-parsing.ts module with parsers for the new network identification format.
  3. To ensure compatibility, this PR restricts its scope to argument parsing and does not incorporate the network multiplexing changes expected from Refactor the Network component to hold different networks #652. which will resolve all the FIXMES introduced here.

New dependencies:

  • parsimmon: for the new input-parsing.ts module.
  • lodash-countby: used in the new validateNetworkOptions function.

Closes #650

@tilacog tilacog requested review from fordN and hopeyen April 27, 2023 22:06
@tilacog tilacog self-assigned this Apr 27, 2023
@tilacog tilacog marked this pull request as ready for review April 28, 2023 01:23
Copy link
Contributor

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

looks pretty good! how do you think untaggedUrl would be used?

usedOptions += ']'

// Check for consistent length across network options
const commonSize = new Set(arraysToCheck.map(a => a.length)).size
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this check given that the next one checks for consistent in identification?

I have an idea that maybe instead of arraysToCheck, we can create a struct for network configurations, and try to fill them with the necessary fields; Everything is okay if we can successfully make a dict or array of network structs for all provided network identifiers, otherwise fail validations by missing or duplicated args? The network structs we make here can be returned and utilized later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!
I believe I did a somewhat similar refactor in #659.

@tilacog
Copy link
Contributor Author

tilacog commented May 12, 2023

looks pretty good! how do you think untaggedUrl would be used?

untagged inputs will be used if an indexer prefers to stick to a single network configuration.

Indexers will likely use untagged and single network configurations for the upcoming release, as using multiple networks will block the next database migration.

@tilacog
Copy link
Contributor Author

tilacog commented May 12, 2023

#659 refactors some changes from this PR, most notably the input validation module.
Reviewing #659 instead of this PR for the latest refactorings is advised.

@tilacog
Copy link
Contributor Author

tilacog commented Aug 29, 2023

Resolved by #668

@tilacog tilacog closed this Aug 29, 2023
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.

Refactor configuration
2 participants