Skip to content

[FEATURE] DeFi: Implement yield farming aggregator service#331

Merged
KevinMB0220 merged 3 commits into
Galaxy-KJ:mainfrom
sotoJ24:issue/298
Jul 3, 2026
Merged

[FEATURE] DeFi: Implement yield farming aggregator service#331
KevinMB0220 merged 3 commits into
Galaxy-KJ:mainfrom
sotoJ24:issue/298

Conversation

@sotoJ24

@sotoJ24 sotoJ24 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the YieldFarmingAggregator service, providing a unified
interface to query APY rates across Blend lending pools, Soroswap
liquidity pools, and internal Vault strategies, and automatically route
deposits to the highest-yielding source (roadmap item #43, issue #298).

Closes #298.

Files

  • packages/core/defi-protocols/src/services/yield-farming-aggregator.ts — service + adapters + types
  • packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts — unit tests
  • packages/core/defi-protocols/tsconfig.test.json — test-only TypeScript config
  • packages/core/defi-protocols/jest.config.cjs — updated Jest configuration
  • packages/core/defi-protocols/src/index.ts — exports added

Design

API surface (matches issue spec)

export interface YieldOpportunity {
  protocol: string;  // 'Blend' | 'Soroswap' | 'Vault'
  asset: string;     // e.g. 'USDC', 'XLM'
  apy: number;       // effective APY as a percentage (10 = 10%)
  tvl: string;       // total value locked in asset units
  sourceId: string;  // opaque ref forwarded to the adapter on deposit
}

export class YieldFarmingAggregator {
  async getOpportunities(): Promise<YieldOpportunity[]>;
  async deposit(opportunity: YieldOpportunity, amount: string): Promise<DepositResult>;

  // Extended API
  async getBestOpportunity(asset: string): Promise<YieldOpportunity | undefined>;
  async depositOptimal(asset: string, amount: string): Promise<DepositResult>;
  async withdraw(opportunity: YieldOpportunity, amount: string): Promise<string>;
  async rebalance(positions, totalAmount, asset): Promise<RebalanceResult | null>;
  weightedAverageApy(positions): number;
}

Protocol adapter pattern

The aggregator has zero hard dependencies on Blend, Soroswap, or Vault
code. Each source is a YieldSource adapter:

export interface YieldSource {
  readonly protocol: string;
  fetchOpportunities(): Promise<YieldOpportunity[]>;
  deposit(opportunity: YieldOpportunity, amount: string): Promise<DepositResult>;
  withdraw(opportunity: YieldOpportunity, amount: string): Promise<string>;
}

Three concrete adapters ship out-of-the-box:

Adapter APY derivation
BlendYieldSource Converts Blend's nominal supply APR to daily-compounded APY via aprToApyByFrequency from the existing yield-calculator.ts
SoroswapYieldSource Derives fee APY from 24h volume, TVL, and fee tier via calculateLpApy (daily compounding)
VaultYieldSource Reads effective APY directly from the vault strategy config — no conversion needed

All three normalise to the same apy field so cross-protocol comparisons
are fair and consistent.

Key behaviours

getOpportunities — uses Promise.allSettled so a single failing
source never blocks the others. Results are sorted by APY descending.

depositOptimal — convenience wrapper that calls getBestOpportunity
and deposit in one call, routing funds to the highest-APY source for
the given asset automatically.

rebalance — exits all current positions and enters the best
available opportunity only when the APY improvement exceeds
rebalanceThresholdPct (default 0.5 pp). Returns null when the
improvement is below the threshold, preventing unnecessary churn.

weightedAverageApy — pure BigNumber arithmetic with no network
calls. Computes the portfolio-level APY weighted by position size.

Input validationamount is validated as a positive numeric string
on every deposit, withdraw, and depositOptimal call. Invalid
amounts and unknown protocol names throw descriptive errors.

Integration with existing package

  • Uses aprToApyByFrequency and calculateLpApy from the existing
    utils/yield-calculator.ts — no new math code needed
  • Uses BigNumber (already a dependency) for all weighted calculations
  • Exported from src/index.ts alongside the existing service exports

Tests

Screenshot from 2026-06-30 23-51-44

Acceptance criteria

  • Correctly fetches and aggregates APY rates from Blend, Soroswap,
    and Vault in a unified YieldOpportunity[] sorted by APY
  • Deposits route through the chosen protocol adapter via deposit
    and depositOptimal
  • Unit tests cover rebalancing calculations — threshold guard,
    multi-position exit, APY gain reporting
  • Failing sources do not block other sources (Promise.allSettled)
  • All APY values are effective/compounded rates for fair comparison
  • Protocol-agnostic — new sources added by implementing YieldSource
  • Uses existing yield-calculator.ts utilities — no duplicated math

Summary by CodeRabbit

  • New Features
    • Added on-chain governance with proposals, token-weighted voting, timed finalization, execution status, and token locking/unlocking.
    • Added a sequential automation execution chain with shared context, per-step retries, and verbose failure reporting.
    • Added a liquidity-pool volume trigger that checks whether 24h activity exceeds a configured threshold.
    • Added a yield farming aggregator with opportunity discovery, best-opportunity selection, deposits/withdrawals, and threshold-based rebalancing.
  • Bug Fixes
    • Improved validation and error handling for invalid inputs and failing pipeline steps.
  • Tests / Chores
    • Added/expanded unit tests and updated Jest/TypeScript configs for the involved packages.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@sotoJ24, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5a2dd7a5-4933-4be7-9050-8318123b6b3f

📥 Commits

Reviewing files that changed from the base of the PR and between b1045a1 and cdb3290.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (11)
  • .gitignore
  • packages/contracts/governance/.gitignore
  • packages/contracts/governance/src/lib.rs
  • packages/core/automation/jest.config.cjs
  • packages/core/defi-protocols/__tests__/protocols/blend-protocol-operations.test.ts
  • packages/core/defi-protocols/__tests__/protocols/blend-protocol.test.ts
  • packages/core/defi-protocols/__tests__/vault/vault-client.test.ts
  • packages/core/defi-protocols/jest.config.cjs
  • packages/core/defi-protocols/package.json
  • packages/core/defi-protocols/src/types/defi-types.ts
  • tsconfig.json
📝 Walkthrough

Walkthrough

This PR adds a Soroban governance contract, an automation execution chain and volume trigger, and a DeFi yield farming aggregator with adapters, tests, and TypeScript/Jest configuration updates.

Changes

Soroban Governance Contract

Layer / File(s) Summary
Manifest and data types
packages/contracts/governance/Cargo.toml, packages/contracts/governance/src/lib.rs
Adds contract metadata, Soroban SDK dependencies, storage keys, constants, and typed proposal/vote/lock structures.
Lifecycle methods
packages/contracts/governance/src/lib.rs
Implements proposal creation, voting, finalization, execution, and token lock management with storage and threshold checks.
Governance tests
packages/contracts/governance/src/lib.rs
Adds harness and tests for success paths, rejections, panics, and lock/id behavior.

Automation Execution Chain and Volume Trigger

Layer / File(s) Summary
Execution chain types
packages/core/automation/src/execution-chain/index.ts
Adds execution-step/result/context types, retry options, and helper utilities.
ExecutionChain implementation
packages/core/automation/src/execution-chain/index.ts
Implements sequential step execution, retries, output threading, and failure logging.
ExecutionChain tests
packages/core/automation/src/test/execution.chain.test.ts
Covers empty chains, sequential execution, context threading, failure handling, retries, registration, and pipeline simulation.
Volume trigger implementation
packages/core/automation/src/triggers/volume-trigger.ts
Adds Horizon trade types, fetcher abstraction, and 24h volume threshold evaluation.
Volume trigger tests
packages/core/automation/src/test/volume-trigger.test.ts
Covers threshold checks, rolling windows, URL construction, validation, and fetch error propagation.
Automation config
packages/core/automation/jest.config.cjs, packages/core/automation/package.json, packages/core/automation/tsconfig.json, packages/core/automation/tsconfig.test.json
Updates Jest transforms/test matching, adds Jest types, and introduces test/build TypeScript settings.

DeFi Yield Farming Aggregator

Layer / File(s) Summary
Contracts and adapters
packages/core/defi-protocols/src/services/yield-farming-aggregator.ts
Defines yield opportunity contracts and Blend, Soroswap, and Vault source adapters.
Aggregator logic
packages/core/defi-protocols/src/services/yield-farming-aggregator.ts
Implements opportunity aggregation, selection, deposit routing, rebalance, weighted APY, and validation helpers.
Aggregator tests
packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts
Covers aggregation, best-opportunity selection, deposit flows, rebalancing, weighted APY, and adapter behavior.
DeFi config
packages/core/defi-protocols/jest.config.cjs, packages/core/defi-protocols/tsconfig.json, packages/core/defi-protocols/tsconfig.test.json
Updates Jest transform setup and adds TypeScript test configuration plus compiler options.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Proposer
  participant GovernanceContract
  participant Storage
  Proposer->>GovernanceContract: propose(action)
  GovernanceContract->>Storage: store Proposal(Active)
  GovernanceContract->>Storage: record VoteRecord on vote(weight, support)
  GovernanceContract->>Storage: finalize() reads votes_for/against
  GovernanceContract->>Storage: update status Passed/Rejected
  GovernanceContract->>Storage: execute() updates status Executed
Loading
sequenceDiagram
  participant Caller
  participant ExecutionChain
  participant StepExecutor
  participant ExecutionContext
  Caller->>ExecutionChain: execute(steps)
  loop each step
    ExecutionChain->>StepExecutor: run step with ExecutionContext
    alt success
      StepExecutor-->>ExecutionChain: output
      ExecutionChain->>ExecutionContext: write outputs[label]
    else failure
      ExecutionChain->>ExecutionChain: runWithRetry
      ExecutionChain-->>Caller: ChainResult(failedAtIndex, error)
    end
  end
  ExecutionChain-->>Caller: ChainResult
Loading
sequenceDiagram
  participant Caller
  participant YieldFarmingAggregator
  participant YieldSource
  Caller->>YieldFarmingAggregator: rebalance(currentPositions, totalAmount, asset)
  YieldFarmingAggregator->>YieldFarmingAggregator: getBestOpportunity(asset)
  alt apyGain below threshold
    YieldFarmingAggregator-->>Caller: null
  else apyGain meets threshold
    loop each current position
      YieldFarmingAggregator->>YieldSource: withdraw(position)
    end
    YieldFarmingAggregator->>YieldSource: deposit(bestOpportunity, totalAmount)
    YieldFarmingAggregator-->>Caller: RebalanceResult(apyGainPct)
  end
Loading

Possibly related PRs

Suggested reviewers: KevinMB0220

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also changes governance and automation crates that are unrelated to #298 and the defi-protocols yield aggregator scope. Split unrelated governance and automation work into separate PRs or remove it from this change so the diff stays scoped to defi-protocols yield aggregation.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the PR's main change: adding a yield farming aggregator service.
Description check ✅ Passed The description covers the feature summary, files, design, tests, and acceptance criteria, though some template checklists are unfilled.
Linked Issues check ✅ Passed The PR implements the requested aggregator, adapters, deposit/withdraw/optimize/rebalance flows, tests, and no-extra-dependency constraint for #298.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 15

🧹 Nitpick comments (8)
packages/core/automation/src/execution-chain/index.ts (2)

378-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale path in trailing comment.

References packages/core/automation/__tests__/execution-chain.test.ts, but the actual suite is at packages/core/automation/src/test/execution.chain.test.ts (also mismatched in that file's header comment at Line 4).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/src/execution-chain/index.ts` around lines 378 -
381, The trailing test-path comment in the execution-chain module is stale and
points to the wrong suite location. Update the comment near the end of the file
that references the Jest tests so it matches the actual test file path used by
the execution-chain suite, and keep the header comment in the related test file
consistent with that same location. Use the execution-chain module comments and
the test file header as the unique spots to correct.

213-271: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Steps sharing a type without a label silently overwrite each other's context output.

context.outputs[label] (Line 271) is keyed by step.label ?? step.type (Line 215). If a pipeline invokes the same step type twice without distinct labels (e.g. two soroswap_swap steps), the second execution's output overwrites the first in context.outputs before any later step can read it, causing silent data loss rather than a visible failure. Consider validating/enforcing unique labels up front, or auto-suffixing duplicates (e.g. type#index).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/src/execution-chain/index.ts` around lines 213 -
271, The execution chain currently uses step.label ?? step.type as the context
key in ExecutionChain, so repeated steps of the same type without explicit
labels can overwrite earlier outputs in context.outputs. Update the chain
validation/execution flow in ExecutionChain.run (and related helpers like
logFailure/runWithRetry if needed) to enforce unique step keys up front or
automatically generate distinct labels for duplicate types, and keep
context.outputs keyed by that unique identifier so later steps cannot silently
clobber prior results.
packages/core/defi-protocols/tsconfig.test.json (1)

1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extending the base tsconfig instead of duplicating options.

Unlike tsconfig.json (which extends ../../../tsconfig.json), this test config redeclares target, esModuleInterop, resolveJsonModule, strict, skipLibCheck independently. Any future change to the shared base config (e.g., stricter compiler flags) won't propagate here, risking drift between build and test compilation.

♻️ Suggested refactor
 {
+  "extends": "../../../tsconfig.json",
   "compilerOptions": {
-    "target": "ES2020",
     "module": "CommonJS",
     "moduleResolution": "node",
-    "esModuleInterop": true,
-    "resolveJsonModule": true,
-    "strict": true,
-    "skipLibCheck": true,
     "declaration": false,
     "baseUrl": "."
   },
   "include": ["src/**/*", "__tests__/**/*"],
   "exclude": ["node_modules", "dist"]
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/defi-protocols/tsconfig.test.json` around lines 1 - 15, The
test TypeScript config is duplicating shared compiler settings instead of
inheriting them, which can cause drift from the main setup. Update the test
config to extend the shared base config like tsconfig.json does, and keep only
test-specific overrides in this file; use the existing tsconfig.test.json
structure and compilerOptions section to locate the duplicated settings to
remove.
packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts (1)

99-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication in failing-source mock.

The bad source is hand-rolled instead of reusing mockSource, duplicating its shape. Extending the helper with an optional fetchOpportunitiesFn/reject option would keep this consistent with the rest of the suite.

♻️ Optional refactor
-function mockSource(
+function mockSource(
   protocol: string,
   opportunities: YieldOpportunity[],
   depositFn?: (opp: YieldOpportunity, amount: string) => Promise<DepositResult>,
   withdrawFn?: (opp: YieldOpportunity, amount: string) => Promise<string>,
+  fetchFn?: () => Promise<YieldOpportunity[]>,
 ): jest.Mocked<YieldSource> {
   return {
     protocol,
-    fetchOpportunities: jest.fn().mockResolvedValue(opportunities),
+    fetchOpportunities: fetchFn ? jest.fn(fetchFn) : jest.fn().mockResolvedValue(opportunities),
     deposit: jest.fn(depositFn ?? (async (opp, amount) => depositResult(opp, amount))),
     withdraw: jest.fn(withdrawFn ?? (async (_opp, amount) => amount)),
   } as jest.Mocked<YieldSource>;
 }
-      const bad  = { protocol: 'Bad',  fetchOpportunities: jest.fn().mockRejectedValue(new Error('network error')), deposit: jest.fn(), withdraw: jest.fn() };
+      const bad  = mockSource('Bad', [], undefined, undefined, () => Promise.reject(new Error('network error')));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts`
around lines 99 - 108, The failing-source setup in YieldFarmingAggregator tests
is duplicating the source shape instead of using the existing mockSource helper.
Update mockSource to support an optional fetchOpportunities override or
rejection option, then replace the hand-rolled bad source in
yield-farming-aggregator.test.ts with that helper so the test stays consistent
with the rest of the suite.
packages/core/automation/src/triggers/volume-trigger.ts (1)

212-217: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

computeVolume uses floating-point arithmetic for on-chain amounts.

parseFloat sums into a plain JS number, which is subject to precision loss for large or many-decimal amounts. Since this feeds a threshold comparison (not a settlement path), the risk is limited, but consider BigNumber/Decimal for consistency with financial-precision conventions used elsewhere in the codebase.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/src/triggers/volume-trigger.ts` around lines 212 -
217, The computeVolume method in volume-trigger.ts is summing HorizonTrade
base_amount values with parseFloat and plain JS numbers, which can lose
precision for on-chain amounts. Update computeVolume to use the project’s
financial-precision type (for example BigNumber/Decimal) for parsing and
accumulation, then convert only at the final threshold comparison point if
needed. Keep the method name computeVolume and its HorizonTrade reduce logic as
the place to update, but avoid floating-point arithmetic in the accumulation
path.
packages/core/automation/src/test/volume-trigger.test.ts (1)

240-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

DefaultHorizonFetcher itself is untested.

All tests inject a mock HorizonFetcher, so the production DefaultHorizonFetcher.fetchTrades (non-ok status handling, JSON parsing) has no direct coverage. Consider a small test using a mocked global fetch to validate the error-status branch.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/src/test/volume-trigger.test.ts` around lines 240 -
250, The current tests only cover injected HorizonFetcher mocks, so
DefaultHorizonFetcher.fetchTrades still lacks direct coverage for its production
error handling. Add a focused test around DefaultHorizonFetcher that mocks the
global fetch response and verifies the non-ok status path (and, if practical,
the JSON parsing path) throws the expected error; use the DefaultHorizonFetcher
and fetchTrades symbols to locate the implementation.
packages/core/automation/tsconfig.json (1)

5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

ignoreDeprecations: "6.0" is masking the baseUrl deprecation rather than fixing it.

TypeScript 6.0 deprecated baseUrl as a module-resolution root; "ignoreDeprecations": "6.0" is documented as these deprecations can be ignored by setting "ignoreDeprecations": "6.0" in your tsconfig; however, note that TypeScript 7.0 will not support any of these deprecated options. This file still keeps baseUrl (line 4) combined with unprefixed paths entries (lines 19-35), which is exactly the pattern that will break once the escape hatch is removed in TypeScript 7.0. The recommended fix is to remove baseUrl and add the prefix to their paths entries instead of suppressing the warning.

rootDir: "." looks like a correct adaptation to the TS 6.0 default-rootDir change, since in v6, the default rootDir is any path that contains the tsconfig.json which is typically the root directory, so this part is fine.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/tsconfig.json` around lines 5 - 6, The tsconfig
change is suppressing the TypeScript 6.0 `baseUrl` deprecation instead of
addressing it. Remove `ignoreDeprecations` from this config and update the
module-resolution setup by eliminating `baseUrl` and prefixing the existing
`paths` entries accordingly in the same tsconfig. Keep the `rootDir` adjustment
as-is, and make sure the `paths` mappings still resolve correctly without
relying on `baseUrl`.
packages/core/automation/jest.config.cjs (1)

13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant testMatch pattern.

'**/src/test/**/*.test.ts' is already covered by '**/src/**/*.test.ts' since src/test/** is a subset of src/**. Consider keeping a single glob.

Simplify
  testMatch: [
-   '**/src/test/**/*.test.ts',
    '**/src/**/*.test.ts',
  ],
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/jest.config.cjs` around lines 13 - 16, The Jest
config in the testMatch array has a redundant glob because the src/test pattern
is already included by the broader src pattern. Simplify the testMatch
definition in the jest.config.cjs config by removing the narrower
'**/src/test/**/*.test.ts' entry and keeping the single broader glob, using the
testMatch symbol to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/contracts/governance/src/lib.rs`:
- Around line 282-309: `lock_tokens` is counting unbacked voting weight, so
update the logic to ensure the locked amount comes from a real governance token
balance before storing it in `LOCKS`. In `lock_tokens`, verify the caller/voter
has sufficient tokens by checking or transferring from the governance token
contract, and only then accumulate the lock amount and persist the `TokenLock`.
Use the existing `lock_tokens`, `LOCKS`, and `TokenLock` flow as the entry
point, and make sure the stored lock reflects escrowed value rather than just an
authenticated request.
- Around line 148-156: The vote validation in the governance flow currently only
compares the requested weight against TokenLock.amount, so the same locked
balance can be reused across overlapping proposals. Update the proposal voting
path in the relevant governance functions (the validation around the lock check
and the vote-casting logic in the same module) to track and deduct
already-committed weight per voter across active proposals, or otherwise enforce
a per-proposal available-weight snapshot before accepting a vote. Make sure the
fix is applied consistently where votes are recorded so the reserved amount
cannot be double-counted across concurrent proposals.
- Around line 319-330: The unlock_tokens flow currently removes the voter’s
TokenLock without checking whether any of their votes are still tied to active
proposals. Update unlock_tokens to inspect the voter’s VoteRecord entries before
calling locks.remove, and reject the unlock if any referenced proposal is still
Active. Use the existing unlock_tokens, TokenLock, and VoteRecord symbols to
locate the logic, and ensure the lock is only deleted once all voted proposals
are finalized.
- Around line 237-238: The proposal pass check in the governance logic currently
treats an exact 50/50 split as passing because the approval ratio uses a
non-strict comparison. Update the condition in the pass calculation around the
proposal voting logic so ties fail by making the approval check strict, and keep
the quorum check unchanged. Use the existing proposal vote variables in the same
expression where passed is computed to ensure the final status only becomes
Passed when the vote share is strictly above the threshold.

In `@packages/core/automation/src/execution-chain/index.ts`:
- Around line 297-351: runWithRetry in ExecutionChain can return a failure
result with undefined error when maxRetries is negative, which later breaks
execute() via logFailure. Clamp retryAttempts/maxRetries to a non-negative value
before the retry loop, or ensure runWithRetry always sets a real Error when no
attempts run; use the runWithRetry, execute, and logFailure symbols to keep the
failure path safe and avoid dereferencing stepResult.error! on undefined.

In `@packages/core/automation/src/test/volume-trigger.test.ts`:
- Around line 170-195: The Horizon URL tests in VolumeTrigger are too loose and
miss the wrong endpoint/query shape. Update the assertions around
trackPoolVolume and buildTradesUrl so they verify the full request path and
parameter structure, not just substring matches: the URL should use the
liquidity_pools/{poolId}/trades endpoint shape and the custom horizonUrl path
should still be preserved. Use the existing VolumeTrigger, buildTradesUrl, and
fetchTrades mock calls to assert the exact endpoint semantics rather than only
checking for POOL_ID or trade_type text.

In `@packages/core/automation/src/triggers/volume-trigger.ts`:
- Around line 195-206: The fetchRecentTrades method only reads the first Horizon
page from fetcher.fetchTrades, which causes volume24h and tradeCount to be
understated when more than one page of trades falls within the window. Update
fetchRecentTrades to follow the response’s _links.next cursor and keep
accumulating records until trades fall outside the cutoff or pagination ends,
using the existing fetcher/buildTradesUrl flow. Keep the filtering by
ledger_close_time and the cutoff logic in fetchRecentTrades, but ensure all
relevant pages are traversed before returning.
- Around line 97-107: DefaultHorizonFetcher.fetchTrades currently calls
fetch(url) without any timeout, so a slow or hanging Horizon request can block
the automation flow indefinitely. Update fetchTrades in DefaultHorizonFetcher to
use an AbortController-based timeout around the fetch call, abort the request
when the deadline is exceeded, and surface a clear timeout error alongside the
existing Horizon request failure handling.
- Around line 224-232: The buildTradesUrl method in volume-trigger.ts is using
the generic /trades endpoint with a liquidity_pool_id query param, which can
return unrelated results and skew computeVolume. Update buildTradesUrl to
construct the pool-specific Horizon URL for GET
/liquidity_pools/{liquidity_pool_id}/trades using the poolId directly in the
path, and keep the existing pagination/sort parameters. Also add or update a
test around buildTradesUrl/volume trigger URL generation that asserts the exact
full URL, not just a substring.

In `@packages/core/automation/tsconfig.test.json`:
- Around line 1-16: The test TypeScript config is duplicating compiler settings
instead of inheriting from the package base config, which drops the shared path
aliases needed by ts-jest. Update tsconfig.test.json to extend the local
tsconfig.json so it reuses the existing compilerOptions, especially the
workspace path mappings used by src imports like `@galaxy-kj/core-oracles` and
`@galaxy-kj/core-stellar-sdk`.

In `@packages/core/defi-protocols/src/services/yield-farming-aggregator.ts`:
- Around line 176-177: The fetcher contract methods in YieldFarmingAggregator
are currently returning only a single string, which cannot satisfy
DepositResult’s txId and sharesReceived fields correctly. Update the affected
adapter interfaces and implementations (including the Soroswap and Vault
adapters) to return structured deposit metadata with separate transaction id and
shares received values, then thread that shape through the
YieldFarmingAggregator deposit flow so each field is populated from the
appropriate source instead of duplicating one string.
- Around line 308-314: Reject negative rebalance thresholds by validating
rebalanceThresholdPct in YieldFarmingAggregator’s constructor. In the
YieldFarmingAggregator constructor, before assigning this.rebalanceThresholdPct,
ensure options.rebalanceThresholdPct is either undefined or a finite number
greater than or equal to zero; otherwise throw an error. Keep the defaulting
behavior to DEFAULT_REBALANCE_THRESHOLD_PCT for missing values, and apply the
check directly in the constructor so invalid configs are blocked early.
- Around line 42-46: Keep YieldOpportunity.tvl in one consistent unit across all
adapters. The current contract in YieldOpportunity and the SoroswapPoolInfo
mapping are mixing native units and USD, so update yield-farming-aggregator.ts
to either normalize SoroswapPoolInfo.tvl to the same unit used by Blend/Vault or
change the exported YieldOpportunity contract and all adapter implementations
together. Focus on the YieldOpportunity type, SoroswapPoolInfo, and the adapter
mapping around the Soroswap forwarding logic so the public field remains
comparable for callers.
- Around line 407-432: The rebalance flow in yield-farming-aggregator.ts should
deposit based on the actual amounts returned by withdraw(), not the
caller-supplied totalAmount. Update rebalance() to accumulate the validated
withdrawn amounts from each withdraw(position.opportunity, position.amount)
call, verify the summed amount before proceeding, and pass that confirmed total
into deposit(best, ...) after exits. Use the rebalance(), withdraw(), and
deposit() symbols to locate the logic and keep the exit-and-enter sequence
consistent with the validated funds.

---

Nitpick comments:
In `@packages/core/automation/jest.config.cjs`:
- Around line 13-16: The Jest config in the testMatch array has a redundant glob
because the src/test pattern is already included by the broader src pattern.
Simplify the testMatch definition in the jest.config.cjs config by removing the
narrower '**/src/test/**/*.test.ts' entry and keeping the single broader glob,
using the testMatch symbol to locate the change.

In `@packages/core/automation/src/execution-chain/index.ts`:
- Around line 378-381: The trailing test-path comment in the execution-chain
module is stale and points to the wrong suite location. Update the comment near
the end of the file that references the Jest tests so it matches the actual test
file path used by the execution-chain suite, and keep the header comment in the
related test file consistent with that same location. Use the execution-chain
module comments and the test file header as the unique spots to correct.
- Around line 213-271: The execution chain currently uses step.label ??
step.type as the context key in ExecutionChain, so repeated steps of the same
type without explicit labels can overwrite earlier outputs in context.outputs.
Update the chain validation/execution flow in ExecutionChain.run (and related
helpers like logFailure/runWithRetry if needed) to enforce unique step keys up
front or automatically generate distinct labels for duplicate types, and keep
context.outputs keyed by that unique identifier so later steps cannot silently
clobber prior results.

In `@packages/core/automation/src/test/volume-trigger.test.ts`:
- Around line 240-250: The current tests only cover injected HorizonFetcher
mocks, so DefaultHorizonFetcher.fetchTrades still lacks direct coverage for its
production error handling. Add a focused test around DefaultHorizonFetcher that
mocks the global fetch response and verifies the non-ok status path (and, if
practical, the JSON parsing path) throws the expected error; use the
DefaultHorizonFetcher and fetchTrades symbols to locate the implementation.

In `@packages/core/automation/src/triggers/volume-trigger.ts`:
- Around line 212-217: The computeVolume method in volume-trigger.ts is summing
HorizonTrade base_amount values with parseFloat and plain JS numbers, which can
lose precision for on-chain amounts. Update computeVolume to use the project’s
financial-precision type (for example BigNumber/Decimal) for parsing and
accumulation, then convert only at the final threshold comparison point if
needed. Keep the method name computeVolume and its HorizonTrade reduce logic as
the place to update, but avoid floating-point arithmetic in the accumulation
path.

In `@packages/core/automation/tsconfig.json`:
- Around line 5-6: The tsconfig change is suppressing the TypeScript 6.0
`baseUrl` deprecation instead of addressing it. Remove `ignoreDeprecations` from
this config and update the module-resolution setup by eliminating `baseUrl` and
prefixing the existing `paths` entries accordingly in the same tsconfig. Keep
the `rootDir` adjustment as-is, and make sure the `paths` mappings still resolve
correctly without relying on `baseUrl`.

In
`@packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts`:
- Around line 99-108: The failing-source setup in YieldFarmingAggregator tests
is duplicating the source shape instead of using the existing mockSource helper.
Update mockSource to support an optional fetchOpportunities override or
rejection option, then replace the hand-rolled bad source in
yield-farming-aggregator.test.ts with that helper so the test stays consistent
with the rest of the suite.

In `@packages/core/defi-protocols/tsconfig.test.json`:
- Around line 1-15: The test TypeScript config is duplicating shared compiler
settings instead of inheriting them, which can cause drift from the main setup.
Update the test config to extend the shared base config like tsconfig.json does,
and keep only test-specific overrides in this file; use the existing
tsconfig.test.json structure and compilerOptions section to locate the
duplicated settings to remove.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 79cfae00-3cbf-4830-ada6-bbcdc7164bb0

📥 Commits

Reviewing files that changed from the base of the PR and between deb9ad9 and 4b58eb7.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (15)
  • packages/contracts/governance/Cargo.toml
  • packages/contracts/governance/src/lib.rs
  • packages/core/automation/jest.config.cjs
  • packages/core/automation/package.json
  • packages/core/automation/src/execution-chain/index.ts
  • packages/core/automation/src/test/execution.chain.test.ts
  • packages/core/automation/src/test/volume-trigger.test.ts
  • packages/core/automation/src/triggers/volume-trigger.ts
  • packages/core/automation/tsconfig.json
  • packages/core/automation/tsconfig.test.json
  • packages/core/defi-protocols/__tests__/services/yield-farming-aggregator.test.ts
  • packages/core/defi-protocols/jest.config.cjs
  • packages/core/defi-protocols/src/services/yield-farming-aggregator.ts
  • packages/core/defi-protocols/tsconfig.json
  • packages/core/defi-protocols/tsconfig.test.json

Comment on lines +148 to +156
// Validate token lock.
let locks: Map<Address, TokenLock> =
env.storage().instance().get(&LOCKS).unwrap_or(Map::new(&env));

let lock = locks.get(voter.clone()).expect("voter has no locked tokens");

if weight > lock.amount {
panic!("voting weight exceeds locked token balance");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

Locked weight is reusable across concurrent proposals.

Line 154 caps a single vote against lock.amount, but nothing deducts weight already committed to other active proposals. A wallet with 100 locked tokens can therefore cast 100 votes on every overlapping proposal, which breaks the contract’s own “prevents double-counting across proposals” guarantee and makes quorum/approval totals forgeable. Track reserved voting power per voter across active proposals, or snapshot/enforce available weight per proposal.

Also applies to: 174-209

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/contracts/governance/src/lib.rs` around lines 148 - 156, The vote
validation in the governance flow currently only compares the requested weight
against TokenLock.amount, so the same locked balance can be reused across
overlapping proposals. Update the proposal voting path in the relevant
governance functions (the validation around the lock check and the vote-casting
logic in the same module) to track and deduct already-committed weight per voter
across active proposals, or otherwise enforce a per-proposal available-weight
snapshot before accepting a vote. Make sure the fix is applied consistently
where votes are recorded so the reserved amount cannot be double-counted across
concurrent proposals.

Comment thread packages/contracts/governance/src/lib.rs Outdated
Comment on lines +282 to +309
pub fn lock_tokens(env: Env, voter: Address, amount: i128) {
voter.require_auth();

if amount <= 0 {
panic!("lock amount must be positive");
}

let mut locks: Map<Address, TokenLock> =
env.storage().instance().get(&LOCKS).unwrap_or(Map::new(&env));

// If a previous lock exists, accumulate rather than replace.
let existing_amount = locks
.get(voter.clone())
.map(|l| l.amount)
.unwrap_or(0);

let new_amount = existing_amount
.checked_add(amount)
.expect("lock amount overflow");

let lock = TokenLock {
voter: voter.clone(),
amount: new_amount,
locked_at: env.ledger().timestamp(),
};

locks.set(voter, lock);
env.storage().instance().set(&LOCKS, &locks);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

lock_tokens creates voting power out of thin air.

require_auth() proves who is calling, not that amount exists. As written, any address can call lock_tokens(voter, 1_000_000) and immediately satisfy quorum on its own because the contract never verifies or escrows a real governance token balance. This lock needs to be backed by a token transfer/balance check before it is counted as voting weight.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/contracts/governance/src/lib.rs` around lines 282 - 309,
`lock_tokens` is counting unbacked voting weight, so update the logic to ensure
the locked amount comes from a real governance token balance before storing it
in `LOCKS`. In `lock_tokens`, verify the caller/voter has sufficient tokens by
checking or transferring from the governance token contract, and only then
accumulate the lock amount and persist the `TokenLock`. Use the existing
`lock_tokens`, `LOCKS`, and `TokenLock` flow as the entry point, and make sure
the stored lock reflects escrowed value rather than just an authenticated
request.

Comment on lines +319 to +330
pub fn unlock_tokens(env: Env, voter: Address) {
voter.require_auth();

let mut locks: Map<Address, TokenLock> =
env.storage().instance().get(&LOCKS).unwrap_or(Map::new(&env));

if !locks.contains_key(voter.clone()) {
panic!("no token lock found for voter");
}

locks.remove(voter);
env.storage().instance().set(&LOCKS, &locks);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Unlock bypasses the active-vote lockup.

The doc comment says a voter must wait until every proposal they voted on is finalized, but this implementation just deletes the lock. That lets voting weight be released while those votes still count on active proposals. Reject unlock while any VoteRecord for voter points to an Active proposal.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/contracts/governance/src/lib.rs` around lines 319 - 330, The
unlock_tokens flow currently removes the voter’s TokenLock without checking
whether any of their votes are still tied to active proposals. Update
unlock_tokens to inspect the voter’s VoteRecord entries before calling
locks.remove, and reject the unlock if any referenced proposal is still Active.
Use the existing unlock_tokens, TokenLock, and VoteRecord symbols to locate the
logic, and ensure the lock is only deleted once all voted proposals are
finalized.

Comment thread packages/core/automation/jest.config.cjs
Comment on lines +1 to +16
{
"compilerOptions": {
"target": "ES2020",
"module": "CommonJS",
"moduleResolution": "node",
"esModuleInterop": true,
"resolveJsonModule": true,
"strict": true,
"skipLibCheck": true,
"declaration": false,
"outDir": "./dist-test",
"baseUrl": "."
},
"include": ["src/**/*"],
"exclude": ["node_modules", "dist"]
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "from ['\"]\.\./?.*\.js['\"]" packages/core/automation/src
rg -n "`@galaxy-kj/`" packages/core/automation/src

Repository: Galaxy-KJ/Galaxy-DevKit

Length of output: 3787


🏁 Script executed:

#!/bin/bash
set -euo pipefail

for f in packages/core/automation/tsconfig.json packages/core/automation/tsconfig.test.json packages/core/automation/jest.config.cjs; do
  echo "===== $f ====="
  cat -n "$f"
  echo
done

Repository: Galaxy-KJ/Galaxy-DevKit

Length of output: 3270


🌐 Web query:

TypeScript moduleResolution node10 .js import resolves to .ts extension substitution documentation

💡 Result:

In TypeScript, the resolution of an import path ending in a.js extension to a corresponding.ts source file is known as "file extension substitution" [1]. This behavior allows you to write import statements using the.js file extension—which is required by modern runtimes like Node.js for ECMAScript modules—while pointing to your.ts source files on disk [2][3]. During compilation, TypeScript looks for a file with the same name but with a.ts (or.tsx,.d.ts) extension when it encounters an import ending in.js [4]. Key points regarding this behavior and its configuration: 1. Consistency with Emit: TypeScript's design philosophy is that import paths in your source code should match the paths you want to appear in the final emitted JavaScript [3][5]. Because Node.js requires explicit extensions (e.g.,.js) in ESM, you should write these extensions in your TypeScript source [2][3]. 2. Module Resolution Strategies: - node10 (legacy): This mode was historically more permissive and did not strictly require extensions [6]. It is now deprecated and should be avoided in modern projects [6]. - node16 / nodenext: These modes strictly follow Node.js module resolution algorithms [7][8]. They require explicit extensions in import specifiers [2][3]. While TypeScript performs extension substitution to resolve these to.ts files during type checking, the emitted JavaScript maintains the.js extension, ensuring the code remains valid for the runtime [3][5]. - bundler: This mode is designed for use with bundlers (like Webpack, Vite, or esbuild) [7]. Unlike the Node.js modes, it generally does not require file extensions in relative imports and is more flexible, making it a common choice for frontend projects [7][6]. 3. Limitations: Note that while TypeScript can resolve these imports during development and compilation, runtimes that do not perform a build step (like raw Node.js without a loader) will not understand that a.js import should map to a.ts file [1]. In such environments, you may receive a module not found error unless you use tools that support in-memory transpilation (e.g., ts-node, tsx, or node --experimental-strip-types) [1]. 4. Configuration: You can configure your editor to automatically add the.js extension to imports by setting the following in your VS Code settings: "typescript.preferences.importModuleSpecifierEnding": "js" [3] As of mid-2026, TypeScript continues to prioritize the approach of writing the desired output extension in your source code to ensure compatibility across different module systems [6][3].

Citations:


Extend tsconfig.test.json from ./tsconfig.json
This test config drops the package paths mappings, so ts-jest won’t resolve workspace aliases like @galaxy-kj/core-oracles and @galaxy-kj/core-stellar-sdk used under src.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/automation/tsconfig.test.json` around lines 1 - 16, The test
TypeScript config is duplicating compiler settings instead of inheriting from
the package base config, which drops the shared path aliases needed by ts-jest.
Update tsconfig.test.json to extend the local tsconfig.json so it reuses the
existing compilerOptions, especially the workspace path mappings used by src
imports like `@galaxy-kj/core-oracles` and `@galaxy-kj/core-stellar-sdk`.

Comment on lines +42 to +46
/**
* Total value locked in the source, as a decimal string in the asset's
* native units. Useful for slippage estimation before depositing.
*/
tvl: string;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Keep tvl in one unit across all adapters.

YieldOpportunity.tvl is documented as the asset's native units, but SoroswapPoolInfo.tvl is explicitly USD and Line 199 forwards it unchanged. That makes tvl incomparable with Blend/Vault and breaks the public contract for any caller using it for liquidity/slippage checks. Normalize this field to a single unit across all sources, or change the exported contract before release.

Also applies to: 165-200

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/defi-protocols/src/services/yield-farming-aggregator.ts` around
lines 42 - 46, Keep YieldOpportunity.tvl in one consistent unit across all
adapters. The current contract in YieldOpportunity and the SoroswapPoolInfo
mapping are mixing native units and USD, so update yield-farming-aggregator.ts
to either normalize SoroswapPoolInfo.tvl to the same unit used by Blend/Vault or
change the exported YieldOpportunity contract and all adapter implementations
together. Focus on the YieldOpportunity type, SoroswapPoolInfo, and the adapter
mapping around the Soroswap forwarding logic so the public field remains
comparable for callers.

Comment on lines +176 to +177
addLiquidity(pairAddress: string, asset: string, amount: string): Promise<string>;
removeLiquidity(pairAddress: string, asset: string, lpTokens: string): Promise<string>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Return structured deposit metadata from the fetchers.

DepositResult promises both txId and sharesReceived, but these fetcher contracts only return one string. The Soroswap and Vault adapters currently copy that same value into both fields, so one of them is necessarily wrong.

Proposed contract shape
 export interface SoroswapPoolFetcher {
   getPools(): Promise<SoroswapPoolInfo[]>;
-  addLiquidity(pairAddress: string, asset: string, amount: string): Promise<string>;
+  addLiquidity(
+    pairAddress: string,
+    asset: string,
+    amount: string,
+  ): Promise<{ txId: string; lpTokens: string }>;
   removeLiquidity(pairAddress: string, asset: string, lpTokens: string): Promise<string>;
 }

-    const lpTokens = await this.fetcher.addLiquidity(
+    const { txId, lpTokens } = await this.fetcher.addLiquidity(
       opportunity.sourceId,
       opportunity.asset,
       amount,
     );
     return {
-      txId: lpTokens,
+      txId,
       amountDeposited: amount,
       sharesReceived: lpTokens,
       opportunity,
     };

 export interface VaultStrategyFetcher {
   getStrategies(): Promise<VaultStrategyInfo[]>;
-  deposit(contractId: string, amount: string): Promise<string>;
+  deposit(contractId: string, amount: string): Promise<{ txId: string; shares: string }>;
   withdraw(contractId: string, shares: string): Promise<string>;
 }

-    const shares = await this.fetcher.deposit(opportunity.sourceId, amount);
+    const { txId, shares } = await this.fetcher.deposit(opportunity.sourceId, amount);
     return {
-      txId: shares,
+      txId,
       amountDeposited: amount,
       sharesReceived: shares,
       opportunity,
     };

Also applies to: 204-215, 236-237, 261-267

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/defi-protocols/src/services/yield-farming-aggregator.ts` around
lines 176 - 177, The fetcher contract methods in YieldFarmingAggregator are
currently returning only a single string, which cannot satisfy DepositResult’s
txId and sharesReceived fields correctly. Update the affected adapter interfaces
and implementations (including the Soroswap and Vault adapters) to return
structured deposit metadata with separate transaction id and shares received
values, then thread that shape through the YieldFarmingAggregator deposit flow
so each field is populated from the appropriate source instead of duplicating
one string.

Comment on lines +308 to +314
constructor(
sources: YieldSource[] = [],
options: YieldFarmingAggregatorOptions = {},
) {
this.sources = sources;
this.rebalanceThresholdPct = options.rebalanceThresholdPct ?? DEFAULT_REBALANCE_THRESHOLD_PCT;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject negative rebalance thresholds.

A negative rebalanceThresholdPct makes gain.isLessThan(threshold) succeed even when the best opportunity yields less than the current one, so a bad config can rebalance into a worse APY. Validate this option as a finite, non-negative number in the constructor.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/defi-protocols/src/services/yield-farming-aggregator.ts` around
lines 308 - 314, Reject negative rebalance thresholds by validating
rebalanceThresholdPct in YieldFarmingAggregator’s constructor. In the
YieldFarmingAggregator constructor, before assigning this.rebalanceThresholdPct,
ensure options.rebalanceThresholdPct is either undefined or a finite number
greater than or equal to zero; otherwise throw an error. Keep the defaulting
behavior to DEFAULT_REBALANCE_THRESHOLD_PCT for missing values, and apply the
check directly in the constructor so invalid configs are blocked early.

Comment on lines +407 to +432
async rebalance(
currentPositions: Array<{ opportunity: YieldOpportunity; amount: string }>,
totalAmount: string,
asset: string,
): Promise<RebalanceResult | null> {
if (currentPositions.length === 0) return null;

const best = await this.getBestOpportunity(asset);
if (!best) return null;

const currentMaxApy = Math.max(...currentPositions.map((p) => p.opportunity.apy));
const gain = new BigNumber(best.apy).minus(currentMaxApy);

if (gain.isLessThan(this.rebalanceThresholdPct)) {
return null; // improvement below threshold — do nothing
}

// Exit all current positions
const exited: RebalanceResult['exited'] = [];
for (const position of currentPositions) {
const withdrawn = await this.withdraw(position.opportunity, position.amount);
exited.push({ opportunity: position.opportunity, amountWithdrawn: withdrawn });
}

// Enter the best opportunity with the full amount
const depositResult = await this.deposit(best, totalAmount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Base the rebalance deposit on validated withdrawn funds, not totalAmount.

withdraw() returns the actual amount withdrawn, but those values are ignored and Line 432 deposits the caller-supplied totalAmount after all exits. If a withdrawal is short because of fees/rounding — or totalAmount is malformed — this can unwind positions and then fail or over-deposit.

Proposed fix
   async rebalance(
     currentPositions: Array<{ opportunity: YieldOpportunity; amount: string }>,
     totalAmount: string,
     asset: string,
   ): Promise<RebalanceResult | null> {
+    this.validateAmount(totalAmount);
+    currentPositions.forEach((p) => this.validateAmount(p.amount));
     if (currentPositions.length === 0) return null;

     const best = await this.getBestOpportunity(asset);
     if (!best) return null;
@@
     // Exit all current positions
     const exited: RebalanceResult['exited'] = [];
+    let withdrawnTotal = new BigNumber(0);
     for (const position of currentPositions) {
       const withdrawn = await this.withdraw(position.opportunity, position.amount);
       exited.push({ opportunity: position.opportunity, amountWithdrawn: withdrawn });
+      withdrawnTotal = withdrawnTotal.plus(withdrawn);
     }

     // Enter the best opportunity with the full amount
-    const depositResult = await this.deposit(best, totalAmount);
+    const depositResult = await this.deposit(best, withdrawnTotal.toFixed());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async rebalance(
currentPositions: Array<{ opportunity: YieldOpportunity; amount: string }>,
totalAmount: string,
asset: string,
): Promise<RebalanceResult | null> {
if (currentPositions.length === 0) return null;
const best = await this.getBestOpportunity(asset);
if (!best) return null;
const currentMaxApy = Math.max(...currentPositions.map((p) => p.opportunity.apy));
const gain = new BigNumber(best.apy).minus(currentMaxApy);
if (gain.isLessThan(this.rebalanceThresholdPct)) {
return null; // improvement below threshold — do nothing
}
// Exit all current positions
const exited: RebalanceResult['exited'] = [];
for (const position of currentPositions) {
const withdrawn = await this.withdraw(position.opportunity, position.amount);
exited.push({ opportunity: position.opportunity, amountWithdrawn: withdrawn });
}
// Enter the best opportunity with the full amount
const depositResult = await this.deposit(best, totalAmount);
async rebalance(
currentPositions: Array<{ opportunity: YieldOpportunity; amount: string }>,
totalAmount: string,
asset: string,
): Promise<RebalanceResult | null> {
this.validateAmount(totalAmount);
currentPositions.forEach((p) => this.validateAmount(p.amount));
if (currentPositions.length === 0) return null;
const best = await this.getBestOpportunity(asset);
if (!best) return null;
const currentMaxApy = Math.max(...currentPositions.map((p) => p.opportunity.apy));
const gain = new BigNumber(best.apy).minus(currentMaxApy);
if (gain.isLessThan(this.rebalanceThresholdPct)) {
return null; // improvement below threshold — do nothing
}
// Exit all current positions
const exited: RebalanceResult['exited'] = [];
let withdrawnTotal = new BigNumber(0);
for (const position of currentPositions) {
const withdrawn = await this.withdraw(position.opportunity, position.amount);
exited.push({ opportunity: position.opportunity, amountWithdrawn: withdrawn });
withdrawnTotal = withdrawnTotal.plus(withdrawn);
}
// Enter the best opportunity with the full amount
const depositResult = await this.deposit(best, withdrawnTotal.toFixed());
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/defi-protocols/src/services/yield-farming-aggregator.ts` around
lines 407 - 432, The rebalance flow in yield-farming-aggregator.ts should
deposit based on the actual amounts returned by withdraw(), not the
caller-supplied totalAmount. Update rebalance() to accumulate the validated
withdrawn amounts from each withdraw(position.opportunity, position.amount)
call, verify the summed amount before proceeding, and pass that confirmed total
into deposit(best, ...) after exits. Use the rebalance(), withdraw(), and
deposit() symbols to locate the logic and keep the exit-and-enter sequence
consistent with the validated funds.

@KevinMB0220 KevinMB0220 self-requested a review July 2, 2026 20:00
@sotoJ24

sotoJ24 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author
Screenshot from 2026-07-02 17-22-16

@KevinMB0220 KevinMB0220 merged commit 592aa6c into Galaxy-KJ:main Jul 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] DeFi: Implement yield farming aggregator service (#43)

2 participants