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.

Sign up for GitHub

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

Jump to bottom

Replace network parameter with indexer host in config #17

Merged
merged 2 commits into from
Oct 6, 2023
Merged

Replace network parameter with indexer host in config #17

merged 2 commits into from
Oct 6, 2023

Conversation

kovalgek
Copy link
Contributor

@kovalgek kovalgek commented Oct 4, 2023

WHAT

Setting network name isn't enough for some indexers. For instance base has .org name instead of .io.

HOW

Use indexer host as parameter to make diffyscan more flexible.

TheDZhon
TheDZhon approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you for pushing this forward!

Left a minor suggestion after looking over the change proposed once again.

config.sample.json Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
mymphe
mymphe approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@mymphe mymphe left a comment

Choose a reason for hiding this comment

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

LGTM!

The proposed change seems to make Diffyscan more flexible, however, this makes the network implicit which could potentially lead to some misconfiguration bugs.

@TheDZhon
Copy link
Contributor

TheDZhon commented Oct 5, 2023 •
edited
Loading

@mymphe Hmm, IDK

https://goerli.etherscan.io/
https://goerli.basescan.org/
https://basescan.org/
https://optimistic.etherscan.io/
https://goerli-optimism.etherscan.io/

I mean the naming approach is non-uniform :(

@kovalgek kovalgek merged commit ae5ecc3 into main Oct 6, 2023
@TheDZhon TheDZhon deleted the replace-network-with-indexer-host branch November 11, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mymphe mymphe mymphe approved these changes

@TheDZhon TheDZhon TheDZhon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kovalgek @TheDZhon @mymphe