fix: use maintained wallet-balance endpoint in star_checker#6789
fix: use maintained wallet-balance endpoint in star_checker#6789zeroknowledge0x wants to merge 1 commit into
Conversation
The check_wallet_exists() function was calling the stale /api/balance/<wallet> route, which can report a wallet as missing even when the node is healthy. Switch to the maintained public endpoint /wallet/balance?miner_id=<wallet> (same as verifier.py) and validate the response shape (dict with amount_i64 key) rather than treating any HTTP 200 as success. Fixes Scottcjn#6779
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
zqleslie
left a comment
There was a problem hiding this comment.
Review — PR #6789 (fix: use maintained wallet-balance endpoint in star_checker)
Scope assessment
Single-file change in tools/bounty_verifier/star_checker.py — 20 lines (16 additions, 4 deletions). Clean BCOS-L1 fix targeting the stale /api/balance/ route used by check_wallet_exists().
What the fix does
- Switches from
/api/balance/<wallet>→/wallet/balance?miner_id=<wallet>, matching the endpoint already used byverifier.py - Uses
params=dict for safe query-string encoding (prevents injection if wallet contains special chars) - Replaces "any 200 = exists" heuristic with proper JSON shape validation (
isinstance(body, dict) and "amount_i64" in body)
Technical review
Correctness ✅
The change aligns star_checker.py with verifier.py which already uses /wallet/balance?miner_id= successfully. The params= pattern is the correct requests idiom for query parameters.
Response validation ✅
Previous code treated any HTTP 200 as "wallet exists", which could false-positive on HTML error pages or maintenance responses. The new isinstance(body, dict) and "amount_i64" in body check ensures we only accept the expected JSON shape from the balance endpoint.
Edge cases considered:
- Wallet address with URL-special characters → handled by
params=encoding - Non-JSON 200 response → caught by
resp.json()raising ValueError, falls toexcept Exception - Empty dict response →
"amount_i64" not in body→ returnsFalse(correct, wallet not confirmed)
Missing regression tests ⚠️
The PR body mentions verification against verifier.py but doesn't include test coverage. A simple regression test would be valuable:
def test_check_wallet_exists_uses_correct_endpoint():
with patch("star_checker.requests.get") as mock_get:
mock_get.return_value.status_code = 200
mock_get.return_value.json.return_value = {"amount_i64": 1000}
assert check_wallet_exists("RTC_test_wallet") is True
mock_get.assert_called_once()
call_kwargs = mock_get.call_args[1]
assert "params" in call_kwargs
assert call_kwargs["params"]["miner_id"] == "RTC_test_wallet"Verdict
APPROVED — Clean, targeted fix. The endpoint change is well-justified and the response validation is strictly more robust than before. Would benefit from regression tests but doesn't block merge for this size of change.
Bounty claim: Code review bounty under #73
Reviewer: zqleslie
Wallet: XKO212dF8324b9b61F294D26A6Dc68e3f81e4BE451D
jaxint
left a comment
There was a problem hiding this comment.
Great contribution! 🔍 Reviewed and looks solid.
|
Closing as a duplicate of #6778 (FakerHideInBush), which fixed the same |
Fixes #6779
Summary
check_wallet_exists()intools/bounty_verifier/star_checker.pywas calling the stale/api/balance/<wallet>route, which can report a wallet as missing even when the node's public wallet-balance endpoint is healthy.Changes
/api/balance/<wallet>to/wallet/balance?miner_id=<wallet>(same maintained endpoint used byverifier.py)params=dict for proper query-string encodingamount_i64rather than treating any HTTP 200 as successTesting
verifier.pyalready uses/wallet/balance?miner_id=successfullycheck_wallet_exists()now returnsTrueonly when the node confirms the wallet has a balance record