Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
murderteeth
left a comment
There was a problem hiding this comment.
Review Summary
This PR partially addresses the bug where metadata parsing failures cause valid data to be overwritten with empty values. The current implementation handles individual item validation failures, but does not fully solve the original issue.
What Works ✅
- Individual items that fail Zod schema validation are now skipped instead of crashing the entire
extractMetasfunction - Failed items are logged for debugging
Critical Issues 🚨
1. Outer functions still return empty objects on failure
The catch blocks in getVaultMeta, getStrategyMeta, and getTokenMeta (lines 8-46) still return empty string values when errors occur:
catch(error) {
return {
displayName: '',
displaySymbol: '',
description: '',
protocols: []
}
}Per the bug report, these should return undefined so the loader can preserve existing data.
2. Loader merge logic is unchanged
The PR description mentions filtering undefined values in load/index.ts, but no changes were made to the loader. The current merge:
snapshot.hook = { ...currentHook, ...snapshot.hook }will still overwrite existing meta with empty strings when failures occur.
3. PR description doesn't match actual changes
- Description says: "return
undefinedinstead of blank objects on failure" - not implemented - Description says: "updates the snapshot ingestion loader to filter out
undefinedmeta values" - not implemented - Description references tests - but only 1 file changed
|
wut, i forgot to push the load update T.T |
When getVaultMeta fails, the new hook's meta would be empty but would still overwrite the existing meta. Now we explicitly merge meta fields so existing values are preserved when the new meta is null/undefined. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
murderteeth
left a comment
There was a problem hiding this comment.
The previous review concerns are addressed — meta functions now return undefined on failure, extractMetas handles individual parse errors gracefully, and the loader preserves existing meta when new meta is falsy.
One remaining edge case in the merge logic at packages/ingest/load/index.ts:107:
meta: snapshot.hook.meta ? { ...currentHook.meta, ...snapshot.hook.meta } : currentHook.metaWhen both getVaultMeta and getTokenMeta fail, vault hooks return meta: { token: undefined }. This object is truthy so the merge branch runs, but spreading { token: undefined } overwrites currentHook.meta.token. When pg serializes to jsonb, undefined is dropped and the existing token data is lost.
Fix — strip undefined values before merging by round-tripping through JSON (no fidelity loss since this goes straight to jsonb):
meta: snapshot.hook.meta
? { ...currentHook.meta, ...JSON.parse(JSON.stringify(snapshot.hook.meta)) }
: currentHook.meta
Summary
This PR improves the robustness of metadata fetching and ingestion. It modifies metadata fetching functions in
meta.tsto returnundefinedinstead of blank objects on failure, avoiding valid data override with empty strings. It also updates the snapshot ingestion loader to filter outundefinedmeta values, preserving existing database records when transient errors occur.How to review
Start with
packages/ingest/abis/yearn/lib/meta.tsto see the logic change (returningundefinedon catch).Then check
packages/ingest/load/index.tsto see howupsertSnapshotfiltersundefinedvalues fromsnapshot.hookbefore merging.Review
packages/ingest/abis/yearn/lib/meta.spec.tsandpackages/ingest/abis/yearn/2/strategy/snapshot/hook.spec.tsfor test coverage.Ignore local config changes if present.
Test plan
npx mocha abis/yearn/2/strategy/snapshot/hook.spec.tsverify hook process handles validation failure gracefully.