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

feat: Allow individual list of monikers per network #9

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

qwertzlbert
Copy link
Contributor

@qwertzlbert qwertzlbert commented Nov 29, 2024

PR(Pull Request) Overview

A simple change to allow defining individual lists of monikers for each network.

If no monikers variable is defined, the content of the root monikers variable will be used.

This helps in situation were you need to track different monikers across different networks, without the need to define them all globally for all networks

Changes

  • Major feature addition or modification
  • Bug fix
  • Code improvement
  • Documentation update

Description of Changes

This change allows to specify individual lists of monikers which should be tracked per configured chain. E.g.

monikers:
  - 'validator1'
  - 'validator2'


  - display_name: 'band'
    chain_id: 'laozi-mainnet'
    monikers:
      - 'validator3'  # <-- will only track 'validator3'
    tracking_addresses:
      - 'xyz'
    nodes:
      - ...


  - display_name: 'injective' # <-- will track 'validator1' and 'validator2'
    chain_id: injective-1
    tracking_addresses:
      - 'xyz'
    nodes:
      - ...

Testing Method

Just a simple execution to test if the overwrite works or not. I did not add any tests yet

…er' config as default if nothing is defined

Signed-off-by: Simon Lichtenauer <[email protected]>
@carameleon
Copy link
Member

carameleon commented Dec 2, 2024

Hi @qwertzlbert
Thank you for your contribution.

Our goal was to simplify the configuration as much as possible. Therefore, currently, if you specify monikers as an array, it will track all chains where those monikers exist.

Your PR seems to be valid for the following scenario:
Let's assume we have:

  • chain: A, moniker: abcd
  • chain: B, moniker: abcd

And you want to track only abcd from chain A among these two chains, is that correct?

We considered such cases as well, but thought they were rare. Therefore, we chose to specify the monikers in an array and track them if they exist.

@qwertzlbert
Copy link
Contributor Author

@carameleon Not only that, but in some cases I might want to get data for different validator monikers from different networks to be able to compare them. This is especially useful in scenarios were I track a lot of different chains, but want to track different monikers across them. Currently this would mean I have to define a big list with monikers which also might return data I'm not interested in.
E.g I only want to track my validator and the current leading validator, which might always be a different moniker.

  • chain: A, moniker: MyAwesomeValidator, cosmostation
  • chain: B, moniker: MyAwesomeValidator, OtherValidator

The change I propose leaves you the option to keep it simple and just define a global list of moniker, the same way as it is now, while still having the flexibility to define an override list for each network if you want.
So it does not really complicate things, but is just a small change providing more flexibility if needed.

Hope this explains the use-case a bit better

@qwertzlbert
Copy link
Contributor Author

Just found some issues with my change. I'll close the PR until I'm ready

@qwertzlbert qwertzlbert closed this Dec 5, 2024
…fferent monikers per chain config due to multiple simultanious write access (#2)

* fix: remove global variable as it creates bogus results when using different monikers per chain config due to multiple simultanious write access

Signed-off-by: Simon Lichtenauer <[email protected]>

* remove log message used for debugging

Signed-off-by: Simon Lichtenauer <[email protected]>

---------

Signed-off-by: Simon Lichtenauer <[email protected]>
@qwertzlbert qwertzlbert reopened this Dec 5, 2024
@@ -17,9 +17,6 @@ var (
_ common.CollectorLoop = loop
)

// NOTE: this is for solo mode
Copy link
Contributor Author

@qwertzlbert qwertzlbert Dec 5, 2024

Choose a reason for hiding this comment

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

the global packageMonikers variable needs to be removed in all packages as it created race conditions due to multiple loops writing to this variable at the same time. This worked as long as the moniker variable was the same for all loops, but with my added change which allows moniker configuration per chain, this creates unexpected results.
As the config is passed to the loop via the p variable anyway, p.Monikers is used instead, removing the race-condition

@qwertzlbert
Copy link
Contributor Author

qwertzlbert commented Dec 5, 2024

Found the root cause of the issue faster than I thought so re-opened the PR.

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