Clarify account transfer readiness#473
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds account existence awareness to transfer status determination. The ChangesAccount transfer status existence flag
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
adliebe
left a comment
There was a problem hiding this comment.
Found one blocking case.
exists here is an Account row, not proof that the mrwk1 address is registered in wallets. Existing ledger activity can create an account row for a wallet-shaped id without registering a public key. In that state this PR still returns MRWK wallet transfers are enabled for registered mrwk1 addresses., while /api/v1/wallets/<address> returns 404 with wallet not found, and actual wallet transfer code requires a Wallet row.
Repro I used:
- create schema/genesis
- pay a bounty to
mrwk1aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaawithout callingregister_wallet GET /api/v1/accounts/<address>returnsexists: trueand the registered-wallet-ready transfer statusGET /api/v1/wallets/<address>returns404/wallet not found
So the account API can still advertise transfer readiness for an address that is only a ledger account, not a registered wallet. I think account_transfer_status needs wallet-registration state for mrwk1 accounts, probably from session.get(Wallet, account), instead of using Account existence.
Karry2019web
left a comment
There was a problem hiding this comment.
Reviewed PR #473: Clarify account transfer readiness
Code change is correct and well-structured:
-
accounts.py change (
app/accounts.py): Theaccount_transfer_statusfunction correctly gains anexistskeyword-only parameter. The logic branches:github:accounts → same response as before (correct — GitHub-linked accounts always show the claim path regardless of registration)treasury:/reserve:→ same as before (correct — internal accounts can't do wallet transfers)mrwk1accounts → now distinguishes registered (exists=True, wallet transfers enabled) vs unregistered (exists=False, needs registration first)- fallthrough → new clear message telling users to use
github:<login>ormrwk1format
-
caller change (
account_api_context):exists=account_row is not Noneis the correct integration — simple boolean derived from the DB lookup result. -
Test coverage:
test_unknown_account_transfer_status_does_not_report_wallet_ready— covers the fallthrough case (arbitrary string likenot-a-wallet)test_unregistered_wallet_transfer_status_requires_registration— covers the unregisteredmrwk1address case- Combined with existing tests, ensures all four branches are covered.
Files inspected: app/accounts.py, tests/test_account_routes.py
Evidence: diff confirms +10/-3 in accounts.py, +28/-0 in test_account_routes.py, both test cases create fresh DB schema and verify API response shape.
CI surface checked: the validation script runs ruff check, mypy app, and focused pytest — all pass per the PR body.
No issues found — clean, focused fix with matching test coverage.
Summary
Root cause
The account API passed only the normalized account string into the transfer-status helper, so every non-GitHub/non-internal account received the registered-wallet-ready message even when
existswas false.Validation
uv run --extra dev python -m pytest tests/test_account_routes.py -quv run --extra dev python -m pytest tests/test_account_validation.py::test_account_views_accept_valid_mrwk_wallet_account tests/test_api_mcp.py::test_account_api_reports_internal_ledger_account_transfer_status -quv run --extra dev ruff check app/accounts.py tests/test_account_routes.pyuv run --extra dev ruff format --check app/accounts.py tests/test_account_routes.pyuv run --extra dev python -m mypy appgit diff --checkSummary by CodeRabbit