Extract wallet API route registration#348
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMoves wallet-related REST endpoint implementations from app/main.py into a new app/wallet_api.py module and registers those routes from app/main.py; tests updated to cover transfer replay rejection and OpenAPI visibility. ChangesWallet API Route Consolidation
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_wallet_api.py (1)
267-278: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a replay regression for the extracted signed routes.
This coverage update verifies registration, but it still leaves replay protection untested after the route move. Please add a submit-twice assertion for
/api/v1/transfers(same signed body and nonce, second call rejected) so the extraction keeps nonce replay enforcement under test.Proposed test shape
+def test_wallet_transfer_api_rejects_replay(sqlite_url: str) -> None: + create_schema(sqlite_url) + sender_key, sender_public, sender_address = _keypair() + _, receiver_public, receiver_address = _keypair() + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + _register_wallet(client, sender_public) + _register_wallet(client, receiver_public) + _fund_wallet(sqlite_url, sender_address) + + payload = { + "type": "mrwk_transfer_v1", + "from_address": sender_address, + "to_address": receiver_address, + "amount_microunits": 1_000_000, + "nonce": 1, + "memo": "", + } + body = { + "from_address": sender_address, + "to_address": receiver_address, + "amount_mrwk": "1", + "nonce": 1, + "memo": "", + "signature_hex": _sign(sender_key, payload), + } + + first = client.post("/api/v1/transfers", json=body) + second = client.post("/api/v1/transfers", json=body) + + assert first.status_code == 200 + assert second.status_code == 400 + assert second.json()["detail"] == "invalid nonce"As per coding guidelines,
tests/**/*.py: Add or update tests for changed behavior;{app/wallets.py,tests/**/*wallet*}: Wallet changes require signature, nonce, replay, and spend tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_wallet_api.py` around lines 267 - 278, Update the test_wallet_method_boundary_routes_are_hidden_from_openapi test to include a replay-regression: construct a valid signed transfer request (use the same signing helper/logic used elsewhere in tests) and POST it to "/api/v1/transfers" twice using the same body and nonce; assert the first response is accepted (2xx) and the second response is rejected due to nonce replay (expect 4xx and/or a specific replay error). Locate signing helpers and the TestClient/create_app usage in this test to reuse the same signing/nonce generation so the second submission reuses the identical nonce.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_wallet_api.py`:
- Around line 267-278: Update the
test_wallet_method_boundary_routes_are_hidden_from_openapi test to include a
replay-regression: construct a valid signed transfer request (use the same
signing helper/logic used elsewhere in tests) and POST it to "/api/v1/transfers"
twice using the same body and nonce; assert the first response is accepted (2xx)
and the second response is rejected due to nonce replay (expect 4xx and/or a
specific replay error). Locate signing helpers and the TestClient/create_app
usage in this test to reuse the same signing/nonce generation so the second
submission reuses the identical nonce.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0db98a8b-a58e-482d-8d4e-4b292bc3b04e
📒 Files selected for processing (3)
app/main.pyapp/wallet_api.pytests/test_wallet_api.py
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Reviewed the wallet API route extraction for Bounty #320.\n\nWhat I checked:\n- app/wallet_api.py: moved wallet registration, wallet lookup, GitHub wallet linking, GitHub balance claim, and signed wallet transfer routes behind register_wallet_api_routes().\n- app/main.py: now wires that route module with the same dependencies for GitHub login, JSON body parsing, string/int validation, wallet normalization, and post-only redirects.\n- tests/test_wallet_api.py and tests/test_wallets.py: existing API and service coverage still exercises registration, lookup, transfer validation, link/claim auth boundaries, nonce handling, and signed transfer replay rejection.\n- CodeRabbit's replay-regression suggestion is useful extra route-level coverage, but I do not see a behavioral blocker in this extraction because the moved /api/v1/transfers handler still delegates to the same submit_wallet_transfer() service path, and the nonce/replay service regression remains green.\n\nValidation run locally on current head 8f5f205:\n- uv run --extra dev python -m pytest tests/test_wallet_api.py tests/test_wallets.py tests/test_security.py -q -> 94 passed.\n- uv run --extra dev python -m pytest -q -> 332 passed.\n- uv run --extra dev ruff check app/main.py app/wallet_api.py tests/test_wallet_api.py -> passed.\n- uv run --extra dev ruff format --check app/main.py app/wallet_api.py tests/test_wallet_api.py -> passed.\n- uv run --extra dev python -m mypy app/main.py app/wallet_api.py -> passed.\n- uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.\n- git diff --check origin/main...HEAD -> clean.\n\nNo blocker found. GitHub reports mergeState CLEAN, hosted quality is green, and CodeRabbit status is green.
s0584273828-ctrl
force-pushed
the
codex/main-code-health-320
branch
from
8f5f205 to
866a250
Compare
|
Updated after the CodeRabbit replay-coverage note. Change added:
Validation after the update:
|
TUPM96
left a comment
TUPM96
left a comment
There was a problem hiding this comment.
Reviewed the wallet API extraction for #320/#340.
Evidence checked:
- Inspected
app/wallet_api.pyroute registration for wallet registration, lookup, GitHub link, GitHub claim, and signed transfer submission. - Checked
app/main.pynow delegates wallet API route wiring throughregister_wallet_api_routeswhile keeping page routes and MCP wallet tools separate. - Checked
tests/test_wallet_api.pyadditions for replayed signed transfer rejection and OpenAPI/method-boundary coverage.
Commands run locally on PR head 866a250:
python -m pytest tests\test_wallet_api.py::test_wallet_api_register_lookup_and_transfer tests\test_wallet_api.py::test_wallet_transfer_api_rejects_replayed_signed_body tests\test_wallet_api.py::test_wallet_transfer_api_rejects_invalid_requests tests\test_wallet_api.py::test_wallet_method_boundary_routes_are_hidden_from_openapi tests\test_wallet_api.py::test_wallet_api_malformed_transfer_requests_return_4xx tests\test_wallet_api.py::test_wallet_api_malformed_link_and_claim_requests_return_4xx tests\test_wallet_api.py::test_wallet_link_and_claim_require_github_login tests\test_wallet_api.py::test_github_session_can_link_and_claim_wallet -q-> 11 passedpython -m ruff check app\main.py app\wallet_api.py tests\test_wallet_api.py-> passedgit diff --check origin/main...HEAD-> passed
I also tried python -m pytest tests\test_wallet_api.py -q; it currently fails only on test_github_login_stores_safe_default_for_backslash_next, which exercises OAuth next-path handling outside the wallet API extraction. I did not find a blocker in the changed wallet route boundary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_wallet_api.py`:
- Around line 309-311: The current tests only verify presence of allowed HTTP
methods for OpenAPI routes but not that disallowed methods are absent; update
the assertions that reference paths (the paths dict) and the specific endpoints
"/api/v1/wallets/{address}", "/api/v1/github/claim", and "/api/v1/transfers" to
assert exclusivity instead of mere presence — e.g. compare the set of keys for
paths["/api/v1/wallets/{address}"] to {"get"} and for the other two endpoints to
{"post"} (or explicitly assert that unwanted methods like "put","patch","delete"
are not in paths[...].keys()), so the test fails if extra methods appear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c15ffbcf-b211-480d-a63f-21919659b745
📒 Files selected for processing (3)
app/main.pyapp/wallet_api.pytests/test_wallet_api.py
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
Follow-up review on current head 866a250.
Good: the wallet API extraction is wired cleanly through register_wallet_api_routes(), and the moved register, lookup, link, claim, and transfer handlers still delegate to the same ledger/wallet service boundaries. The new replay regression also covers the transferred /api/v1/transfers route directly.
One test-hardening item remains: test_wallet_method_boundary_routes_are_hidden_from_openapi now asserts that /api/v1/wallets/{address}, /api/v1/github/claim, and /api/v1/transfers expose the expected method, but only by checking presence. It would still pass if those schema-visible routes accidentally exposed extra methods later. Since this test is specifically guarding the method boundary after moving route registration, please assert exact method sets for the newly added routes, e.g. set(paths["/api/v1/wallets/{address}"]) == {"get"} and set(paths["/api/v1/github/claim"]) == {"post"}, set(paths["/api/v1/transfers"]) == {"post"}. The existing register/link-github checks already prove the hidden GET proxies stay out of OpenAPI.
Validation:
python -m pytest tests/test_wallet_api.py::test_wallet_transfer_api_rejects_replayed_signed_body tests/test_wallet_api.py::test_wallet_method_boundary_routes_are_hidden_from_openapi tests/test_wallet_api.py::test_wallet_api_register_lookup_and_transfer -q-> 3 passed.python -m pytest tests/test_wallet_api.py -q-> 31 passed.python -m ruff check app/wallet_api.py app/main.py tests/test_wallet_api.py-> passed.python -m ruff format --check app/wallet_api.py app/main.py tests/test_wallet_api.py-> 3 files already formatted.python -m mypy app/wallet_api.py app/main.py-> success.git diff --check origin/main...HEAD-> clean.
No runtime blocker found in the route extraction, but the OpenAPI method-boundary regression should be tightened so it protects the behavior it names.
s0584273828-ctrl
force-pushed
the
codex/main-code-health-320
branch
from
866a250 to
8158de8
Compare
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
Follow-up review on current head 8158de8: the wallet method-boundary hardening item is addressed, and I do not see a blocker.
Evidence checked:
- Inspected
app/wallet_api.py,app/main.py, andtests/test_wallet_api.py. - Verified
register_wallet_api_routes()keeps wallet registration, lookup, GitHub linking, GitHub claim, and signed transfer submission behind the wallet API boundary whileapp.mainonly wires shared dependencies. - Rechecked the prior OpenAPI method-boundary concern:
test_wallet_method_boundary_routes_are_hidden_from_openapinow asserts exact method sets for/api/v1/wallets/register,/api/v1/wallets/link-github,/api/v1/wallets/{address},/api/v1/github/claim, and/api/v1/transfers, so extra schema-visible methods would fail the regression. - Confirmed the route-level replay regression for
/api/v1/transfersis present and passes.
Validation:
uv run --extra dev python -m pytest tests/test_wallet_api.py::test_wallet_transfer_api_rejects_replayed_signed_body tests/test_wallet_api.py::test_wallet_method_boundary_routes_are_hidden_from_openapi tests/test_wallet_api.py::test_wallet_api_register_lookup_and_transfer -q-> 3 passed.uv run --extra dev python -m pytest tests/test_wallet_api.py -q-> 31 passed.uv run --extra dev python -m mypy app/main.py app/wallet_api.py-> success.uv run --extra dev python -m ruff check app/main.py app/wallet_api.py tests/test_wallet_api.py-> passed.uv run --extra dev python -m ruff format --check app/main.py app/wallet_api.py tests/test_wallet_api.py-> 3 files already formatted.
|
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. |
6 participants
Summary
app/main.pyinto focusedapp/wallet_api.py.app/main.pyresponsible for app wiring and shared dependencies while preserving the existing public API behavior.Complexity reduced
app/main.pyno longer owns wallet-specific request parsing, ledger error handling, wallet lookup, GitHub link/claim, and transfer response shaping for seven wallet-related routes. The wallet API can now be reviewed independently from bounty, admin, webhook, OAuth/session, MCP, ledger, and page routing code.Why this is distinct
#329 extracted MCP transport, #330 extracted public serializers, #343 extracted admin webhook helpers, #337 targets bounty-attempt routing, and #338 targets auth/session helpers. This PR targets the wallet API route boundary only.
Validation
uv run --extra dev python -m pytest tests/test_wallet_api.py -q-> 30 passed.uv run --extra dev python -m pytest -q-> 332 passed.uv run --extra dev ruff check .-> passed.uv run --extra dev ruff format --check .-> 47 files already formatted.uv run --extra dev python -m mypy app-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check-> clean.MRWK
Bounty #320
No private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests