Add portfolio CRUD routes to the API router - #50
Conversation
POST /portfolio, GET /user/:address/portfolios, GET /portfolio/:id, and POST /portfolio/:id/rebalance didn't exist, so the frontend fell back to demo data on every portfolio operation. Wires up services that were already built and waiting for an HTTP layer: - POST /portfolio uses portfolioStorage.createPortfolio, validated against the existing (previously unused) createPortfolioSchema. userAddress isn't checked as a Stellar address since the frontend sends a 'demo-user' placeholder when no wallet is connected. - GET /user/:address/portfolios returns a bare array via portfolioStorage.getUserPortfolios, matching what Dashboard.tsx's fetchPortfolioData expects — not wrapped in an envelope, and not validating :address format since a malformed address on a read just naturally matches nothing. - GET /portfolio/:id uses stellarService.getPortfolio, which already computes the enriched allocations/needsRebalance/dayChange shape Dashboard.tsx renders. 404s cleanly via a portfolioStorage existence check first, rather than string-matching stellarService's wrapped error. - POST /portfolio/:id/rebalance calls stellarService.executeRebalance, which already handles risk checks, cooldown, circuit breakers and DEX execution. Only slippageOverrides is forwarded — the schema also accepts simulateOnly and ignoreSafetyChecks, but executeRebalance has no simulate-only or safety-bypass path, so forwarding them would let a caller think they asked for a dry run while a real trade still executes. Left unimplemented rather than silently mishandled. Not adding writeRateLimiter to either write route — it's a single shared limiter across every write endpoint in this router (not per-route), and adding two more high-traffic consumers to that shared budget starves unrelated endpoints under normal use, exactly as it did to the notification tests once these routes existed. Closes grantFoxin#36.
These four describe.skip blocks were written for this feature ahead of
time, but assumed a different response contract than what the live
frontend actually needs — a flat {portfolioId, status, mode: 'demo'}
envelope with error-string assertions ('Missing required fields',
'100%', 'Threshold') that predate the createPortfolioSchema/
validateRequest pair now handling validation. Matching them as-written
would mean abandoning that schema.
Rewrote the assertions against the real, frontend-verified contract
instead: {success, portfolio} envelopes, a bare array for
/user/:address/portfolios, Zod's {error, details[]} shape for
validation failures, and 404/500 status codes that match what the
routes actually return (portfolio-not-found is a clean 404 checked
before calling into the service layer; business-rule failures from
executeRebalance — cooldown, risk block, not-needed — are a generic
500 like every other service-layer failure in this router, not a
special-cased 4xx).
Left the API Health Check describe.skip alone — unrelated to this
issue, and skipped because that block's test app never mounts
healthRouter, not because of anything to do with portfolios.
|
went through this one carefully since the whole thing hinges on plumbing that already existed. checked createPortfolioSchema and rebalancePortfolioSchema directly, both were already there and match what you said, no stellar format check on userAddress, allocations required to sum to 100. also traced executeRebalance and dex.ts end to end to make sure simulateOnly and ignoreSafetyChecks really have nowhere to go downstream before trusting that you left them out on purpose rather than missed them, and that holds up. cooldown test checks out too, ran through the logic and it throws exactly what the test expects. one small thing, not a blocker: the writeRateLimiter is currently only attached to the notifications subscribe route, not shared across every write endpoint like the commit message says. doesn't change the call you made here, just the reasoning was a bit oversold. rewriting the stale skipped tests instead of leaving them skipped or matching them blindly was the right move too. good work, approved |
|
Fair catch, you're right, it's only on notifications/subscribe right now, not broadly shared like I made it sound. The mechanism still stands (one instance, one bucket per IP, so my two routes would've eaten into that same one route's budget if I'd added it), just wasn't fair to call that "every write endpoint" when it's currently just the one. Appreciate you tracing executeRebalance and dex.ts to check the simulateOnly/ignoreSafetyChecks call too. Thanks for the review. |
Follow-up to review feedback on grantFoxin#50: writeRateLimiter is currently only used by /notifications/subscribe, not shared across every write endpoint like an earlier commit message here claimed — that framing was inaccurate. But the underlying mechanism it described is real: express-rate-limit tracks one counter per limiter instance, so reusing writeRateLimiter for POST /portfolio and POST /portfolio/:id/rebalance would still put all three routes on one shared budget, which is what caused the notification test 429 during development. Rather than leave these two routes with no rate limiting at all, give them their own instance — portfolioWriteRateLimiter, configurable via RATE_LIMIT_PORTFOLIO_WRITE_MAX (defaults to 30, well above writeRateLimiter's default of 10 to leave headroom for portfolio setup flows that reasonably make several calls in a row). Own counter store, so it can't starve or be starved by notifications/subscribe.
|
Pushed a follow-up: gave POST /portfolio and POST /portfolio/:id/rebalance their own rate limiter (portfolioWriteRateLimiter) instead of leaving them with none. Own counter store, separate from writeRateLimiter, so it can't share a bucket with notifications/subscribe the way the commit message wrongly implied it already did. Configurable via RATE_LIMIT_PORTFOLIO_WRITE_MAX, defaults to 30. Didn't add a test that actually drives it past the limit — the integration suite reuses one Express app per file, so 30+ rapid POSTs to trigger a 429 would burn real budget off the same instance every other test in that file shares, which is the exact problem we're trying to avoid. Happy to add one in an isolated app instance if you'd rather have that coverage. |
|
Nice perfect LGTM |
2 participants
Closes #36
Wired up POST /portfolio, GET /user/:address/portfolios, GET /portfolio/:id, and POST /portfolio/:id/rebalance — all backed by services (portfolioStorage, stellarService) and Zod schemas that already existed and just needed an HTTP layer. userAddress isn't checked as a Stellar format since the frontend sends a "demo-user" placeholder in demo mode. GET /portfolio/:id 404s cleanly via a portfolioStorage check before calling stellarService, instead of string-matching its wrapped error.
For rebalance, only slippageOverrides gets forwarded — the schema also accepts simulateOnly and ignoreSafetyChecks, but executeRebalance has no dry-run or safety-bypass path, so forwarding those would let a caller think they got a simulation while a real trade executed. Left unimplemented on purpose.
Heads up: there were four describe.skip tests already written for these exact routes, but assuming an older response shape ({portfolioId, status, mode: 'demo'}) that predates the validation schema I used. I went with what the live frontend actually needs and rewrote those tests to match, rather than the stale assumption — flagging in case there's context I'm missing.
Didn't add writeRateLimiter to either route, it's one shared limiter across all write endpoints and was starving an unrelated notification test in my own run.
Verified locally: typecheck clean, full backend suite passes (101/104, pre-existing unrelated skips). Frontend and contract builds fail too, but confirmed both pre-exist on a clean main and are untouched by this change.