refactor: move remaining miner shared helpers to helpers.py#435
Conversation
|
Hey, @anderdc |
5956563 to
80d9f2b
Compare
anderdc
left a comment
There was a problem hiding this comment.
Three of the four items (NETUID_DEFAULT, _load_config_value, _error) are clean moves. But _resolve_endpoint was reimplemented instead of moved, introducing a behavioral change:
- Missing
.lower()on network name: The original delegates toresolve_network()which doesnetwork.lower()before theNETWORK_MAPlookup. The new standalone version skips this. If a user passes--network FinneyorFINNEY, the old code resolves it; the new code silently treats it as a custom URL.
Fix: either keep _resolve_endpoint as a thin wrapper around resolve_network (true to the "move, don't change" intent), or add .lower() to the new NETWORK_MAP lookup.
|
Thanks for your detailed review, @anderdc |
anderdc
left a comment
There was a problem hiding this comment.
The diff is unchanged from the last review — the fix mentioned in your comment does not appear to have been pushed.
Two things still need to happen:
_resolve_endpointis still a reimplementation, not a move. The old version was a one-liner wrappingresolve_network()fromissue_commands/helpers. The new version duplicates that logic standalone. Either keep it as a thin wrapper (just move the import), or justify why a second implementation is needed.- Double config file reads. The new
_resolve_endpointcalls_load_config_valuetwice (two separate JSON parses of the same file). The existingresolve_networkloads config once viaload_config(). This is a regression if you keep the standalone version.
anderdc
left a comment
There was a problem hiding this comment.
Correction on my last review — the .lower() fix was pushed, that part is resolved.
The remaining concern is minor: _resolve_endpoint is now a standalone reimplementation rather than a wrapper around resolve_network. There is one small behavioral difference — if someone has an unrecognized network name in their config, the old code defaults to finney while the new code returns the raw value. Arguably the new behavior is better.
Not blocking on this. If you want to keep the standalone version that is fine — just be aware the two implementations could drift over time.
Dismissing — the .lower() fix was pushed, I missed it. Remaining duplication concern is non-blocking.
anderdc
left a comment
There was a problem hiding this comment.
Looks good — clean move of the shared helpers. The standalone _resolve_endpoint is functionally correct after the .lower() fix.
Follow-up to #426, based on reviewer feedback.
What
_error,_load_config_value,_resolve_endpoint, andNETUID_DEFAULTwere still defined inpost.pyand imported from there bycheck.py. This moves all of them intominer_commands/helpers.py, which was introduced in #426 as the dedicated home for shared miner utilities.Why
post.pyis a command file, not a utility module. Havingcheck.pyimport shared helpers from a sibling command was an awkward dependency that would only grow worse if a third miner command were added.helpers.pyis the right home — it keeps command files as pure command files and gives all shared utilities a single, predictable location.Changes
helpers.py— gainsNETUID_DEFAULT,_load_config_value,_resolve_endpoint,_errorpost.py— removes those definitions, imports them fromhelpers.pycheck.py— imports everything fromhelpers.pydirectly, no longer touchespost.pyfor shared utilitiesPyright clean
Ruff lint and format clean