Feat/issue 322 portfolio optimization - #328
Merged
robertocarlous merged 3 commits intoAug 17, 2026
Merged
Conversation
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #322
feat: Portfolio Optimization & Optimal Allocation Suggestion Engine (#322)
Summary
Adds an engine that computes an optimal portfolio allocation from historical protocol APY data, per-protocol risk scores, and the user's risk tolerance — returned with an efficient frontier and a backtest of its simulated consequences, as a one-tap suggestion. The suggestion is advisory: it is persisted and audited, but it is structurally incapable of mutating
User.strategyConfig.Changes Made
New Module —
src/analytics/optimizer.ts(new, 812 lines)maximize μᵀw − (λ/2)·wᵀΣwsubject toΣw = 1,lo_i ≤ w_i ≤ hi_i, and an optional stable-group floorΣ_{i∈S} w_i ≥ fw_i(θ) = clamp(v_i − θ, lo_i, hi_i)is monotone in θ, so bisection converges to machine precision with no tolerance to tune[5, 500]; efficient-frontier sweep, default 12 points, hard max 25toPercentageAllocations— the single fraction→percent boundary for the whole packageNew Module —
src/analytics/estimation.ts(new, 316 lines)ProtocolRatehistory(protocol, UTC day)—ProtocolRateis keyed by(protocolName, assetSymbol, network, fetchedAt), so without this the series would depend on scan orderingbuildDailyRateSeries, then keeping only days on which every admitted protocol has a value(j,i)entry is assigned from the(i,j)computation, so float associativity cannot produce a matrix that fails a PSD assertion at the 1e-18 level)periodReturns(see Design Decisions)New Module —
src/analytics/service.ts(new, 497 lines)suggestAllocation(userId): effective-ceiling resolution → estimation → optimization → backtest → persistencesha256:-prefixed input-snapshot hash, numbers fixed to 9 dpNew Module —
src/analytics/types.ts(new, 248 lines)Shared types and the units contract for the package. Zero imports — a pure type module.
New Module —
src/utils/concurrency.ts(new, 102 lines)ConcurrencyLimiter— per-key + global in-flight semaphore, non-blocking, idempotent release, no per-key map growth. The repo's first. Bounds a different thing from the rate limiter (see Design Decisions).New Job —
src/jobs/allocationSuggestions.ts(new, 137 lines)6-hour precompute over users with an ACTIVE
Position. Standard observability quartet (logBackgroundJob+recordBackgroundJob+recordJobSuccess/recordJobFailure), correlation-ID scope, batched with serial await, per-user failure isolation, errors logged and never rethrown.New Validators —
src/validators/allocation-validators.ts(new, 100 lines)Whole-request Zod schemas matching the file they mount in.
.strict()on the body so a typo'd knob is a 400 rather than a confidently-wrong answer computed over the wrong window. Every numeric bound is also a CPU bound, expressed with the optimizer's own exported constants rather than duplicated magic numbers.Migration —
prisma/migrations/20260817000000_add_allocation_suggestions/(new, 25 + 25 lines)allocation_suggestionstable, two indexes (userId, computedAtanduserId, inputHash), FK touserswithON DELETE CASCADE. Hand-writtenrollback.sqlalongside, per the CI gate, documenting that this is advisory data only — no funds, keys, or agent behaviour depend on it.Handlers —
src/routes/portfolio.ts(+133)POST /:userId/suggest-allocationandGET /:userId/suggestions, bothrequireAuth → enforceUserAccess → validate(...)GET /:userId, following therouter.use('/goals', …)precedentConcurrencyLimiter; 429 withRetry-After; slot released infinallyso a throw cannot permanently wedge a user out of the endpointShared —
src/agent/strategyMetrics.ts(+11 / −4)Exports
meanandsampleStdev, previously module-private, so estimation reuses them. That file declares itself the one definition of risk-adjusted return math in the codebase; a private copy insrc/analytics/would have been the third.Schema —
prisma/schema.prisma(+47 real)AllocationSuggestionmodel +allocationSuggestionsrelation onUser. The raw diff shows ±215 lines;--ignore-all-spaceshows 47 added / 0 removed — the remainder isprisma formatcolumn reflow from running codegen, with no semantic change.Config / Middleware / Startup / Formatters
src/config/env.ts(+37) —security.optimizerRateLimitandallocationSuggestionsblocks, all optional with defaultssrc/middleware/rateLimiter.ts(+21) —optimizerRateLimitervia the existingbuildRateLimiterfactorysrc/index.ts(+26) — registers the new job andscheduleProtocolRiskScoring, which existed onmainbut was never started; two module handles; twogracefulShutdownclear blockssrc/utils/api-formatters.ts(+27) —mapAllocationSuggestionToResponse, hand-written allowlist,userIddeliberately omitted,isSuggestion: trueon every rowTests — 7 new suites, +161 tests (2,097 lines)
optimizer60 ·service28 ·estimation23 ·integration16 ·structural15 ·concurrency11 ·job8. One existing file touched:tests/integration/rateLimiter.integration.test.ts(+3) neededoptimizerRateLimitin its config mock — every limiter is constructed at module load, so a missing block fails that suite at import, not at use.Documentation
docs/PORTFOLIO_OPTIMIZATION.md(new, 393 lines),docs/openapi.yaml(+370: 2 paths, 8 schemas),ASSUMPTIONS.md(+52: entries 12–18),docs/DOCUMENTATION_INDEX.md(+1).Invariants
src/analytics/orsrc/jobs/allocationSuggestions.tswritesUser.strategyConfigorUser.rebalanceStrategy.allocationSuggestion.createis the only write in the entire packagesrc/stellar/or anything matchingwallet. No suggestion path can touch custodypublishableConfigSchema.superRefinealready enforces, so a suggestion is directly acceptable by the existing update path with no re-conversion[lo, hi]bound and any stable-group floor. Every solver iterate is a projection onto the feasible set, so even an abandoned run yields a constraint-satisfying vectorriskCeilingis fail-closed: a protocol with no known score is excluded, never given the benefit of the doubt. MirrorsapplyRiskCeilingrather than reimplementing itMath.max); an ACTIVESavingsGoaloverrides outright (??). Both mirror the agent exactlyinputHashimplies equal weights, which is what makes "did my recommendation change, or only my inputs?" answerableI1 and I2 are enforced by
tests/unit/analytics/structural.test.ts, which scans source text and fails on anyuser.update/upsert/delete/create, anystrategyConfig:write outside aselect, any forbidden import, and any Prisma model access outside a fixed allowlist. It uses the stricter specifier-parsing form fromstrategy-follow.integration.test.ts— assertingimports.length > 0— so a regex that silently stops matching cannot pass vacuously. Comments are stripped first, so the files can document exactly what is banned without tripping their own assertions.Failure Modes and Protections
allocationSuggestion.createis the only permitted write (I1)non_convergedstill carries a constraint-satisfying vectorcheckFeasibilitynaming the binding constraint, run before any solvinginsufficient_universewith per-protocol exclusion reasons andbindingConstraint: riskCeiling— never a ceiling-violating vectorperiodReturnsdeliberately avoidedYieldSnapshot.apyunderstating volatilityProtocolRate.supplyApyonly — the trap documented inSTRATEGY_MARKETPLACE.md§2req.params.userId, which is what keepsenforceUserAccesseffective; a body-only or/suggestions/:idroute would make it a silent no-opConcurrencyLimiterper-key bound of 1, non-blocking 429optimizerRateLimiter(5/min) — the two bound different thingsfinally; asserted by a test that a 404 is followed by a successful 200.strict()Zod body with bounds drawn from the optimizer's own constants; frontier hard-capped at 25ProtocolRiskScoresilently disabling all ceiling-constrained rebalancingscheduleProtocolRiskScoringwired up and started before the suggestion jobResponse Contract (clients need these)
weightsare PERCENTAGES, not fractions, and sum to 100 ± 0.01 — directly acceptable by the existing strategy update endpoint with no conversion. Everything else in the payload (expectedReturn,expectedVolatility, frontierrisk/return) is a decimal fraction:0.082means 8.2 % APY,0.014means 1.4 percentage points of APY volatility.All four outcome statuses return HTTP 200. "The optimizer ran and could not produce a portfolio" is a result, not a request error:
statusokinfeasiblebindingConstraintnames whichinsufficient_universenon_convergedTwo caveats ship inside the payload, not just in the docs, because the chart would otherwise be misread:
backtest.caveat— the agent holds one protocol at a time, so the comparison is what the agent would have done under each configuration. It is not a simulation of holding the weighted basket.backtest.currentisnullwhen the user has no allocation configured; an invented baseline would be worse than none.disclaimer— Σ measures APY co-movement, not capital risk.ProtocolRateis a yield-quote series, so the optimizer minimizes yield volatility; it does not model principal loss, depeg, or smart-contract failure. Those enter only throughProtocolRiskScoreas a universe filter.ceilingSource(goal|follow|own|none) reports which layer supplied the effective ceiling, so the resolution is visible rather than inferred.Breaking Changes
None. Both endpoints are new, no existing response shape changed, no existing column changed, and no dependency was added or removed.
The
AllocationSuggestiontable is additive with a nullable-safeON DELETE CASCADEFK. The only non-additive change anywhere issrc/agent/strategyMetrics.tswidening two functions from module-private to exported, which cannot break a caller.How Has This Been Tested?
npm testnpm run lintnpm run format:checknpm run buildnpm run typecheckscripts/check-migration-rollback.shmainbefore this branch)redocly lintredocly bundle$refs resolvenpm audit --audit-level=highlicense-checkerAcceptance criteria, mapped to tests:
weights sum to 1 and respect every bound/sums to 100 within the ±0.01 tolerance— property-style over a seeded mulberry32 generator, 10 seeds × 20 problems, plus 300 randomized percentage conversionsexplicit min/max bounds are always respectedandthe stablecoin floor is never violated— 10 seeds × 15 problems each, floor asserted to1e-9higher riskTolerance gives weakly higher expected return and volatility— monotonicity across the full 1–10 rangeis non-decreasing in risk and in return/every frontier point is itself a valid portfolio— a point that took on risk for less return would mean the sweep is not tracing an efficient frontierproduces byte-identical output for the same input twiceandis invariant to the caller ordering the protocols differently— determinism (I7)a follow can never LOOSEN a tighter own ceiling/an active goal OVERRIDES outright, even to loosen— the two merge rules (I6)NEVER writes to the user record+ the wholestructural.test.tssuite — the advisory invariant (I1, I2)429s the second concurrent optimization for the same user— holds the first request inside the service by stalling a DB read, fires the second, asserts 429 +Retry-After, then releases and asserts the first still completes 200releases the concurrency slot after a failure— a leaked slot would wedge a user out permanently with nothing loggedonly counts days on which EVERY protocol has a value/forward-fills gaps so a gappy protocol stays index-aligned— the silent-corruption pathone user failure does not abort the batch— asserts the third user is still attempted after the second throwsVerified by hand beyond the assertions: the optimizer was re-run end-to-end on realistic 90-day data to confirm behaviour, not just passing assertions. As
riskTolerancerises 1 → 10, expected return climbs 7.42 % → 10.31 %, volatility climbs 0.697 pp → 1.573 pp, and the weighted risk score falls 70.6 → 65.2 — the conservative profile diversifies into the safest protocol (score 88), the aggressive one concentrates in the high-yield pair. Weights summed to exactly 100 at every level and output was byte-identical on repeat.Measured cost (full solve + 12-point frontier sweep): 0.11 ms at 3 protocols, 1.33 ms at 10, 7.10 ms at 25. Across 450 seeded problems: 0 non-converged, median 104 iterations, max 802 of a 2000 budget.
Pre-existing Bugs Found — NOT Fixed Here
1.
npm run devandnpm startboth point at a stale demo file.src/index.tsis the real application, but"dev": "nodemon --exec ts-node src/app.ts"runssrc/app.ts— a standalone CORS demo with its own express app, its ownlisten, and hard-coded/api/datahandlers that nothing imports. Worse,"start": "node dist/src/app.js"targets a path the build never produces:tsconfig.build.jsonsetsrootDir: ./src, so the output isdist/index.js. Verified both:dist/src/app.jsdoes not exist afternpm run build. The Dockerfile is correct and authoritative. Fixing the scripts is a one-line change but touches how everyone runs the project locally, so it deserves its own PR.2.
src/routes/backtest.tsis dead code masquerading as a route file. It is a near-identical stale copy ofsrc/agent/backtest.ts, differing only in import paths and a missing BigInt fix for the1e21exponential-notation bug. Verified nothing imports it and nothing mounts it. Deleting it is safe but out of scope here.3.
enforceUserAccessreturns 401, not 403, for a cross-user target.authenticate.ts:145answersAUTH_ERRORS.UNAUTHORIZEDwhenreq.auth.userId !== targetUserId— but the caller is authenticated; they are simply not authorised, which is a 403. This PR's tests assert 401, the behaviour that actually exists, rather than the behaviour one might assume. Changing it is a cross-cutting API contract change affecting every owner-scoped route.4.
enforceUserAccesssilently no-ops when neitherreq.params.userIdnorreq.body.userIdis present. Already documented inCLAUDE.mdas a known trap. Not a new bug, but it is the direct reason both new routes are keyed onreq.params.userId— noted so the keying is not "simplified" later.5.
docs/openapi.yamlgains one newno-ambiguous-pathswarning, between/portfolio/{userId}/suggest-allocationand/portfolio/goals/{id}— the same spec-level ambiguity the pre-existing/transactions/*pair already has. It resolves correctly at runtime becauserouter.use('/goals', …)is mounted before/:userId. Fixing it properly means restructuring the/goalsmount, which is a separate change.Design Decisions
Σ is the covariance of annual rate levels, and λ is log-spaced over
[5, 500]. This deviates from the issue plan and is one decision, not two. Under the plan'sΣ = cov(apy/100/365.25) × 365.25withλ ∈ [1,25], the risk term is 5–6 orders of magnitude below the return term:(λ/2)·wᵀΣwat w=1The consequence is not "slightly aggressive" — the optimum is always the max-return corner and the frontier collapses to a point. The chosen Σ is exactly the plan's matrix × 365.25, which also makes
expectedVolatilityread in the same units as the APY, so "8.2 % expected return, 1.4 % expected volatility" is directly legible. Log spacing is the natural parameterization for a risk-aversion coefficient — each step is a constant ratio of risk appetite.A default 60 % per-protocol concentration cap, applied as an explicit constraint. Even after rescaling, unconstrained mean-variance is corner-seeking (Michaud's "error maximization" — it treats a historical mean as if known exactly). Measured on this repo's scale, every
riskTolerancefrom 3 to 10 returned a single protocol at 100 %. That is a true optimum of the stated objective and simultaneously terrible advice for an endpoint whose purpose is to suggest a diversified allocation. Applied as an overridablebounds.maxdefault, raised to1/non small universes so the guardrail can never empty the feasible set — rather than quietly distorting μ or λ until the answer looked reasonable.Exact split projection instead of Dykstra. The plan called for Dykstra's alternating projections for the stable-group floor. For a convex set ∩ halfspace, the projection is either the plain capped-simplex projection or it lies on
Σ_S w = f, which is two capped simplices over disjoint index groups — and Euclidean distance separates across disjoint coordinates. So the split is exact and non-iterative. This is not merely tidier: the Dykstra draft's final re-projection pushed the iterate back out of the floor halfspace, returning 0.30 for a 0.60 floor.Nesterov acceleration is load-bearing, not an optimization. Σ is a sample covariance over ~90 observations, so with many protocols — or protocols whose APYs move together, which is the norm in DeFi — it is ill-conditioned or rank-deficient. The objective is then concave but not strongly concave, and plain projected gradient converges at O(1/k). A 7-protocol problem still had a 5.2e-8 residual after the full 2000-iteration budget and reported
non_convergedwhile sitting on a perfectly good allocation; reaching 1e-10 that way needs ~10⁶ iterations.buildDailyRateSeries, deliberately notperiodReturns. A covariance matrix requires index-aligned vectors — entry(i,j)must pair protocolsiandjon the same day.periodReturnsskips any interval whose starting value is non-positive, which is correct for its own job (a portfolio funded from empty is a deposit, not a return) and fatal here: two protocols skipping different intervals produce vectors of different lengths whose k-th entries are different days. The resulting matrix looks perfectly well-formed and is silently, badly wrong.weightsstored asJsonpercentages, notDecimal. A deliberate deviation from the issue's "use Decimal for storage". The vector's entire purpose is to round-trip intoUser.strategyConfig.targetAllocations, itself aJsonmap of numbers validated bypublishableConfigSchema;Decimalwould force a lossy re-conversion at the one place the value is ever consumed.PublishedStrategyMetricalready sets the Float-for-computed-analytics precedent.The rate limiter is applied per-endpoint, not through the
apiRoutestable. The table convention is right for a resource whose routes are uniformly costly, but/portfoliois mostly cheap reads that must not inherit a 5/min budget because one POST on the same router is expensive. This is not the double-application the warning insrc/routes/admin.tsguards against — no table-level limiter applies to this route.A concurrency semaphore in addition to the rate limiter, because they bound different things. A rate limiter bounds requests per window; it says nothing about how many run at once. The optimizer is the first genuinely CPU-bound thing in this API and runs on the single event-loop thread, so ten concurrent solves do not merely run slower — they block every other request in the process, including
/health/ready. Non-blocking by design: queueing would convert a CPU problem into a latency-and-memory problem and hold sockets open behind work the client has probably abandoned.The scheduled job stores suggestions without backtest legs. They are two full historical replays per user and are only interesting when a human is looking — which is exactly when the POST endpoint computes them live. Job rows carry a null
backtest, documented so an absent comparison never reads as a failure.The two ceiling merge rules were mirrored, not unified. A follow merges via
Math.max(may only tighten); an ACTIVE goal overrides via??(may loosen). Unifying them into one rule would be tidier and wrong — a suggestion computed under a third rule would be advice about a portfolio the agent will never build.Checklist
npm run lint,npm run format:check)docs/PORTFOLIO_OPTIMIZATION.mdnew;docs/openapi.yaml+ASSUMPTIONS.md+docs/DOCUMENTATION_INDEX.mdupdated in the same change, per the repo's route-change rule)npm run lintclean;redoclygains 1 pre-existing-category warning, noted above)main— clean descendant of96a47e8; one add/add conflict onadd_sub_accounts/rollback.sqlresolved by keepingmain's SQL byte-identical (verified with comments stripped) while preserving an operator-safety headermigration-smokeneeds a live Postgres; see the warning under How Has This Been Tested?)