Repository navigation
Conversation
…are not rejected as future (PR Twigpine#29) - fromChainlink takes injected now() and samples after await feedFn() - refresh stamps cache from completion time - regression test: slow Coinbase failure + round published mid-request returns 2441.34
feedUsd accepted updatedAt in the future (nowS - updatedAt negative never exceeds maxAgeS), so a skewed clock or bad RPC round served as a price. feedEthUsd delegates to the same helper, so ETH/USD had the same hole. Reject r.updatedAt > nowS and cover it in baseStocks + ethPrice tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change rejects future-dated feed rounds. Chainlink validation samples the clock after feed reads complete. Refresh timestamps also use completion time. Tests cover future rounds and delayed Coinbase fallback behavior. ChangesPrice validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Future-dated feed rounds are rejected, while valid rounds returned after a delayed fallback remain accepted. The change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Ayush7614 I'm closing this as a duplicate of #29. I compared the current diffs: all four files are identical, including the fallback-clock fix and regression test. Please continue in #29 so we keep the existing review history in one place. Please stop opening parallel PRs for the same change. Keep one active PR per fix, add review follow-ups as commits there, and update that branch when it needs a rebase. If a replacement is genuinely necessary, explain why, link it and close the superseded PR. Small, independent fixes are welcome. Duplicate submissions are the issue here: they consume another review cycle without adding new work. If there is a distinct change missing from #29, point it out in that thread. |
Future rounds (updatedAt > now) from a bad RPC or clock skew previously passed as a valid price, and the ETH price fallback sampled the clock before the Coinbase wait, so a round published while Coinbase was failing was rejected as future.
This PR:
baseStocks.ts:feedUsdandethPrice.ts:feedEthUsdreturn null whenupdatedAt > nowS(future) or non-positive / stale (> maxAge), coveringBASE_STOCK_MAX_FEED_AGE_SandETH_FEED_MAX_AGE_S.ethPrice.ts:fromChainlinksamplesnow()afterfeedFn()completes, so a Chainlink round published mid-fallback (Coinbase 5s timeout) is correctly aged 2s, not rejected as future.baseStocks.test.ts(future round at +60s and far-future),ethPrice.test.ts(future at +60s, plus PR fix(app): reject future-dated Chainlink rounds in feedUsd #29 regression: slow Coinbase 503 + round published 3s after start still prices at 2441.34).Verified on upstream/main base:
Summary by CodeRabbit