Skip to content

Clarify account transfer readiness#473

Open
jtc268 wants to merge 1 commit into
ramimbo:mainfrom
jtc268:codex/b406-account-status-193942
Open

Clarify account transfer readiness#473
jtc268 wants to merge 1 commit into
ramimbo:mainfrom
jtc268:codex/b406-account-status-193942

Conversation

@jtc268
Copy link
Copy Markdown

@jtc268 jtc268 commented May 26, 2026

Summary

  • distinguish registered-wallet transfer readiness from unknown accounts in account API responses
  • add regressions for arbitrary unknown accounts and unregistered mrwk1 addresses

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 exists was false.

Validation

  • uv run --extra dev python -m pytest tests/test_account_routes.py -q
  • uv 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 -q
  • uv run --extra dev ruff check app/accounts.py tests/test_account_routes.py
  • uv run --extra dev ruff format --check app/accounts.py tests/test_account_routes.py
  • uv run --extra dev python -m mypy app
  • git diff --check

Summary by CodeRabbit

  • Improvements
    • Account transfer eligibility status now provides more accurate and contextual messages based on registration state, improving clarity for users attempting transfers.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1dfeab76-0725-4fcc-ba57-289d5ff3928a

📥 Commits

Reviewing files that changed from the base of the PR and between d8532d4 and b1b5c2f.

📒 Files selected for processing (2)
  • app/accounts.py
  • tests/test_account_routes.py

📝 Walkthrough

Walkthrough

This PR adds account existence awareness to transfer status determination. The account_transfer_status function now accepts an exists parameter to conditionally message MRWK transfer eligibility based on registration state. The account_api_context function computes this flag from account lookup and threads it through the API response. Tests cover both unknown and unregistered wallet account cases.

Changes

Account transfer status existence flag

Layer / File(s) Summary
Transfer status contract and conditional messaging
app/accounts.py
account_transfer_status accepts an exists keyword-only parameter and varies the MRWK transfer eligibility message based on whether the account is registered in the database.
API context integration
app/accounts.py
account_api_context computes exists from the account database lookup and passes it to account_transfer_status, making the API response's transfer_status conditional on account existence.
Transfer status tests
tests/test_account_routes.py
Tests verify that unknown accounts and unregistered wallet addresses return exists: false with exact expected transfer status message strings.

Possibly related PRs

  • ramimbo/mergework#388: Directly implements the exists flag logic in account_transfer_status and account_api_context that this PR builds on.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Clarify account transfer readiness' directly names the changed surface (account transfer status logic) and accurately reflects the main change in the pull request.
Description check ✅ Passed The description covers the summary, root cause, and comprehensive validation steps matching the template's intent, though it omits some optional template sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR contains no investment, price, cash-out, payout, or private security claims. All transfer status messages describe MRWK functionality neutrally without prohibited artifacts.
Bounty Pr Focus ✅ Passed Branch name contains bounty identifier (193942). Diff matches stated files. Two focused regression tests validate the exists flag fix. No unrelated scope creep.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@adliebe adliebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mrwk1aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa without calling register_wallet
  • GET /api/v1/accounts/<address> returns exists: true and the registered-wallet-ready transfer status
  • GET /api/v1/wallets/<address> returns 404 / 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.

Copy link
Copy Markdown
Contributor

@Karry2019web Karry2019web left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #473: Clarify account transfer readiness

Code change is correct and well-structured:

  1. accounts.py change (app/accounts.py): The account_transfer_status function correctly gains an exists keyword-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)
    • mrwk1 accounts → 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> or mrwk1 format
  2. caller change (account_api_context): exists=account_row is not None is the correct integration — simple boolean derived from the DB lookup result.

  3. Test coverage:

    • test_unknown_account_transfer_status_does_not_report_wallet_ready — covers the fallthrough case (arbitrary string like not-a-wallet)
    • test_unregistered_wallet_transfer_status_requires_registration — covers the unregistered mrwk1 address 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants