Extract wallet view helpers from app.main#370
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces reusable wallet view context helpers (list/detail) and refactors the FastAPI app to consume them, with accompanying tests and a hardening update to _safe_next_path.
Changes:
- Added
app/wallet_views.pywithwallet_list_context,wallet_detail_context, and related query helpers. - Refactored
app/main.pywallet HTML routes and wallet address normalization to use the new helpers. - Added tests for wallet view contexts and wallet address/path error handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_wallet_views.py | Adds coverage for wallet list/detail context serialization and error cases |
| app/wallet_views.py | New module encapsulating wallet list/detail context creation and related queries |
| app/main.py | Uses new wallet view helpers; strengthens next-path validation by checking decoded content |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| register_wallet(session, public_key_hex=public_key_hex, label="Funded wallet") | ||
| entry = add_ledger_entry( | ||
| session, | ||
| entry_type="test_funding", | ||
| from_account=TREASURY_ACCOUNT, | ||
| to_account=address, | ||
| amount_microunits=2_500_000, | ||
| reference="test funding", | ||
| ) | ||
| session.add( | ||
| Proof( | ||
| hash=proof_hash, | ||
| ledger_sequence=entry.sequence, | ||
| bounty_id=None, | ||
| submission_id=None, | ||
| kind="test_proof", | ||
| public_json="{}", | ||
| ) | ||
| ) | ||
|
|
||
| context = wallet_detail_context(sqlite_url, address.upper()) | ||
|
|
||
| assert context["wallet"]["address"] == address | ||
| assert context["wallet"]["balance_mrwk"] == "2.5" | ||
| assert context["transactions"][0]["sequence"] == entry.sequence | ||
| assert context["transactions"][0]["proof_hash"] == proof_hash |
AugmentSecurity
left a comment
AugmentSecurity
left a comment
There was a problem hiding this comment.
Reviewed PR #370 for the MRWK #340 review bounty.
Evidence checked:
- Inspected
app/wallet_views.py,app/main.py, andtests/test_wallet_views.pyat head608ed9e9f22dc86d9309f7f2a7be51a82f56d092. - Verified wallet list rendering now delegates to
wallet_list_context()while preserving newest-first wallet ordering, serialization, labels, and balances. - Verified wallet detail rendering now delegates to
wallet_detail_context(), keeps wallet address normalization throughwallet_address_from_path(), returns 404 for missing wallets, and preserves transaction rows with proof hashes viaproof_hashes_by_sequence(). - Confirmed the API/MCP wallet-address paths now reuse the same wallet address validator without changing the existing response shape.
- Confirmed the OAuth decoded-backslash
nextsafety regression remains covered.
Local validation:
./.venv/bin/python -m pytest tests/test_wallet_views.py tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 5 passed../.venv/bin/python -m pytest -q-> 338 passed../.venv/bin/python -m ruff check app/main.py app/wallet_views.py tests/test_wallet_views.py-> passed../.venv/bin/python -m ruff format --check app/main.py app/wallet_views.py tests/test_wallet_views.py-> passed../.venv/bin/python -m mypy app/main.py app/wallet_views.py-> passed.git diff --check origin/main...HEAD-> clean.
No blockers found.
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers from my review of current head 608ed9e.
Evidence checked:
- Inspected
app/wallet_views.py,app/main.py, andtests/test_wallet_views.pyagainstorigin/main. - Verified wallet address normalization moved through
wallet_address_from_path()still raisesHTTPException(400)on invalid addresses and is used by/api/v1/wallets/{address},/wallets/{address},_normalized_account(), and MCPget_wallet. - Verified
wallet_detail_context()preserves the old ledger query semantics: it selects both incoming/outgoingLedgerEntryrows for the normalized wallet, keepssequence descordering/limit, and still attaches proof hashes by ledger sequence. - Verified page/API regression coverage exercises list serialization, detail transactions with proof hashes, invalid wallet path handling, and missing-wallet 404 behavior.
Validation run locally:
.venv/bin/python -m pytest tests/test_wallet_views.py tests/test_bounty_pages.py tests/test_wallet_api.py -q-> 39 passed.venv/bin/python -m mypy app/main.py app/wallet_views.py-> success
Verdict: comment-only / no blockers. This is a focused extraction with behavior covered by the new helper tests plus existing wallet/page tests.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
|
Closing this stale bounty submission. It still references a closed/filled bounty round and is dirty or unstable against current main after the route/test extraction batches. A future attempt needs a fresh PR against current main, a currently open bounty reference, and scope distinct from work already accepted. |
Refs #320
Summary
app.wallet_views./walletsand/wallets/{address}route handlers inapp.mainas thin template renderers.Complexity reduced
app.mainno longer owns wallet page query composition or wallet route address normalization. Wallet HTML view data can now be reviewed and tested independently from the broader app wiring, API routes, MCP dispatch, and admin flows.Tests
python -m pytest tests\test_wallet_views.py tests\test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows tests\test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -qpython -m pytest -qpython -m ruff check .git diff --check