feat(sdk): implement Blend and DeFindex rate sources for the migration keeper - #538
feat(sdk): implement Blend and DeFindex rate sources for the migration keeper#538OpadijoIdris wants to merge 9 commits into
Conversation
|
@IdrisBranda is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
CI is currently failing on two checks, not the code itself:
- Commit Messages: header is 77 characters, over the 72-char limit (
^(feat|fix|docs|chore|refactor|test|style|ci|perf)(\(.+\))?: .{1,72}$). Since squash merge is enforced, this also affects the PR title if it's reused as the squash message. - Lint & Typecheck:
packages/stellar-sdk-helpers/src/rate-sources.test.tsfailsprettier --check .. Runpnpm formatto fix.
Vercel is also failing, but that's the pre-existing #513 outage, unrelated to this PR.
| const loadPool = | ||
| options.loadPool ?? | ||
| ((network: StellarNetwork, poolId: string) => | ||
| PoolV2.load( |
There was a problem hiding this comment.
PoolV2.load() here has no timeout wrapper. vaults.ts's fetchBlendApy and blend.ts's fetchBlendPositions both wrap the identical call in withRaceTimeout/withRetry. Without it, a hung Blend RPC call blocks indefinitely, since withKeeperRetry's deadline check only runs between attempts, not on an in-flight call. Worth wrapping this the same way the other two call sites do.
| const growth = | ||
| Number(priceStroops - prior.priceStroops) / Number(prior.priceStroops); | ||
| const elapsedYears = elapsedMs / (SECONDS_PER_YEAR * 1000); | ||
| const apy = Math.pow(1 + growth, 1 / elapsedYears) - 1; |
There was a problem hiding this comment.
At the 10-minute MIN_SAMPLE_INTERVAL_MS floor, Math.pow(1 + growth, 1 / elapsedYears) extrapolates by roughly a 52,000x factor. A few-percent share-price move within one sample window (a single large deposit/withdrawal skewing get_asset_amounts_per_shares, not necessarily a DeFindex bug) produces a finite but absurd APY that clears toFiniteBps's only check (Number.isFinite), and can win the migration comparison outright against Blend's real rate. Worth a sanity ceiling on the computed APY, or a longer minimum interval than 10 minutes.
| }); | ||
|
|
||
| return async (query: RateQuery) => { | ||
| switch (query.protocol) { |
There was a problem hiding this comment.
.env.example states "a new protocol is a new env var, never a code change." This switch means a third protocol adapter would silently get null rates forever until this function is also patched. Worth dispatching off options/config the same config-driven way MERIDIAN_ADAPTER_<PROTOCOL>_ID already works, or at least logging when a protocol falls through to default.
|
@OpadijoIdris |
|
@OpadijoIdris checking in, no update in 3 days since the review. Let me know if you're still working on it or need help with the findings. |
Will do it now, i didnt know you dropped a comment |
Wrap the default Blend pool loader in withRaceTimeout (matches vaults.ts/blend.ts) so a hung RPC call can't block indefinitely between withKeeperRetry's deadline checks. Cap the DeFindex APY at a plausibility ceiling so a single skewed share-price sample near the 10-minute floor can't compounded-extrapolate into an absurd rate that wins the migration comparison outright. Dispatch createDefaultRateSource through a protocol registry instead of a switch, and log a warning when a configured protocol has no registered source, since adapter discovery is config-driven and can otherwise fall through to null forever, silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4728c85 to
8d77703
Compare
…er-rate-sources # Conflicts: # apps/docs/operations/migration-keeper.md
| import type { RateQuery, RateSourceFn } from "./migration-keeper"; | ||
|
|
||
| const BPS_SCALAR = 10_000; | ||
| const BLEND_RPC_TIMEOUT_MS = 10_000; |
There was a problem hiding this comment.
blend.ts already defines BLEND_RPC_TIMEOUT_MS and a withBlendTimeout wrapper around withRaceTimeout for this exact purpose. This redefines both locally instead of importing them, if the Blend RPC timeout is ever tuned, only one of the two copies is likely to get updated.
| export function createBlendRateSource( | ||
| options: BlendRateSourceOptions | ||
| ): RateSourceFn { | ||
| const assetId = options.assetId ?? APP_ADDRESSES.usdc; |
There was a problem hiding this comment.
assetId defaults from the process-wide APP_ADDRESSES singleton rather than being derived from the network parameter this function actually receives. Works today since config.network is always APP_NETWORK in practice, but any future caller constructing this with an explicit network object that differs from process.env.STELLAR_NETWORK would silently get the wrong network's USDC address, and pool.reserves.get(assetId) would just return null with no error.
| if (query.protocol !== "defindex") return null; | ||
|
|
||
| const server = getRpcServer(options.network.rpcUrl, 10_000); | ||
| const amounts = (await simulateView( |
There was a problem hiding this comment.
This simulateView(...get_asset_amounts_per_shares...) + toBigInt(amounts[0]) extraction already exists twice in defindex.ts (buildDefindexWithdrawTx, fetchDefindexPosition), this makes a third copy. Worth factoring into a shared helper so a future fix to how that response is parsed only needs to land once.
| const timestampMs = now(); | ||
|
|
||
| const key = snapshotKey(query.poolId); | ||
| const prior = await options.store.get(key); |
There was a problem hiding this comment.
The DeFindex quote (simulateView above) and this store read don't depend on each other but run sequentially. This function is on findBestCandidate's deadline-budget-constrained hot path, worth a Promise.all to run them concurrently instead of adding their latencies together.
Import blend.ts's own BLEND_RPC_TIMEOUT_MS/withBlendTimeout instead of redefining them locally. Derive the Blend assetId from the network param this function actually receives instead of the process-wide APP_ADDRESSES singleton, so a mainnet call can't silently resolve testnet's USDC (or vice versa). Factor the get_asset_amounts_per_shares simulateView + toBigInt extraction (duplicated across buildDefindexWithdrawTx, fetchDefindexPosition, and this file) into defindex.ts's getDefindexAssetAmountPerShares. Run the DeFindex price quote and the prior-snapshot store read concurrently instead of sequentially, since they're independent and this sits on findBestCandidate's deadline-budget-constrained hot path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| async set(key, snapshot) { | ||
| const value = `${snapshot.timestampMs}:${snapshot.priceStroops}`; | ||
| const res = await fetchFn( | ||
| `${restUrl}/set/${encodeURIComponent(key)}/${encodeURIComponent(value)}?EX=${ttlSeconds}`, |
There was a problem hiding this comment.
This URL isn't a documented Upstash REST API shape. Upstash documents two forms for SET key value EX ttl: the all-path-segments form (GET /set/key/value/EX/ttl) or a POST /set/key?EX=ttl with the value in the request body. This is a third, undocumented hybrid: POST with the value already in the path and EX as a query string. If Upstash rejects the combination, every .set() call throws, and since this file deliberately doesn't catch these errors (so withKeeperRetry can classify them), isTransientKeeperError's status-code check doesn't include 400, so a rejection here would be treated as a permanent failure, not a transient one, failing every DeFindex rate evaluation outright the first time Upstash is actually configured. If Upstash instead silently ignores the unrecognized combination and sets without an expiry, ttlSeconds never actually applies and old snapshots accumulate forever instead of expiring after DEFAULT_SNAPSHOT_TTL_SECONDS. rate-sources.test.ts's assertion on this only checks the URL string the code itself produces, so it wouldn't catch either failure mode. Worth switching to the documented all-path-segments form (/set/${key}/${value}/EX/${ttlSeconds}) to remove the ambiguity, and testing against a real Upstash instance before this ships.
| // run concurrently rather than sequentially: this is on | ||
| // findBestCandidate's deadline-budget-constrained hot path (see | ||
| // migration-keeper.ts), so their latencies shouldn't just add up. | ||
| const [priceStroops, prior] = await Promise.all([ |
There was a problem hiding this comment.
discoverMigrationVaults (migration-keeper.ts:344-355) has a comment explaining in detail why it deliberately uses Promise.allSettled instead of Promise.all: an orphaned, still-in-flight promise from a rejected Promise.all keeps running unobserved, and a retry re-invokes the whole function, starting a second concurrent call for the same work. This Promise.all reintroduces exactly that pattern. findBestCandidate calls this rate source inside withKeeperRetry, which re-invokes the callback on each retry attempt. If options.store.get(key) rejects (a transient Upstash 500, which .get() turns into a thrown error) while the RPC quote promise is still pending, Promise.all rejects immediately, the RPC call is not awaited or cancelled, and the retry kicks off a second concurrent Soroban RPC call for the same quote. If the orphaned call later itself rejects, that's an unhandled promise rejection in a long-running process. Same fix as the neighboring code: Promise.allSettled.
|
|
||
| return { | ||
| async get(key) { | ||
| const res = await fetchFn(`${restUrl}/get/${encodeURIComponent(key)}`, { |
There was a problem hiding this comment.
Both fetchFn calls in this store (get and set) have no timeout, unlike every other I/O path in this file: createBlendRateSource uses withBlendTimeout, createDefindexRateSource gets its RPC timeout via getRpcServer(..., 10_000). withKeeperRetry's deadline check only fires between retry attempts, not while an attempt's own promise is still pending, so a hung (not erroring, just slow) Upstash REST call blocks Promise.all in createDefindexRateSource indefinitely and can burn the entire keeper FUNCTION_BUDGET_MS on one vault instead of failing fast into the retry/deadline logic as designed.
…-rate-sources # Conflicts: # packages/stellar-sdk-helpers/src/migration-keeper.ts
f63337e to
3a55de9
Compare
Fills in the always-null RateSourceFn stub from #469/#511. Blend prices via @blend-capital/blend-sdk's own Reserve.estSupplyApy (the real three-slope curve, not a hand-rolled reimplementation). DeFindex has no on-chain rate, only a share-price snapshot, so it persists timestamped samples via a pluggable RateSnapshotStore (Upstash Redis REST in production, reusing the same credentials apps/api's rate limiter already needs; in-memory for local dev/tests) and reports a rate once two samples exist far enough apart.
Both are wired in as runMigrationKeeper's default. The live testnet vault still predates migrate_adapter (#514), so this alone doesn't make the keeper migrate anything on testnet yet.
Summary
packages/stellar-sdk-helpers/src/rate-sources.ts:createBlendRateSourcereads the pool via@blend-capital/blend-sdkand convertsReserve.estSupplyApyto bps, so Blend's real deployed rate curve is reused rather than reimplemented off the adapter's raw reserve fields.createDefindexRateSource+RateSnapshotStore: DeFindex'sget_asset_amounts_per_shares()is only a point-in-time share price, so a rate needs two samples over time. This persists each sample keyed by the DeFindex vault's own contract address and returns null (not a fabricated rate) until a second, sufficiently time-separated sample exists.createUpstashRateSnapshotStore(plain HTTP REST, no new dependency) as the production-durable store, reusing the existingUPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKENcredentialsapps/api's rate limiter already requires — no new infra to provision.createInMemoryRateSnapshotStoreis the local/test fallback and is explicitly documented as non-durable across serverless invocations.createDefaultRateSource(config.network)asrunMigrationKeeper's default inmigration-keeper.ts, replacing the stub that always returned null.deps.rateSourceremains fully overridable.migration-keeper.test.ts's one test that asserted the old "default never migrates" behavior to instead assert the real default gets wired in (mocking./rate-sourcesso keeper-mechanics tests stay decoupled from rate-formula correctness)..env.exampleandapps/docs/operations/migration-keeper.mdto describe the new rate-comparison behavior instead of "not implemented yet."Test plan
pnpm typecheck && pnpm testpass locally forstellar-sdk-helpers,api,api-core, andsharedrate-sources.test.ts(22 tests): Blend wiring + a from-scratch reimplementation of Blend's three-slope curve cross-checked against the real SDK'sReserve.setRates()output across all three utilization regimes; DeFindex annualization (including a compounding-vs-linear-extrapolation check), persistence, and edge cases (zero/negative price, malformed responses, too-close-together samples); Upstash REST store request/response shapemigrate_adaptertrigger — blocked on [Bug] Live testnet vault predates migrate_adapter, needs redeployment #514 (the live testnet vault predatesmigrate_adapterand needs a redeploy I don't have a funded deployer key for)Closes #511