diff --git a/packages/stellar-sdk-helpers/src/migration-keeper.test.ts b/packages/stellar-sdk-helpers/src/migration-keeper.test.ts index 5b71b67..3290c1c 100644 --- a/packages/stellar-sdk-helpers/src/migration-keeper.test.ts +++ b/packages/stellar-sdk-helpers/src/migration-keeper.test.ts @@ -1405,6 +1405,228 @@ describe("runMigrationKeeper", () => { ]); }); + // #699: When an active migration snapshot exists for a configured + // candidate adapter that differs from the freshly-derived "best" + // candidate, but the snapshotted adapter still clears + // minImprovementBps, the keeper should prefer completing the existing + // migration instead of overwriting the snapshot with a new + // begin_migration for a different adapter. Without this, alternating + // rates between two adapters across scheduled runs would reset the + // ledger-gap cooldown on every run, and a real migration opportunity + // could never reach migrate_adapter. + it("preserves an active migration snapshot whose adapter still clears the improvement threshold (#699)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + stellarMocks.waitForTransaction.mockResolvedValue({ ledger: 321 }); + + // Two candidate adapters: "defindex" (the existing CONFIG candidate) + // and "blendv2" (a second one that will hold the active snapshot). + const configWithTwoCandidates: MigrationKeeperConfig = { + ...CONFIG, + candidateAdapters: { + defindex: "CDEFINDEXADAPTER", + blendv2: "CBLENDV2ADAPTER", + }, + }; + + // The snapshot on-chain is for blendv2 (CBLENDV2ADAPTER), not the + // best candidate defindex (CDEFINDEXADAPTER) that findBestCandidate + // will derive this run. + mockLiveAdapterAndActiveSnapshot( + DISCOVERED_VAULT.currentAdapterId, + "CBLENDV2ADAPTER" + ); + + // Rates: current (blend) = 500, defindex = 700 (best, improvement 200), + // blendv2 = 650 (still clears minImprovementBps=50, improvement 150). + // findBestCandidate picks defindex (700), but the #699 check should + // override to blendv2 (650) because that's what the snapshot holds. + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + if (adapterId === "CDEFINDEXADAPTER") return 700; + if (adapterId === "CBLENDV2ADAPTER") return 650; + return null; + }); + + const result = await runMigrationKeeper(configWithTwoCandidates, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CSOMEPOOL", + sleep: vi.fn(), + }); + + // migrate_adapter should have been submitted (not begin_migration), + // and the migration target should be the snapshotted adapter + // (CBLENDV2ADAPTER), not the "best" one (CDEFINDEXADAPTER). + expect(server.sendTransaction).toHaveBeenCalledOnce(); + expect(result.migrations).toHaveLength(1); + expect(result.migrations[0]).toMatchObject({ + vaultId: "meridian-usdc", + fromAdapterId: "CBLENDADAPTER", + toAdapterId: "CBLENDV2ADAPTER", + toProtocol: "blendv2", + improvementBps: 150, + }); + expect(result.skipped).toEqual([]); + }); + + // #699: When the snapshotted adapter's rate has genuinely decayed below + // the threshold, the keeper should let the snapshot lapse and pick the + // fresh best candidate instead. This is the "only let the snapshot + // lapse once the snapshotted adapter's rate genuinely stops clearing + // the threshold" half of the fix. + it("replaces a stale migration snapshot whose adapter no longer clears the threshold (#699)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + stellarMocks.waitForTransaction.mockResolvedValue({ ledger: 321 }); + + const configWithTwoCandidates: MigrationKeeperConfig = { + ...CONFIG, + candidateAdapters: { + defindex: "CDEFINDEXADAPTER", + blendv2: "CBLENDV2ADAPTER", + }, + }; + + // Snapshot is for blendv2, but blendv2's rate has dropped to 510 + // (only 10 bps over current=500, below minImprovementBps=50). + // defindex is still 700 (200 bps over current), so findBestCandidate + // picks defindex, and the #699 check should NOT override it. + mockLiveAdapterAndActiveSnapshot( + DISCOVERED_VAULT.currentAdapterId, + "CBLENDV2ADAPTER" + ); + + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + if (adapterId === "CDEFINDEXADAPTER") return 700; + if (adapterId === "CBLENDV2ADAPTER") return 510; + return null; + }); + + const result = await runMigrationKeeper(configWithTwoCandidates, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CSOMEPOOL", + sleep: vi.fn(), + }); + + // begin_migration should have been submitted for the fresh best + // (defindex), not migrate_adapter for the stale snapshot (blendv2). + expect(server.sendTransaction).toHaveBeenCalledOnce(); + expect(result.migrations).toEqual([]); + expect(result.skipped).toMatchObject([ + { + vaultId: "meridian-usdc", + reason: expect.stringContaining("begin_migration submitted"), + }, + ]); + }); + + // Coverage for the branch where candidates is empty but a pinned snapshot + // exists (the pinned adapter is the only non-current candidate). + it("migrates via pinned candidate when it is the only non-current candidate (#699)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + stellarMocks.waitForTransaction.mockResolvedValue({ ledger: 321 }); + + // Only one candidate adapter, which is also the pinned snapshot adapter. + const configWithOneCandidate: MigrationKeeperConfig = { + ...CONFIG, + candidateAdapters: { + blendv2: "CBLENDV2ADAPTER", + }, + }; + + mockLiveAdapterAndActiveSnapshot( + DISCOVERED_VAULT.currentAdapterId, + "CBLENDV2ADAPTER" + ); + + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + if (adapterId === "CBLENDV2ADAPTER") return 600; // 100 bps improvement, clears 50 + return null; + }); + + const result = await runMigrationKeeper(configWithOneCandidate, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CSOMEPOOL", + sleep: vi.fn(), + }); + + // migrate_adapter should have been submitted for the pinned adapter. + expect(server.sendTransaction).toHaveBeenCalledOnce(); + expect(result.migrations).toHaveLength(1); + expect(result.migrations[0]).toMatchObject({ + vaultId: "meridian-usdc", + fromAdapterId: "CBLENDADAPTER", + toAdapterId: "CBLENDV2ADAPTER", + toProtocol: "blendv2", + improvementBps: 100, + }); + }); + + // Coverage for clearsImprovementThreshold at the exact boundary. + it("preserves a snapshot whose improvement is exactly at the threshold boundary (#699)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + stellarMocks.waitForTransaction.mockResolvedValue({ ledger: 321 }); + + const configWithTwoCandidates: MigrationKeeperConfig = { + ...CONFIG, + candidateAdapters: { + defindex: "CDEFINDEXADAPTER", + blendv2: "CBLENDV2ADAPTER", + }, + }; + + mockLiveAdapterAndActiveSnapshot( + DISCOVERED_VAULT.currentAdapterId, + "CBLENDV2ADAPTER" + ); + + // blendv2 rate = 550, current = 500, improvement = 50 (exactly at threshold). + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + if (adapterId === "CDEFINDEXADAPTER") return 700; + if (adapterId === "CBLENDV2ADAPTER") return 550; + return null; + }); + + const result = await runMigrationKeeper(configWithTwoCandidates, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CSOMEPOOL", + sleep: vi.fn(), + }); + + // The pinned candidate clears the threshold (>=), so it should be preferred. + expect(server.sendTransaction).toHaveBeenCalledOnce(); + expect(result.migrations).toHaveLength(1); + expect(result.migrations[0]).toMatchObject({ + toAdapterId: "CBLENDV2ADAPTER", + improvementBps: 50, + }); + }); + it("releases the submission lease after begin_migration so a later run isn't blocked", async () => { const server = makeServer(); stellarMocks.getRpcServer.mockReturnValue(server); @@ -1466,6 +1688,96 @@ describe("runMigrationKeeper", () => { ]); }); + it("does not read the migration snapshot when no candidate clears the threshold (#705)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + return 510; // 10 bps is below CONFIG.minImprovementBps (50). + }); + + const result = await runMigrationKeeper(CONFIG, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CDEFINDEXPOOL", + sleep: vi.fn(), + }); + + expect(result.skipped).toEqual([ + { + vaultId: "meridian-usdc", + reason: "no candidate clears the improvement threshold", + }, + ]); + expect(server.sendTransaction).not.toHaveBeenCalled(); + expect(stellarMocks.simulateView).not.toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.anything(), + "get_migration_snapshot" + ); + }); + + // A failed candidate must not discard a healthy candidate that already + // succeeded in the same batch. The active snapshot's adapter failed, so + // it cannot qualify for preservation; the healthy fresh candidate is + // allowed to proceed instead of being blocked indefinitely. + it("does not let a failed pinned snapshot block a healthy non-pinned candidate (#705)", async () => { + const server = makeServer(); + stellarMocks.getRpcServer.mockReturnValue(server); + stellarMocks.waitForTransaction.mockResolvedValue({ ledger: 321 }); + + const configWithTwoCandidates: MigrationKeeperConfig = { + ...CONFIG, + candidateAdapters: { + defindex: "CDEFINDEXADAPTER", + blendv2: "CBLENDV2ADAPTER", + }, + }; + + // Snapshot pins blendv2 as the in-progress migration target. + mockLiveAdapterAndActiveSnapshot( + DISCOVERED_VAULT.currentAdapterId, + "CBLENDV2ADAPTER" + ); + + // The snapshotted adapter (blendv2) throws; defindex succeeds and + // remains a valid migration because its rate was already obtained. + const rateSource = vi.fn(async ({ adapterId }: { adapterId: string }) => { + if (adapterId === "CBLENDADAPTER") return 500; + if (adapterId === "CBLENDV2ADAPTER") + throw new Error("RPC error: blendv2 rate fetch failed"); + if (adapterId === "CDEFINDEXADAPTER") return 700; // 200 bps improvement + return null; + }); + + const result = await runMigrationKeeper(configWithTwoCandidates, { + logger: logger(), + discoverVaults: async () => ({ + vaults: [DISCOVERED_VAULT], + failures: [], + }), + rateSource, + resolveCandidatePool: async () => "CSOMEPOOL", + sleep: vi.fn(), + }); + + // begin_migration should be submitted for the healthy candidate. + expect(server.sendTransaction).toHaveBeenCalledOnce(); + expect(result.migrations).toEqual([]); + expect(result.failures).toEqual([]); + expect(result.skipped).toMatchObject([ + { + vaultId: "meridian-usdc", + reason: expect.stringContaining("begin_migration submitted"), + }, + ]); + }); + it("skips submission when the deadline is reached during evaluation, not just before it started", async () => { // The deadline check at the top of the loop only catches a budget // that's already gone before this vault starts; evaluation itself can diff --git a/packages/stellar-sdk-helpers/src/migration-keeper.ts b/packages/stellar-sdk-helpers/src/migration-keeper.ts index 7cefdf8..c768ed9 100644 --- a/packages/stellar-sdk-helpers/src/migration-keeper.ts +++ b/packages/stellar-sdk-helpers/src/migration-keeper.ts @@ -125,6 +125,33 @@ function isUsableRate(rate: number | null): rate is number { return rate !== null && Number.isFinite(rate); } +/** Returns true when `rate - currentRate >= minImprovementBps`. Extracted + * so the fresh-candidate path and the snapshot-preservation path share one + * comparison. A future change to threshold semantics (e.g. rounding-aware + * or percentage-based) only changes the comparison in one place. */ +function clearsImprovementThreshold( + rate: number, + currentRate: number, + minImprovementBps: number +): boolean { + return rate - currentRate >= minImprovementBps; +} + +/** A successfully evaluated candidate, retained so the caller can prefer an + * already-snapshotted adapter without fetching its rate a second time. */ +interface CandidateRate { + protocol: string; + adapterId: string; + rate: number; +} + +interface FindBestCandidateResult { + best: BestCandidate | null; + currentRate: number | null; + candidateRates: Map; + skipReason?: string; +} + export interface MigrationKeeperConfig { network: StellarNetwork; secretKey: string; @@ -521,13 +548,18 @@ async function findBestCandidate( logger: KeeperLogger, sleepFn: (ms: number) => Promise, deadlineAt: number -): Promise<{ best: BestCandidate | null; skipReason?: string }> { +): Promise { // Nothing to compare against: don't pay for a retried rate lookup (up to // maxAttempts, with backoff) just to discover there was never a candidate // to evaluate. This is the documented default state today (no // MERIDIAN_ADAPTER__ID configured), not a rare edge case. if (Object.keys(config.candidateAdapters).length === 0) { - return { best: null, skipReason: "no candidate adapters configured" }; + return { + best: null, + currentRate: null, + candidateRates: new Map(), + skipReason: "no candidate adapters configured", + }; } // Only excludes the vault's literal current adapter, not same-protocol @@ -546,6 +578,8 @@ async function findBestCandidate( if (candidates.length === 0) { return { best: null, + currentRate: null, + candidateRates: new Map(), skipReason: "every configured candidate is the vault's current adapter", }; } @@ -585,37 +619,51 @@ async function findBestCandidate( ); } if (!isUsableRate(currentRate)) { - return { best: null, skipReason: "current rate unavailable" }; + return { + best: null, + currentRate: null, + candidateRates: new Map(), + skipReason: "current rate unavailable", + }; } - // Evaluated concurrently, like discovery above: each candidate's pool - // resolution and rate lookup is independent of every other candidate, so - // running them one at a time would let the deadline budget get eaten by - // earlier candidates before later ones are even attempted. + const candidateRates = new Map(); + + // Evaluated concurrently: every candidate's pool resolution and rate + // lookup is independent of every other entry, so running them one at a + // time would let the deadline budget get eaten by earlier entries before + // later ones are even attempted. + const evaluate = (protocol: string, adapterId: string) => + withKeeperRetry( + async () => { + const poolId = await resolveCandidatePool(adapterId); + return rateSource({ + protocol, + adapterId, + poolId, + ...(vault.assetId !== undefined && { assetId: vault.assetId }), + }); + }, + { + maxAttempts: config.maxAttempts, + baseDelayMs: config.baseDelayMs, + deadlineAt, + }, + logger, + { vaultId: vault.vaultId, adapterId, protocol, stage: "evaluate" }, + sleepFn, + isTransientKeeperError, + "migration-keeper" + ); + const settled = await Promise.allSettled( candidates.map(async ([protocol, adapterId]) => { - const result = await withKeeperRetry( - async () => { - const poolId = await resolveCandidatePool(adapterId); - return rateSource({ - protocol, - adapterId, - poolId, - ...(vault.assetId !== undefined && { assetId: vault.assetId }), - }); - }, - { - maxAttempts: config.maxAttempts, - baseDelayMs: config.baseDelayMs, - deadlineAt, - }, - logger, - { vaultId: vault.vaultId, adapterId, protocol, stage: "evaluate" }, - sleepFn, - isTransientKeeperError, - "migration-keeper" - ); - return { protocol, adapterId, rate: result.value }; + const result = await evaluate(protocol, adapterId); + return { + protocol, + adapterId, + rate: result.value, + }; }) ); @@ -646,22 +694,35 @@ async function findBestCandidate( protocol, error: errorMessage(outcome.reason), }); - firstFailure ??= { protocol, adapterId, reason: outcome.reason }; + firstFailure ??= { + protocol, + adapterId, + reason: outcome.reason, + }; continue; } const { rate } = outcome.value; if (!isUsableRate(rate)) continue; anyRateKnown = true; + candidateRates.set(adapterId, { protocol, adapterId, rate }); const improvementBps = rate - currentRate; - if (improvementBps < config.minImprovementBps) continue; + if ( + !clearsImprovementThreshold(rate, currentRate, config.minImprovementBps) + ) + continue; + if (!best || improvementBps > best.improvementBps) { - best = { protocol, adapterId, improvementBps }; + best = { + protocol, + adapterId, + improvementBps, + }; } } if (best) { - return { best }; + return { best, currentRate, candidateRates }; } // A failed candidate must never block a valid decision reached from a // different candidate, this applies just as much when that decision is @@ -684,6 +745,8 @@ async function findBestCandidate( // compared when they weren't. return { best: null, + currentRate, + candidateRates, skipReason: anyRateKnown ? "no candidate clears the improvement threshold" : "no candidate rate was available to compare", @@ -871,7 +934,7 @@ export async function runMigrationKeeper( }); } - let evaluation: { best: BestCandidate | null; skipReason?: string }; + let evaluation: FindBestCandidateResult; try { evaluation = await findBestCandidate( vault, @@ -913,7 +976,7 @@ export async function runMigrationKeeper( continue; } - const { best } = evaluation; + let { best } = evaluation; // Re-checked here, not just at the top of the loop: evaluation itself // can retry and consume most of the budget, and this is an @@ -937,43 +1000,12 @@ export async function runMigrationKeeper( }); continue; } - // Taken before anything is built: a plain "no record" read is not a - // claim on the vault, so two concurrent invocations could otherwise both - // pass the check above and both broadcast. - const acquired = await SubmissionLease.acquire({ - store: stateStore, - key: stateKey, - submissionTtlMs: config.submissionTtlMs, - logger, - context: priorContext, - }); - if ("error" in acquired) { - skipped.push({ - vaultId: vault.vaultId, - reason: `could not take the submission lease (${acquired.error}); skipped rather than risk a duplicate migration`, - }); - continue; - } - const lease = acquired.lease; - const submissionHooks = lease.hooks; - // migrate_adapter (#567) now requires an active begin_migration - // snapshot for the same target adapter, at least MIN_LEDGER_GAP - // ledgers old. Rather than duplicate the contract's ledger-gap math - // here, this only checks whether a matching snapshot exists; if the - // cooldown hasn't elapsed yet, the contract itself rejects the call - // with MigrationCooldownNotMet during simulation (no fee, nothing - // sent), which falls through to the existing failure handling below - // and is retried on a later run — comfortably fine given - // MIN_LEDGER_GAP is ~1 minute and this keeper runs far less often - // than that. - // - // Skipped entirely when deps.submitMigration is injected: that's a - // full override of the on-chain submission mechanism (see its use - // below), and this on-chain precheck would otherwise reach the real - // network/mocks regardless of the injected override, same as - // assertAdapterUnchanged inside submitMigrationTransaction only runs - // on the real path. + // Read the snapshot only after a qualifying fresh candidate exists. + // In steady state no candidate clears the threshold, so this avoids + // one on-chain read per vault per run (#705) while preserving #699: + // the snapshotted adapter was already evaluated in the same concurrent + // candidate batch, and a still-qualifying snapshot is preferred here. let hasMatchingSnapshot = deps.submitMigration != null; let snapshotReadFailed = false; if (!hasMatchingSnapshot) { @@ -984,11 +1016,33 @@ export async function runMigrationKeeper( config.network.passphrase, "get_migration_snapshot" )) as { adapter: string } | null; - hasMatchingSnapshot = snapshot?.adapter === best.adapterId; + const snapshotAdapter = snapshot?.adapter ?? null; + const snapshotCandidate = + snapshotAdapter === null + ? undefined + : evaluation.candidateRates.get(snapshotAdapter); + if ( + snapshotAdapter !== null && + snapshotAdapter !== vault.currentAdapterId && + snapshotCandidate !== undefined && + clearsImprovementThreshold( + snapshotCandidate.rate, + evaluation.currentRate ?? 0, + config.minImprovementBps + ) + ) { + best = { + protocol: snapshotCandidate.protocol, + adapterId: snapshotCandidate.adapterId, + improvementBps: + snapshotCandidate.rate - (evaluation.currentRate ?? 0), + }; + hasMatchingSnapshot = true; + } } catch (err) { // A genuine "no snapshot" (or one for a different adapter) traps // with MigrationNotInitialized, which is not a transient failure by - // isTransientKeeperError's classification — that's the case this + // isTransientKeeperError's classification, that's the case this // falls through to begin_migration for. A real RPC/network failure // reading the snapshot must NOT be treated the same way: assuming // "no snapshot" and firing begin_migration would reset a possibly @@ -996,7 +1050,6 @@ export async function runMigrationKeeper( // pushing a ready-to-migrate vault's migration back a full // MIN_LEDGER_GAP for no reason. Report it as a retryable failure // instead, same as this file's other on-chain read checks. - hasMatchingSnapshot = false; snapshotReadFailed = isTransientKeeperError(err); } } @@ -1013,10 +1066,46 @@ export async function runMigrationKeeper( error: "could not read the migration snapshot; skipped rather than risk resetting an existing cooldown", }); - await lease.releaseIfUnsent(); continue; } + // Taken before anything is built: a plain "no record" read is not a + // claim on the vault, so two concurrent invocations could otherwise both + // pass the check above and both broadcast. + const acquired = await SubmissionLease.acquire({ + store: stateStore, + key: stateKey, + submissionTtlMs: config.submissionTtlMs, + logger, + context: priorContext, + }); + if ("error" in acquired) { + skipped.push({ + vaultId: vault.vaultId, + reason: `could not take the submission lease (${acquired.error}); skipped rather than risk a duplicate migration`, + }); + continue; + } + const lease = acquired.lease; + const submissionHooks = lease.hooks; + + // migrate_adapter (#567) now requires an active begin_migration + // snapshot for the same target adapter, at least MIN_LEDGER_GAP + // ledgers old. Rather than duplicate the contract's ledger-gap math + // here, this only checks whether a matching snapshot exists; if the + // cooldown hasn't elapsed yet, the contract itself rejects the call + // with MigrationCooldownNotMet during simulation (no fee, nothing + // sent), which falls through to the existing failure handling below + // and is retried on a later run. That is comfortably fine given + // MIN_LEDGER_GAP is ~1 minute and this keeper runs far less often + // than that. + // + // Skipped entirely when deps.submitMigration is injected: that's a + // full override of the on-chain submission mechanism (see its use + // below), and this on-chain precheck would otherwise reach the real + // network/mocks regardless of the injected override, same as + // assertAdapterUnchanged inside submitMigrationTransaction only runs + // on the real path. if (!hasMatchingSnapshot) { try { await submitKeeperOperation(