Support monitoring of blockchain forks #302
Conversation
Merge stuff from Chiadog dev branch.
There was a problem hiding this comment.
Thanks for the PR!
I'm not sure I like the overhead of propagating these values across all object constructors. Since this is a global configuration that must be accessed by majority components in the system, I think it'll be better to define the values globally instead. What do you think?
Benefit of that is that the default value will only be defined in a single place as well and easy to change, compared to now having default values hardcoded in all constructors and making it hard to change / maintain.
src/chia_log/handlers/daily_stats/stat_accumulators/wallet_added_coin_stats.py
Outdated
Show resolved
Hide resolved
src/chia_log/log_consumer.py
Outdated
| return 'chia' | ||
|
|
||
| def get_symbol(self): | ||
| return 'xch' |
There was a problem hiding this comment.
Some left-overs that shouldn't be here?
There was a problem hiding this comment.
Can these methods be fully implemented in the abstract class, and the child classes call:
super().__init__(prefix, symbol)
Agreed. How would you suggest I best define the values globally, that fits in with the current code base? I want to follow best practice here. Thanks for the review! |
Fix coin symbol display in daily summary.
Fix formatting of symbol.
There was a problem hiding this comment.
Thanks for considering the current architecture. Let's stick to this approach so we can minimise refactoring work in the scope of this PR. It's generally better to have config passed in the constructor for unit testing and modularity.
Ideally a single dictionary that already contains all necessary configs to create the object. I've attempted to pass sub-configuration trees to objects so they only get what's relevant for their instantiation but coin name and symbol are at the root of the tree so we'll either need to pass full config object or pass these two additionally to the config subtree.
Let's stick to your initial approach. Only thing I don't like is that the code will be very easy to break without noticing if some object is instantiated without passing coin name or symbol in the constructor and the construction will succeed with the default value. It'll only break functionality for fork coins but still something to consider. Making the values required on the other hand, will require hardcoding chia defaults everywhere in the test cases. Which in my opinion, would still be preferred.
src/chia_log/log_consumer.py
Outdated
| return 'chia' | ||
|
|
||
| def get_symbol(self): | ||
| return 'xch' |
There was a problem hiding this comment.
Can these methods be fully implemented in the abstract class, and the child classes call:
super().__init__(prefix, symbol)
Refactoring.
…i#307) This commits adds new notification for plot increases (e.g connecting HDD) and makes the notifications for increases/decreases configurable via the config for every integration.
Re-sync with upstream.
Extracted the
coin_symbol(default: xch),coin_name(default: chia), andprefix(default:chia) as configurable settings in config.yaml. This allows me to run Chiadog against blockchain forks such as Flax, NChain, HDDCoin, and Chives.All 3 new configuration values are optional and have defaults as shown above so any existing Chia-only users will not need to touch their config.yaml at all. This PR just adds the possibility of monitoring other blockchain forks without detracting in any way from the existing Chia monitoring.