DAH-2585: fail lium fund non-zero when the coldkey will not unlock - #107
Open
arhangel66 wants to merge 2 commits into
Open
DAH-2585: fail lium fund non-zero when the coldkey will not unlock#107arhangel66 wants to merge 2 commits into
arhangel66 wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #99, which was merged before the second review pass landed. Two review comments, both on the TAO path of
lium fund.ui.error(...)plus a barereturn;ui.erroronly prints, sohandle_errorsnever saw a failure and click exited successfully.lium fund && lium uptherefore treated a failed top-up as a completed one. It now raisesCliFailure("coldkey_unlock_failed", ..., EXIT_CONFIGURATION_ERROR), matchingwallet_load_failedandinvalid_amountin the same function. This is the exact anti-pattern DAH-2593 (DAH-2593: stop the CLI Action layer from swallowing errors #104) removed from the Action layer — the new block reintroduced it in one spot. The alpha path was already correct (it raisesLiumError).CheckWalletRegistrationAction, the only coldkey-signing step —Lium.add_walletreadsbt_wallet.coldkey, which is what triggers bittensor's raw password prompt. The one spinner now ahead of it,ui.load("Loading balance"), never touches the coldkey and so cannot hide a prompt. The guard test was retightened to state that invariant precisely instead of "unlock is the first event".Tests:
test_fund_tao_unlock_failure_abortsnow asserts the exit code, andtest_fund_tao_declined_confirm_never_asks_for_the_passwordis new. Both fail againstmainand pass here.uv run pytest test/test_fund_cli.py -q→ 53 passed. Full suite: 11 pre-existing failures, identical on an untouched tree.No version bump — this needs to ride the next release.
CheckWalletRegistrationAction->Lium.add_walletposts/tao/create-transferand/token/verifyand binds the wallet to the account. Before,lium fund -w new_walletbound the wallet even when the user then declined at the confirm; now declining leaves the account untouched. Side effect of the same move: the first network call isbalance(), so a broken API key no longer costs the user a password prompt first.A review pass on this branch found the renamed guard test had been weakened —
events.index(...)comparison alone still passes if the unlock is wrapped in a spinner of its own, which is the exact failure #99 fixed._spy_ui_loadnow records spinner exits too and the test asserts no spinner is open when the prompt goes up. Verified by mutation: wrapping the unlock inui.load("Unlocking coldkey", ...)fails the test, and passed it before the change.