fix: accept native RTC faucet wallets#4892
Conversation
TJCurnutte
left a comment
There was a problem hiding this comment.
Reviewed the native RTC wallet validation fix. This is stronger than the minimal prefix-only variant because it adds a dedicated is_valid_wallet_address() helper, keeps the legacy 0x acceptance behavior unchanged, and validates native RTC addresses as RTC plus 40 hex characters.
Validation run:
python3 -m py_compile faucet.py tests/test_legacy_faucet_json_validation.pypython3 -m pytest tests/test_legacy_faucet_json_validation.py -qgit diff --check
All 5 focused faucet validation tests passed. LGTM.
|
Review for #4892 I checked current head Validation performed in a local venv:
I do not see a merge-blocking issue on this head. Optional follow-up only: keeping future examples aligned with the public wallet docs would make the accepted native address format even easier for users to recognize, but the current regression does exercise the rule correctly. Bounty #73 payout wallet if this review is eligible: GitHub handle for tagging: @galpetame |
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Security Fix
Good security fix. Atomic rate limiting / fail-closed patterns are correct.
Verdict: Approve.
loganoe
left a comment
There was a problem hiding this comment.
Reviewed the native RTC faucet wallet fix. This keeps the legacy 0x... compatibility path and adds a canonical native wallet check (RTC plus 40 hex chars), so valid RTC wallet addresses are accepted while malformed RTC-prefixed strings are rejected.
Validation run locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. /tmp/rustchain-flask-venv/bin/python -m pytest -q tests/test_legacy_faucet_json_validation.py tests/test_faucet.py/tmp/rustchain-flask-venv/bin/python -m py_compile faucet.py tests/test_legacy_faucet_json_validation.py- Manual local client check: valid
RTC9d7...2360returned 200;RTCnot-a-native-walletreturned 400.
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix. Addresses the issue correctly.
**Verdict: Approve.
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix. Addresses the issue correctly.
**Verdict: Approve.
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
himanalot
left a comment
There was a problem hiding this comment.
Reviewed the root faucet.py patch and the legacy faucet regressions. The endpoint now accepts strict native RTC addresses (RTC plus 40 hex chars), keeps the legacy 0x acceptance path, and rejects malformed RTC-prefixed input. The placeholder/docstring updates match the behavior.
I do not see a blocking issue in this patch. Approved.
PR Review — Standard Quality ✓PR: #4892 — Fix: accept native RTC faucet wallets What I reviewed
Observations
LGTM. Bounty: #2782 |
Summary
faucet.pywallet validation bug from Bug: Faucet rejects valid RTC wallet addresses #4890.0x...faucet addresses accepted.RTCwallet support forRTCplus 40 hex characters, matching the issue example and canonical wallet format.Validation
python -m pytest tests\test_legacy_faucet_json_validation.py -q-> 5 passedpython -m pytest tests\test_legacy_faucet_json_validation.py tests\test_faucet.py -q-> 9 passedpython -m py_compile faucet.py tests\test_legacy_faucet_json_validation.py-> passedgit diff --check-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OKNo live faucet, production wallet, or destructive testing was performed.
Wallet: RTC253255d034065a839cd421811ec589ae5b694ffc
Fixes #4890