Only include netAPR and netAPY in estimated APR response when present#353
Only include netAPR and netAPY in estimated APR response when present#353matheus1lva wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Now that seems fine. |
murderteeth
left a comment
There was a problem hiding this comment.
Review
Summary
The core fix is correct — missing netAPR/netAPY should not be coerced to 0. The destructuring + conditional spread pattern in the helper is clean. Removing netAPR/netAPY from the components bag to avoid duplication is a good cleanup.
However, downstream schemas still declare apr/apy as required fields, so returning an object without them will break consumers.
Issues
1. GraphQL schema — apr: Float! / apy: Float! are non-nullable
packages/web/app/api/gql/typeDefs/vault.ts:124-125
The EstimatedApr type marks both fields as Float!. When a V3 vault has no netAPR/netAPY data and a client queries estimated { apr apy }, GraphQL will return a field-level error. This won't surface if clients omit those fields from their selection set, but any client that does select them will break.
Fix: change to apr: Float / apy: Float (nullable).
2. REST Zod schema — apr / apy are required
packages/web/app/api/rest/list/db.ts:42-43
The estimated object is .nullish() at the top level, but when present, apr: z.number() and apy: z.number() are required. The REST list endpoint reads snapshot.hook->'performance' directly from the DB (line 140), so if the stored JSON has an estimated object without apr/apy, z.array(VaultListItemSchema).parse() at line 204 will throw — failing the entire list response, not just one vault.
Fix: change to apr: z.number().optional() / apy: z.number().optional().
3. Lib types — EstimatedAprSchema inconsistency
packages/lib/types.ts:445-446
Not directly used in the V3 path (only V2 calls .parse()), but the shared type now misrepresents what V3 actually returns. Worth updating for consistency: apr: z.number().optional() / apy: z.number().optional().
Test plan feedback
The current test plan has a few gaps:
- Full index on blank DB: As written it implies running
fanout abisagainst the entire config, which is heavy. Consider scoping to a few specific vaults viaconfig/abis.local.yamlto keep the test fast and focused. - Redundant docker step:
make devalready starts Redis/Postgres — the standalonedocker start redisstep can be dropped. - Vault selection is left to the reviewer: Instead of asking the reviewer to query for vaults with/without
netAPR/netAPY, pick concrete test addresses and state what to expect. For example:- A V3 vault that has
netAPR/netAPYcomponents → expected:aprandapypresent with numeric values,netAPR/netAPYabsent fromcomponents - A V3 vault that does NOT have those components (e.g. a freshly deployed vault, or one using oracle APR only) → expected: no
apr/apykeys at all inestimated, not0
- A V3 vault that has
Nit
componentsNonAprAPY is a bit of a mouthful. A simpler name like rest or remainingComponents would read more naturally, though this is purely cosmetic.
Summary
When
netAPRornetAPYcomponents are missing from theoutputtable, the V3 estimated APR helper (getLatestEstimatedAprV3) was returningapr: 0andapy: 0via the?? 0fallback. This is misleading — a missing value is not the same as zero. Nowaprandapyare only included in the response when the corresponding component actually exists in the data.Additionally,
netAPRandnetAPYare removed from thecomponentsbag to avoid duplication, since they are already promoted to top-levelapr/apyfields.How to review
Single file change in
packages/ingest/helpers/apy-apr.ts. Review the destructuring + conditional spread pattern on lines 30–36.Test plan
In the terminal UI, select
ingest→fanout abisto trigger indexingFind a vault that has netAPR/netAPY components in the output table
apr/apyshould be present with numeric valuesExpected:
{"type":"...","apr":<number>,"apy":<number>,"components":{...}}—apr/apypresent, andnetAPR/netAPYNOT incomponentsapr/apykeys should be absent (not zero)Expected:
{"type":"...","components":{...}}— noaprorapykeysRisk / impact
Low risk. Only affects the V3 estimated APR path (
getLatestEstimatedAprV3). Consumers that rely onapr/apyalways being present will now need to handle the optional case — but this is the correct behavior since a missing value should not masquerade as zero.