feat: add integration tests, risk scoring, timelock, and vesting cont…#325
Conversation
📝 WalkthroughWalkthroughThis PR adds two new Soroban smart contracts ( ChangesSoroban Timelock Contract
Soroban Vesting Contract
Wallet Risk Scoring Service
Integration Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/core/wallet/src/index.tsOops! Something went wrong! :( ESLint: 9.39.2 YAMLException: Cannot read config file: /.eslintrc.js.bak 1 | module.exports = { packages/core/wallet/src/risk-score.service.tsOops! Something went wrong! :( ESLint: 9.39.2 YAMLException: Cannot read config file: /.eslintrc.js.bak 1 | module.exports = { packages/core/wallet/src/tests/risk-score.service.test.tsOops! Something went wrong! :( ESLint: 9.39.2 YAMLException: Cannot read config file: /.eslintrc.js.bak 1 | module.exports = {
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. Comment |
|
@ExcelDsigN-tech fix the conflicts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (15)
packages/integration-tests/tsconfig.json (2)
10-12: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
excludearray to preventdist/andnode_modules/from being compiled.Without
exclude, TypeScript may attempt to process files indist/ornode_modules/whenrootDiris., especially withdeclaration: true."declaration": true, "resolveJsonModule": true - } + }, + "exclude": ["node_modules", "dist", "**/*.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/integration-tests/tsconfig.json` around lines 10 - 12, The TypeScript config for the integration tests is missing an exclude list, so the compiler can pick up generated output and dependencies when rootDir is set to ".". Update the tsconfig in the integration-tests package to add an exclude array that skips dist/ and node_modules/, keeping the existing compiler options like declaration and resolveJsonModule unchanged.
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider disabling
declarationfor integration test package.
declaration: truegenerates.d.tsfiles, which is unusual for an integration test package that isprivate: trueand not meant to be consumed as a library.🤖 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/integration-tests/tsconfig.json` at line 11, Disable declaration output in the integration test package tsconfig by changing the "declaration" setting in the integration-test config, since this private package is not intended to publish or consume generated type files. Update the tsconfig entry that currently enables declarations so the test package compiles without emitting .d.ts artifacts.
packages/integration-tests/src/defi.test.ts (3)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace hardcoded Friendbot URL with
fundAccountutility.The test duplicates the Friendbot URL and funding logic already defined in
setup/testnet.ts. Import and reusefundAccountto reduce duplication and ensure consistent error handling.describe('Friendbot Account Funding', () => { it('should fund a test account via Friendbot', async () => { const keypair = Keypair.random(); - const response = await fetch( - `https://friendbot-testnet.stellar.org?addr=${keypair.publicKey()}` - ); - expect(response.ok).toBe(true); - const result = await response.json(); - expect(result).toHaveProperty('hash'); + await fundAccount(keypair.publicKey()); + // Verify account exists on horizon + const horizon = new Horizon.Server(env.horizonUrl); + const account = await horizon.loadAccount(keypair.publicKey()); + expect(account.account_id).toBe(keypair.publicKey()); }, 30000); });
🤖 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/integration-tests/src/defi.test.ts` around lines 33 - 43, The
Friendbot funding test duplicates the URL and request logic that already exists
in `fundAccount`, so update the `Friendbot Account Funding` test to import and
call that utility instead of doing a direct `fetch`. Keep the test’s assertions
focused on the returned funded account behavior, and rely on `fundAccount` from
`setup/testnet.ts` for the shared Friendbot endpoint and error handling.
65-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Soroswap test is a local math check, not a protocol integration.
The Soroswap Integration test computes a constant-product swap locally without interacting with any Soroswap contract or pool. Rename to Soroswap Swap Math Validation or implement actual contract interaction via RPC.
🤖 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/integration-tests/src/defi.test.ts` around lines 65 - 84, The
Soroswap test in describe('Soroswap Integration') is only validating local
constant-product math and does not perform any real protocol interaction. Either
rename the test block and it() case to reflect that it is a swap math
validation, or update the test to use actual Soroswap contract/pool calls via
the existing integration test setup and RPC so the name matches the behavior.
28-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
RPC server instance check doesn't verify connectivity.
expect(env.server).toBeInstanceOf(rpc.Server) only checks object construction, not that the server is reachable or the testnet is healthy. Add a lightweight health check like server.getHealth() or server.getLatestLedger().
it('should have a valid RPC server connection', async () => {
- expect(env.server).toBeInstanceOf(rpc.Server);
+ expect(env.server).toBeInstanceOf(rpc.Server);
+ const health = await env.server.getHealth();
+ expect(health.status).toBe('healthy');
});🤖 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/integration-tests/src/defi.test.ts` around lines 28 - 30, The
current RPC assertion in the defi test only checks that env.server is
constructed as an rpc.Server, which does not confirm the endpoint is actually
reachable. Update the test around env.server to perform a lightweight
connectivity/health verification using an existing RPC method such as
getHealth() or getLatestLedger(), and assert on the returned result instead of
relying only on toBeInstanceOf. Keep the check minimal and tied to the same
env.server symbol so the test validates the live testnet connection.
packages/integration-tests/setup/testnet.ts (4)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused imports
Account,TransactionBuilder, andBASE_FEE.These imports are not referenced in the module body and increase bundle size and confusion.
import { Keypair, Horizon, rpc, - BASE_FEE, - TransactionBuilder, Networks, - Account, } from '`@stellar/stellar-sdk`';
🤖 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/integration-tests/setup/testnet.ts` around lines 1 - 9, Remove the
unused `Account`, `TransactionBuilder`, and `BASE_FEE` imports from the
`testnet.ts` module. Keep the remaining `@stellar/stellar-sdk` imports that are
actually used, and ensure no references to those symbols remain in the file.
49-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add timeout and retry to fundAccount.
The fetch call lacks an AbortController timeout and retry logic. Friendbot rate-limiting causes flaky integration tests.
export async function fundAccount(publicKey: string): Promise<void> {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 30000);
+ try {
- const response = await fetch(`${FRIENDBOT_URL}?addr=${publicKey}`);
+ const response = await fetch(`${FRIENDBOT_URL}?addr=${publicKey}`, {
+ signal: controller.signal,
+ });
+ clearTimeout(timeout);
- if (!response.ok) {
+ if (!response.ok) {
- throw new Error(
+ throw new Error(
- `Friendbot funding failed: ${response.status} ${response.statusText}`
+ `Friendbot funding failed: ${response.status} ${response.statusText}`
- );
+ );
+ }
+ } catch (err) {
+ clearTimeout(timeout);
+ throw err;
}
}🤖 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/integration-tests/setup/testnet.ts` around lines 49 - 56, The
fundAccount helper currently calls fetch without any timeout or retry handling,
which makes Friendbot-backed test setup flaky under rate limiting. Update
fundAccount to use an AbortController-based timeout around the fetch request and
add limited retry logic for transient failures/non-ok responses; keep the
existing Friendbot URL and error reporting in fundAccount, but make the request
resilient before throwing the final funding failure error.
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Refactor setupTestnetEnv to reuse createTestEnv and fundAccount.
setupTestnetEnv duplicates default value construction from createTestEnv and reimplements the Friendbot funding logic already in fundAccount. Deduplicate to reduce maintenance burden.
export async function setupTestnetEnv(): Promise<IntegrationTestEnv> {
const testAccount = Keypair.random();
await fundAccount(testAccount.publicKey());
- const networkPassphrase = Networks.TESTNET;
- const server = new rpc.Server(TESTNET_RPC_URL);
-
- const response = await fetch(
- `${FRIENDBOT_URL}?addr=${testAccount.publicKey()}`
- );
- if (!response.ok) {
- throw new Error(
- `Friendbot funding failed: ${response.status} ${response.statusText}`
- );
- }
-
const horizon = new Horizon.Server(TESTNET_HORIZON_URL);
await horizon.loadAccount(testAccount.publicKey());
- return {
- testAccount,
- networkPassphrase,
- rpcUrl: TESTNET_RPC_URL,
- horizonUrl: TESTNET_HORIZON_URL,
- server,
- };
+ return createTestEnv({ testAccount });
}🤖 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/integration-tests/setup/testnet.ts` around lines 58 - 68,
`setupTestnetEnv` is duplicating the default environment setup from
`createTestEnv` and the Friendbot funding flow from `fundAccount`. Update
`setupTestnetEnv` to build its base values by calling `createTestEnv` with any
overrides, then reuse `fundAccount` for the test account funding instead of
reimplementing the request logic. Keep the existing `createTestEnv`,
`fundAccount`, and `setupTestnetEnv` symbols as the main integration points so
the testnet setup stays centralized.
23-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add timeout and retry logic to Friendbot funding calls.
Friendbot can be rate-limited or experience transient failures. The current fetch has no timeout and no retry, which can cause tests to hang or fail flakily.
export async function setupTestnetEnv(): Promise<IntegrationTestEnv> {
const testAccount = Keypair.random();
const networkPassphrase = Networks.TESTNET;
const server = new rpc.Server(TESTNET_RPC_URL);
- const response = await fetch(
- `${FRIENDBOT_URL}?addr=${testAccount.publicKey()}`
- );
- if (!response.ok) {
- throw new Error(
- `Friendbot funding failed: ${response.status} ${response.statusText}`
- );
- }
+ await fundAccount(testAccount.publicKey());🤖 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/integration-tests/setup/testnet.ts` around lines 23 - 47, The
Friendbot funding call in setupTestnetEnv is currently a single fetch with no
resilience, so add timeout and retry handling around the
`${FRIENDBOT_URL}?addr=${testAccount.publicKey()}` request. Use setupTestnetEnv
as the entry point and wrap the existing fetch in a small retry loop with
backoff plus an AbortController-based timeout, then keep the existing error path
when all attempts fail. Ensure the server, horizon, and account loading flow
remain unchanged after a successful funding response.
packages/integration-tests/jest.config.cjs (1)
6-18: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding
setupFilesAfterEnvfor integration test lifecycle management.Integration tests against live testnet may need global teardown for resource cleanup or shared setup for RPC rate limiting. Consider adding a setup file for common initialization.
🤖 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/integration-tests/jest.config.cjs` around lines 6 - 18, The integration test Jest config currently lacks shared lifecycle hooks for testnet-facing tests. Update the module.exports config in jest.config.cjs to add setupFilesAfterEnv pointing to a setup file that can handle common initialization and global teardown for the integration suite. Use the existing Jest config keys (like testEnvironment and testMatch) as the place to integrate this lifecycle management without changing the test discovery behavior.
packages/integration-tests/src/oracle.test.ts (2)
5-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
envor convert to actual oracle integration tests.The
envvariable is initialized viacreateTestEnv()but never referenced. These are unit tests, not integration tests. Either remove the unused setup or implement actual price feed RPC calls.🤖 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/integration-tests/src/oracle.test.ts` around lines 5 - 10, The Oracle Integration test setup in describe('Oracle Integration') initializes env with createTestEnv() but never uses it, so either remove the unused env and the beforeAll setup entirely or turn the suite into real oracle integration coverage by using env for actual price feed RPC calls. If you keep the suite, ensure the test cases reference env and exercise the intended integration path instead of leaving it as a unit-style placeholder.
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename TWAP test to "simple average" or implement true TWAP.
The test computes an arithmetic mean, not a time-weighted average price. True TWAP weights prices by their duration. Rename to avoid misleading readers, or implement actual TWAP with time weights.
- it('should compute TWAP correctly', () => { + it('should compute simple average correctly', () => {
🤖 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/integration-tests/src/oracle.test.ts` around lines 51 - 56, The test
in the TWAP case is calculating a plain arithmetic mean rather than a
time-weighted average, so update the expectation to match the behavior or rename
the test to reflect a simple average. Locate the spec in the oracle test suite
by the describe/it block for TWAP and either implement actual time weighting in
the calculation or change the test name and assertions so they no longer claim
to verify TWAP.
packages/integration-tests/package.json (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
@types/jestmay conflict with Jest 30 built-in types.Jest 30 includes its own TypeScript types. Installing
@types/jestseparately can cause type conflicts. Verify if@types/jestis needed or if it should be removed.🤖 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/integration-tests/package.json` at line 16, The integration-tests package is still declaring `@types/jest`, which can conflict with Jest 30’s built-in TypeScript types. Check the package.json dependencies for the integration-tests package and remove `@types/jest` if the project is already relying on Jest 30’s bundled typings; only keep it if there is a specific compatibility reason. Make sure any remaining TypeScript/Jest setup in the test package continues to resolve types through Jest 30’s native definitions.
packages/contracts/timelock/test_snapshots/test/test_execute_removes_from_queue_and_marks_executed.1.json (1)
1-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese success-path snapshots are duplicating the same test.
This file appears identical to
test_execute_transaction_after_eta.1.json, and the supplied test bodies inpackages/contracts/timelock/src/test.rs:47-66andpackages/contracts/timelock/src/test.rs:127-145assert the same queue/execute/post-state flow. Keeping both doubles snapshot churn without adding coverage; either merge them or make one verify a distinct invariant.🤖 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/timelock/test_snapshots/test/test_execute_removes_from_queue_and_marks_executed.1.json` around lines 1 - 307, The success-path snapshot is redundant because it matches the existing execute-after-ETA snapshot and covers the same queue/execute/post-state flow. Update the timelock tests by either removing this duplicate snapshot or changing one of the related test cases in test_execute_removes_from_queue_and_marks_executed / test_execute_transaction_after_eta to assert a different invariant so each snapshot has unique coverage.
packages/contracts/timelock/test_snapshots/test/test_queue_transaction_past_eta_panics.1.json (1)
124-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid snapshotting Soroban internal object ids.
This snapshot bakes in
Symbol(obj#9), and the other panic snapshots do the same with different ids. Those ids are runtime internals, not contract behavior, so they can churn on SDK/protocol upgrades without any real regression. Prefer asserting on the stable panic message or redacting theSymbol(obj#...)suffix before snapshotting.🤖 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/timelock/test_snapshots/test/test_queue_transaction_past_eta_panics.1.json` around lines 124 - 127, The panic snapshot is including Soroban runtime internals via the contract function suffix `Symbol(obj#...)`, which is unstable and can change across SDK/protocol versions. Update the snapshot generation in the timelock panic test so it asserts only on the stable panic text from the relevant test helper/output, or redact the `Symbol(obj#...)` portion before writing snapshots. Apply the same treatment anywhere the test snapshots serialize panic messages for this contract so only behaviorally meaningful text remains.
🤖 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/timelock/src/lib.rs`:
- Around line 13-18: The timelock queue currently stores only metadata in
QueuedTransaction, so execute_transaction can mark an item executed without ever
performing the delayed call. Update the timelock flow around QueuedTransaction
and execute_transaction to store and consume the full call payload (including
target, action, and any needed arguments) and actually dispatch it when
executing a queued entry, rather than only removing it from the queue. Ensure
the execution path in the timelock contract uses the stored transaction fields
to invoke the intended action before recording it as executed.
- Around line 43-51: The timelock queued/executed state stored via QUEUED_TXS
and EXECUTED in instance storage can expire before delayed actions are
processed, so update the timelock flow to either move this state to persistent
storage or extend its TTL whenever it is accessed or mutated. Apply the fix
consistently across queue_transaction, execute_transaction, revoke_transaction,
get_queued_transaction, and is_executed, and use the existing storage access
points in lib.rs to ensure queued transactions remain available until they are
executed or revoked.
In `@packages/contracts/timelock/src/test.rs`:
- Around line 48-65: The execute-path tests for TimelockContractClient currently
only check bookkeeping via is_executed and get_queued_transaction, so a no-op
execute_transaction can still pass. Update one of the affected tests in
test_execute_transaction_after_eta (and the similar case near the other cited
test block) to invoke execute_transaction against a dummy target contract
through the target Address and action, then assert the target contract’s state
actually changed; keep the existing queue/removal checks as secondary
validation.
In `@packages/contracts/vesting/src/lib.rs`:
- Around line 72-82: The vesting math in the claim logic is starting accrual
from schedule.start instead of after the cliff, so tokens vest during the locked
cliff window. Update the elapsed-time calculation in the claim path to base
vesting on schedule.start + schedule.cliff, and compute vested amount over the
remaining window (schedule.duration - schedule.cliff) rather than the full
duration. Make the same adjustment in the duplicated vesting block referenced by
the claim calculation so both paths use the cliff-adjusted start and release
span.
- Around line 12-18: `claim_tokens` currently only updates `CLAIMD` and returns
an amount, so no actual token transfer happens. Update the vesting flow around
`VestingSchedule` and `claim_tokens` to include a real token source and transfer
mechanism, such as storing the token contract/address or escrow account in the
schedule/state and invoking the appropriate transfer call when a claim succeeds.
Make sure the claimed amount is deducted from the escrow/source balance and that
`claim_tokens` performs the release action rather than just accounting updates.
- Around line 25-60: The vesting entrypoints are missing an authorization model,
so any caller can create or advance state for an arbitrary beneficiary. Update
create_schedule in vesting::create_schedule to require issuer/admin
authorization before mutating SCHEDULES, and ensure the claim flow in the
related claim entrypoints (the CLAIMD-related paths in the same module) either
requires beneficiary auth or sends funds directly to the stored
VestingSchedule.beneficiary. Use the existing create_schedule and claim-related
symbols to centralize the auth checks so unauthorized accounts cannot pre-create
or block schedules.
In `@packages/core/wallet/src/risk-score.service.ts`:
- Around line 7-14: The WalletRiskInput contract and risk scoring flow are
inconsistent because risk uses a caller-supplied leverageRatio even though
totalCollateralUSD and totalDebtUSD are already available. Update
risk-score.service.ts so WalletRiskInput and the scoring logic in the
service/class that consumes it derive leverage internally from the balance
fields, or else remove the redundant balance fields from the public input
contract; make the scoring path use a single source of truth in the method that
computes the wallet classification.
- Around line 71-99: The `collectContributingFactors` logic in
`RiskScoreService` can return an empty `factors` array even when the overall
risk is elevated, because it only adds entries when a normalized component is
strictly greater than 50. Update the thresholding in
`collectContributingFactors` so that high-risk profiles always surface the top
contributors, even when the normalized values are exactly at the boundary. Keep
the factor messages based on the existing `input` fields and ensure the returned
array is never empty for non-low risk outcomes.
In `@packages/core/wallet/src/tests/risk-score.service.test.ts`:
- Around line 10-136: The evaluateWalletRisk test suite in
risk-score.service.test.ts only checks broad score ranges, so changes to
weighting or the 25/50/75 cutoff behavior could slip through. Update the tests
around service.evaluateWalletRisk to assert exact scores for the known scenarios
and add dedicated boundary cases that verify the transition behavior at 24→25,
49→50, and 74→75, including the riskLevel and factors produced at each
threshold.
In `@packages/integration-tests/jest.config.cjs`:
- Line 10: The Jest config currently defines only test discovery via testMatch
and does not enforce the 90%+ coverage objective. Update the jest.config.cjs
settings by adding a coverageThreshold so the integration test suite fails when
coverage drops below the target, using the existing Jest config object in this
file as the place to locate and extend the coverage settings.
In `@packages/integration-tests/src/defi.test.ts`:
- Around line 1-6: Integration tests are importing and using the mock
environment helper instead of the real testnet setup; update the test setup in
defi.test.ts to rely on setupTestnetEnv and remove createTestEnv usage. Make
sure the test harness still initializes through the setup/testnet.js helpers and
any references to IntegrationTestEnv are wired to the funded, RPC-verified
environment so the tests exercise real testnet connectivity.
- Around line 45-63: The Blend Protocol Integration block in the test suite is
only validating hardcoded float comparisons, so either rename the describe block
and its test names to reflect placeholder math coverage, or replace the
assertions with a real Blend SDK/RPC integration flow that exercises
`@blend-capital/blend-sdk` instead of local parsing logic. Update the relevant
describe block and the two it cases so the naming matches the actual behavior
being tested.
In `@packages/integration-tests/src/oracle.test.ts`:
- Around line 25-36: The test case in oracle.test.ts has a mismatch between its
name and the assertion logic: the current `should detect price deviation
anomaly` title implies a positive anomaly result, but `isAnomaly` is expected to
be false when `deviationThreshold` equals the computed deviation. Update the
test in the relevant `it(...)` block to either rename it to reflect the
exact-threshold non-anomaly behavior or adjust `deviationThreshold` so the
`isAnomaly` expectation matches the intended anomaly detection scenario.
---
Nitpick comments:
In
`@packages/contracts/timelock/test_snapshots/test/test_execute_removes_from_queue_and_marks_executed.1.json`:
- Around line 1-307: The success-path snapshot is redundant because it matches
the existing execute-after-ETA snapshot and covers the same
queue/execute/post-state flow. Update the timelock tests by either removing this
duplicate snapshot or changing one of the related test cases in
test_execute_removes_from_queue_and_marks_executed /
test_execute_transaction_after_eta to assert a different invariant so each
snapshot has unique coverage.
In
`@packages/contracts/timelock/test_snapshots/test/test_queue_transaction_past_eta_panics.1.json`:
- Around line 124-127: The panic snapshot is including Soroban runtime internals
via the contract function suffix `Symbol(obj#...)`, which is unstable and can
change across SDK/protocol versions. Update the snapshot generation in the
timelock panic test so it asserts only on the stable panic text from the
relevant test helper/output, or redact the `Symbol(obj#...)` portion before
writing snapshots. Apply the same treatment anywhere the test snapshots
serialize panic messages for this contract so only behaviorally meaningful text
remains.
In `@packages/integration-tests/jest.config.cjs`:
- Around line 6-18: The integration test Jest config currently lacks shared
lifecycle hooks for testnet-facing tests. Update the module.exports config in
jest.config.cjs to add setupFilesAfterEnv pointing to a setup file that can
handle common initialization and global teardown for the integration suite. Use
the existing Jest config keys (like testEnvironment and testMatch) as the place
to integrate this lifecycle management without changing the test discovery
behavior.
In `@packages/integration-tests/package.json`:
- Line 16: The integration-tests package is still declaring `@types/jest`, which
can conflict with Jest 30’s built-in TypeScript types. Check the package.json
dependencies for the integration-tests package and remove `@types/jest` if the
project is already relying on Jest 30’s bundled typings; only keep it if there
is a specific compatibility reason. Make sure any remaining TypeScript/Jest
setup in the test package continues to resolve types through Jest 30’s native
definitions.
In `@packages/integration-tests/setup/testnet.ts`:
- Around line 1-9: Remove the unused `Account`, `TransactionBuilder`, and
`BASE_FEE` imports from the `testnet.ts` module. Keep the remaining
`@stellar/stellar-sdk` imports that are actually used, and ensure no references
to those symbols remain in the file.
- Around line 49-56: The fundAccount helper currently calls fetch without any
timeout or retry handling, which makes Friendbot-backed test setup flaky under
rate limiting. Update fundAccount to use an AbortController-based timeout around
the fetch request and add limited retry logic for transient failures/non-ok
responses; keep the existing Friendbot URL and error reporting in fundAccount,
but make the request resilient before throwing the final funding failure error.
- Around line 58-68: `setupTestnetEnv` is duplicating the default environment
setup from `createTestEnv` and the Friendbot funding flow from `fundAccount`.
Update `setupTestnetEnv` to build its base values by calling `createTestEnv`
with any overrides, then reuse `fundAccount` for the test account funding
instead of reimplementing the request logic. Keep the existing `createTestEnv`,
`fundAccount`, and `setupTestnetEnv` symbols as the main integration points so
the testnet setup stays centralized.
- Around line 23-47: The Friendbot funding call in setupTestnetEnv is currently
a single fetch with no resilience, so add timeout and retry handling around the
`${FRIENDBOT_URL}?addr=${testAccount.publicKey()}` request. Use setupTestnetEnv
as the entry point and wrap the existing fetch in a small retry loop with
backoff plus an AbortController-based timeout, then keep the existing error path
when all attempts fail. Ensure the server, horizon, and account loading flow
remain unchanged after a successful funding response.
In `@packages/integration-tests/src/defi.test.ts`:
- Around line 33-43: The Friendbot funding test duplicates the URL and request
logic that already exists in `fundAccount`, so update the `Friendbot Account
Funding` test to import and call that utility instead of doing a direct `fetch`.
Keep the test’s assertions focused on the returned funded account behavior, and
rely on `fundAccount` from `setup/testnet.ts` for the shared Friendbot endpoint
and error handling.
- Around line 65-84: The Soroswap test in describe('Soroswap Integration') is
only validating local constant-product math and does not perform any real
protocol interaction. Either rename the test block and it() case to reflect that
it is a swap math validation, or update the test to use actual Soroswap
contract/pool calls via the existing integration test setup and RPC so the name
matches the behavior.
- Around line 28-30: The current RPC assertion in the defi test only checks that
env.server is constructed as an rpc.Server, which does not confirm the endpoint
is actually reachable. Update the test around env.server to perform a
lightweight connectivity/health verification using an existing RPC method such
as getHealth() or getLatestLedger(), and assert on the returned result instead
of relying only on toBeInstanceOf. Keep the check minimal and tied to the same
env.server symbol so the test validates the live testnet connection.
In `@packages/integration-tests/src/oracle.test.ts`:
- Around line 5-10: The Oracle Integration test setup in describe('Oracle
Integration') initializes env with createTestEnv() but never uses it, so either
remove the unused env and the beforeAll setup entirely or turn the suite into
real oracle integration coverage by using env for actual price feed RPC calls.
If you keep the suite, ensure the test cases reference env and exercise the
intended integration path instead of leaving it as a unit-style placeholder.
- Around line 51-56: The test in the TWAP case is calculating a plain arithmetic
mean rather than a time-weighted average, so update the expectation to match the
behavior or rename the test to reflect a simple average. Locate the spec in the
oracle test suite by the describe/it block for TWAP and either implement actual
time weighting in the calculation or change the test name and assertions so they
no longer claim to verify TWAP.
In `@packages/integration-tests/tsconfig.json`:
- Around line 10-12: The TypeScript config for the integration tests is missing
an exclude list, so the compiler can pick up generated output and dependencies
when rootDir is set to ".". Update the tsconfig in the integration-tests package
to add an exclude array that skips dist/ and node_modules/, keeping the existing
compiler options like declaration and resolveJsonModule unchanged.
- Line 11: Disable declaration output in the integration test package tsconfig
by changing the "declaration" setting in the integration-test config, since this
private package is not intended to publish or consume generated type files.
Update the tsconfig entry that currently enables declarations so the test
package compiles without emitting .d.ts artifacts.
🪄 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: fc4ee1c7-2b39-4c25-9514-d34817f8aed8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
package.jsonpackages/contracts/timelock/Cargo.tomlpackages/contracts/timelock/src/lib.rspackages/contracts/timelock/src/test.rspackages/contracts/timelock/test_snapshots/test/test_double_execute_panics.1.jsonpackages/contracts/timelock/test_snapshots/test/test_execute_before_eta_panics.1.jsonpackages/contracts/timelock/test_snapshots/test/test_execute_removes_from_queue_and_marks_executed.1.jsonpackages/contracts/timelock/test_snapshots/test/test_execute_transaction_after_eta.1.jsonpackages/contracts/timelock/test_snapshots/test/test_queue_multiple_transactions.1.jsonpackages/contracts/timelock/test_snapshots/test/test_queue_transaction.1.jsonpackages/contracts/timelock/test_snapshots/test/test_queue_transaction_past_eta_panics.1.jsonpackages/contracts/timelock/test_snapshots/test/test_revoke_after_eta_panics.1.jsonpackages/contracts/timelock/test_snapshots/test/test_revoke_transaction_before_eta.1.jsonpackages/contracts/vesting/Cargo.tomlpackages/contracts/vesting/src/lib.rspackages/contracts/vesting/src/test.rspackages/contracts/vesting/test_snapshots/test/test_claim_during_cliff_panics.1.jsonpackages/contracts/vesting/test_snapshots/test/test_claim_tokens.1.jsonpackages/contracts/vesting/test_snapshots/test/test_cliff_period_blocks_claim.1.jsonpackages/contracts/vesting/test_snapshots/test/test_create_schedule.1.jsonpackages/contracts/vesting/test_snapshots/test/test_create_schedule_zero_amount_panics.1.jsonpackages/contracts/vesting/test_snapshots/test/test_full_claim_at_maturity.1.jsonpackages/contracts/vesting/test_snapshots/test/test_full_vesting_after_duration.1.jsonpackages/contracts/vesting/test_snapshots/test/test_multiple_claims_accumulate.1.jsonpackages/contracts/vesting/test_snapshots/test/test_partial_claim.1.jsonpackages/core/wallet/src/index.tspackages/core/wallet/src/risk-score.service.tspackages/core/wallet/src/tests/risk-score.service.test.tspackages/integration-tests/jest.config.cjspackages/integration-tests/package.jsonpackages/integration-tests/setup/testnet.tspackages/integration-tests/src/defi.test.tspackages/integration-tests/src/oracle.test.tspackages/integration-tests/tsconfig.json
| pub struct QueuedTransaction { | ||
| pub target: Address, | ||
| pub action: String, | ||
| pub eta: u64, | ||
| pub queued_at: u64, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Executing a queued transaction currently does not execute anything.
execute_transaction only removes the queue entry and records it as executed; the stored target and action are never consumed, and the queued payload does not include any callable arguments. That means the contract can pass its tests while never performing the delayed action the timelock is meant to gate.
Also applies to: 56-77
🤖 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/timelock/src/lib.rs` around lines 13 - 18, The timelock
queue currently stores only metadata in QueuedTransaction, so
execute_transaction can mark an item executed without ever performing the
delayed call. Update the timelock flow around QueuedTransaction and
execute_transaction to store and consume the full call payload (including
target, action, and any needed arguments) and actually dispatch it when
executing a queued entry, rather than only removing it from the queue. Ensure
the execution path in the timelock contract uses the stored transaction fields
to invoke the intended action before recording it as executed.
| let mut queued_txs: Map<BytesN<32>, QueuedTransaction> = | ||
| env.storage().instance().get(&QUEUED_TXS).unwrap_or(Map::new(&env)); | ||
|
|
||
| if queued_txs.contains_key(tx_hash.clone()) { | ||
| panic!("Transaction already queued"); | ||
| } | ||
|
|
||
| queued_txs.set(tx_hash.clone(), queued); | ||
| env.storage().instance().set(&QUEUED_TXS, &queued_txs); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In soroban-sdk 21.0.0, does instance storage require explicit TTL extension for long-lived timelock data, and is persistent storage the recommended place for queued items that may execute far in the future?
💡 Result:
In soroban-sdk 21.0.0, all contract data, including instance storage, has a Time To Live (TTL) that must be periodically extended to prevent the data from being archived [1][2][3]. Instance storage shares a single TTL with the contract instance itself [1][3]. Consequently, when you extend the TTL of the instance storage, you are also extending the TTL of the contract instance and the contract code [1][3]. You must explicitly call the extend_ttl method (e.g., env.storage().instance().extend_ttl(...)) to maintain the data's availability [4][1][2]. Persistent storage is generally recommended for data that may need to exist for a long duration, particularly when that data scales in size [5][3]. Because instance storage is limited by the ledger entry size (typically ~100 KB) and is loaded into memory every time the contract is accessed, it is intended only for small, frequently used global state, such as admin accounts, contract metadata, or specific configuration settings [5][3]. For queued items that may execute far in the future, especially if they are numerous or variable in size, persistent storage is the appropriate choice, as each persistent entry has its own independent TTL that can be managed individually [1][3][6]. Reliance on TTL extensions should never be a substitute for proper design; if data must be permanent, it should be managed with that expectation, noting that archival and restoration mechanisms exist for persistent data [7].
Citations:
- 1: https://developers.stellar.org/docs/learn/fundamentals/contract-development/storage/state-archival
- 2: https://developers.stellar.org/docs/build/smart-contracts/getting-started/storing-data
- 3: https://developers.stellar.org/docs/build/guides/storage/choosing-the-right-storage
- 4: https://docs.rs/soroban-sdk/latest/soroban%5Fsdk/storage/struct.Instance.html
- 5: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Storage.html
- 6: Soroban HTLC: Fix Storage TTL — Prevent Fund Loss on Entry Expiry 0xdevcollins/useroutr#97
- 7: https://developers.stellar.org/docs/learn/fundamentals/contract-development/storage/persisting-data
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files packages/contracts/timelock/src
echo
echo "== outline: packages/contracts/timelock/src/lib.rs =="
ast-grep outline packages/contracts/timelock/src/lib.rs --view expanded || true
echo
echo "== search for TTL handling and storage kinds =="
rg -n "extend_ttl|persistent\(\)|instance\(\)|storage\(\)\.instance|storage\(\)\.persistent|QUEUED_TXS|QueuedTransaction" packages/contracts/timelock/src packages/contracts -S || true
echo
echo "== relevant excerpt from lib.rs =="
sed -n '1,220p' packages/contracts/timelock/src/lib.rsRepository: Galaxy-KJ/Galaxy-DevKit
Length of output: 21649
Extend TTL for queued timelock state or move it to persistent storage. QUEUED_TXS and EXECUTED live in instance storage, and none of queue_transaction, execute_transaction, revoke_transaction, get_queued_transaction, or is_executed calls extend_ttl. A transaction scheduled beyond the current instance TTL can disappear before it becomes executable or revocable.
🤖 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/timelock/src/lib.rs` around lines 43 - 51, The timelock
queued/executed state stored via QUEUED_TXS and EXECUTED in instance storage can
expire before delayed actions are processed, so update the timelock flow to
either move this state to persistent storage or extend its TTL whenever it is
accessed or mutated. Apply the fix consistently across queue_transaction,
execute_transaction, revoke_transaction, get_queued_transaction, and
is_executed, and use the existing storage access points in lib.rs to ensure
queued transactions remain available until they are executed or revoked.
| fn test_execute_transaction_after_eta() { | ||
| let (env, contract_id) = setup(); | ||
| let client = TimelockContractClient::new(&env, &contract_id); | ||
|
|
||
| env.ledger().with_mut(|l| l.timestamp = 1000); | ||
|
|
||
| let target = Address::generate(&env); | ||
| let action = String::from_str(&env, "transfer"); | ||
| let eta = 2000u64; | ||
|
|
||
| let tx_hash = client.queue_transaction(&target, &action, &eta); | ||
|
|
||
| env.ledger().with_mut(|l| l.timestamp = 2500); | ||
|
|
||
| client.execute_transaction(&tx_hash); | ||
|
|
||
| assert!(client.is_executed(&tx_hash)); | ||
| assert!(client.get_queued_transaction(&tx_hash).is_none()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The execute-path tests only verify bookkeeping, not the delayed action itself.
Both success cases assert is_executed and queue removal, so the current no-op implementation still passes. One of these tests should instead execute against a dummy target contract and assert the target-side state change.
Also applies to: 128-145
🤖 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/timelock/src/test.rs` around lines 48 - 65, The
execute-path tests for TimelockContractClient currently only check bookkeeping
via is_executed and get_queued_transaction, so a no-op execute_transaction can
still pass. Update one of the affected tests in
test_execute_transaction_after_eta (and the similar case near the other cited
test block) to invoke execute_transaction against a dummy target contract
through the target Address and action, then assert the target contract’s state
actually changed; keep the existing queue/removal checks as secondary
validation.
| pub struct VestingSchedule { | ||
| pub beneficiary: Address, | ||
| pub total_amount: i128, | ||
| pub start: u64, | ||
| pub cliff: u64, | ||
| pub duration: u64, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
claim_tokens never releases any tokens.
VestingSchedule stores only accounting fields, and claim_tokens just updates CLAIMD plus returns an i128. There is no token contract address, escrow source, or transfer call, so a successful claim leaves balances unchanged and does not satisfy the “token release” behavior described for this contract.
Also applies to: 62-97
🤖 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/vesting/src/lib.rs` around lines 12 - 18, `claim_tokens`
currently only updates `CLAIMD` and returns an amount, so no actual token
transfer happens. Update the vesting flow around `VestingSchedule` and
`claim_tokens` to include a real token source and transfer mechanism, such as
storing the token contract/address or escrow account in the schedule/state and
invoking the appropriate transfer call when a claim succeeds. Make sure the
claimed amount is deducted from the escrow/source balance and that
`claim_tokens` performs the release action rather than just accounting updates.
| pub fn create_schedule( | ||
| env: Env, | ||
| beneficiary: Address, | ||
| total_amount: i128, | ||
| start: u64, | ||
| cliff: u64, | ||
| duration: u64, | ||
| ) { | ||
| if total_amount <= 0 { | ||
| panic!("Total amount must be positive"); | ||
| } | ||
| if duration == 0 { | ||
| panic!("Duration must be positive"); | ||
| } | ||
| if cliff > duration { | ||
| panic!("Cliff cannot exceed duration"); | ||
| } | ||
|
|
||
| let mut schedules: Map<Address, VestingSchedule> = | ||
| env.storage().instance().get(&SCHEDULES).unwrap_or(Map::new(&env)); | ||
|
|
||
| if schedules.contains_key(beneficiary.clone()) { | ||
| panic!("Schedule already exists for beneficiary"); | ||
| } | ||
|
|
||
| let schedule = VestingSchedule { | ||
| beneficiary: beneficiary.clone(), | ||
| total_amount, | ||
| start, | ||
| cliff, | ||
| duration, | ||
| }; | ||
|
|
||
| schedules.set(beneficiary, schedule); | ||
| env.storage().instance().set(&SCHEDULES, &schedules); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Add an authorization model before this becomes externally callable.
These entrypoints mutate persistent state for any supplied beneficiary without authenticating the caller. Right now any account can pre-create a bogus schedule for someone else and later advance that beneficiary’s CLAIMD state, which blocks legitimate use. Gate create_schedule behind an issuer/admin auth path, and require beneficiary auth for claims (or transfer directly to the stored beneficiary during the claim flow).
Also applies to: 62-97
🤖 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/vesting/src/lib.rs` around lines 25 - 60, The vesting
entrypoints are missing an authorization model, so any caller can create or
advance state for an arbitrary beneficiary. Update create_schedule in
vesting::create_schedule to require issuer/admin authorization before mutating
SCHEDULES, and ensure the claim flow in the related claim entrypoints (the
CLAIMD-related paths in the same module) either requires beneficiary auth or
sends funds directly to the stored VestingSchedule.beneficiary. Use the existing
create_schedule and claim-related symbols to centralize the auth checks so
unauthorized accounts cannot pre-create or block schedules.
| describe('evaluateWalletRisk', () => { | ||
| it('should return low risk for healthy position', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 3.5, | ||
| poolConcentration: 0.1, | ||
| assetVolatility: 0.05, | ||
| leverageRatio: 1.0, | ||
| totalCollateralUSD: 10000, | ||
| totalDebtUSD: 1000, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBeLessThan(25); | ||
| expect(result.riskLevel).toBe('low'); | ||
| expect(result.factors).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('should return critical risk for highly leveraged position', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 1.01, | ||
| poolConcentration: 0.9, | ||
| assetVolatility: 0.8, | ||
| leverageRatio: 4.5, | ||
| totalCollateralUSD: 5000, | ||
| totalDebtUSD: 4500, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBeGreaterThanOrEqual(75); | ||
| expect(result.riskLevel).toBe('critical'); | ||
| expect(result.factors.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('should return medium risk for moderate position', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 2.0, | ||
| poolConcentration: 0.3, | ||
| assetVolatility: 0.2, | ||
| leverageRatio: 2.0, | ||
| totalCollateralUSD: 10000, | ||
| totalDebtUSD: 5000, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBeGreaterThanOrEqual(25); | ||
| expect(result.score).toBeLessThan(50); | ||
| expect(result.riskLevel).toBe('medium'); | ||
| }); | ||
|
|
||
| it('should return high risk for imbalanced position', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 1.2, | ||
| poolConcentration: 0.7, | ||
| assetVolatility: 0.6, | ||
| leverageRatio: 3.0, | ||
| totalCollateralUSD: 10000, | ||
| totalDebtUSD: 8000, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBeGreaterThanOrEqual(50); | ||
| expect(result.score).toBeLessThan(75); | ||
| expect(result.riskLevel).toBe('high'); | ||
| }); | ||
|
|
||
| it('should cap score at 0 for minimum risk', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 10.0, | ||
| poolConcentration: 0.0, | ||
| assetVolatility: 0.0, | ||
| leverageRatio: 1.0, | ||
| totalCollateralUSD: 100000, | ||
| totalDebtUSD: 0, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBe(0); | ||
| expect(result.riskLevel).toBe('low'); | ||
| }); | ||
|
|
||
| it('should cap score at 100 for maximum risk', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 1.0, | ||
| poolConcentration: 1.0, | ||
| assetVolatility: 1.0, | ||
| leverageRatio: 5.0, | ||
| totalCollateralUSD: 100, | ||
| totalDebtUSD: 1000, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.score).toBe(100); | ||
| expect(result.riskLevel).toBe('critical'); | ||
| }); | ||
|
|
||
| it('should include factor descriptions when thresholds exceeded', () => { | ||
| const input: WalletRiskInput = { | ||
| healthFactor: 1.1, | ||
| poolConcentration: 0.8, | ||
| assetVolatility: 0.6, | ||
| leverageRatio: 4.0, | ||
| totalCollateralUSD: 10000, | ||
| totalDebtUSD: 9000, | ||
| }; | ||
|
|
||
| const result = service.evaluateWalletRisk(input); | ||
|
|
||
| expect(result.factors).toContainEqual( | ||
| expect.stringContaining('Low health factor') | ||
| ); | ||
| expect(result.factors).toContainEqual( | ||
| expect.stringContaining('High pool concentration') | ||
| ); | ||
| expect(result.factors).toContainEqual( | ||
| expect.stringContaining('High asset volatility') | ||
| ); | ||
| expect(result.factors).toContainEqual( | ||
| expect.stringContaining('High leverage ratio') | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The suite does not pin the promised weights or boundary cutoffs.
These cases only assert coarse score bands, so changing the weights or the inclusivity at 25/50/75 can still pass. The acceptance criteria call out correct weighting and threshold behavior, so please add exact-score assertions and dedicated boundary cases for 24→25, 49→50, and 74→75.
🤖 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/wallet/src/tests/risk-score.service.test.ts` around lines 10 -
136, The evaluateWalletRisk test suite in risk-score.service.test.ts only checks
broad score ranges, so changes to weighting or the 25/50/75 cutoff behavior
could slip through. Update the tests around service.evaluateWalletRisk to assert
exact scores for the known scenarios and add dedicated boundary cases that
verify the transition behavior at 24→25, 49→50, and 74→75, including the
riskLevel and factors produced at each threshold.
| testEnvironment: 'node', | ||
| displayName: 'integration', | ||
| rootDir: '.', | ||
| testMatch: ['<rootDir>/src/**/*.test.ts'], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add coverage thresholds to enforce the 90%+ objective.
The PR objectives specify 90%+ coverage, but no coverage thresholds are configured. Add coverageThreshold to fail builds below the target.
moduleNameMapper: {
'^(\\.\\.?/.*)\\.js$': '$1',
},
+ coverageThreshold: {
+ global: {
+ branches: 90,
+ functions: 90,
+ lines: 90,
+ statements: 90,
+ },
+ },📝 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.
| testMatch: ['<rootDir>/src/**/*.test.ts'], | |
| testMatch: ['<rootDir>/src/**/*.test.ts'], | |
| moduleNameMapper: { | |
| '^(\\.\\.?/.*)\\.js$': '$1', | |
| }, | |
| coverageThreshold: { | |
| global: { | |
| branches: 90, | |
| functions: 90, | |
| lines: 90, | |
| statements: 90, | |
| }, | |
| }, |
🤖 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/integration-tests/jest.config.cjs` at line 10, The Jest config
currently defines only test discovery via testMatch and does not enforce the
90%+ coverage objective. Update the jest.config.cjs settings by adding a
coverageThreshold so the integration test suite fails when coverage drops below
the target, using the existing Jest config object in this file as the place to
locate and extend the coverage settings.
| import { Keypair, Networks, rpc, BASE_FEE } from '@stellar/stellar-sdk'; | ||
| import { | ||
| setupTestnetEnv, | ||
| createTestEnv, | ||
| IntegrationTestEnv, | ||
| } from '../setup/testnet.js'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use setupTestnetEnv instead of createTestEnv for integration tests.
createTestEnv creates mock/unfunded environment values. Integration tests should use setupTestnetEnv to actually fund accounts and verify RPC connectivity. The current tests don't exercise real testnet integration.
beforeAll(async () => {
- env = createTestEnv();
+ env = await setupTestnetEnv();
});🤖 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/integration-tests/src/defi.test.ts` around lines 1 - 6, Integration
tests are importing and using the mock environment helper instead of the real
testnet setup; update the test setup in defi.test.ts to rely on setupTestnetEnv
and remove createTestEnv usage. Make sure the test harness still initializes
through the setup/testnet.js helpers and any references to IntegrationTestEnv
are wired to the funded, RPC-verified environment so the tests exercise real
testnet connectivity.
| describe('Blend Protocol Integration', () => { | ||
| it('should return mock pool health factor above liquidation threshold', () => { | ||
| const healthFactor = '1.5'; | ||
| const liquidationThreshold = '1.0'; | ||
| const healthFactorNum = parseFloat(healthFactor); | ||
| const liquidationThresholdNum = parseFloat(liquidationThreshold); | ||
| const isHealthy = healthFactorNum > liquidationThresholdNum; | ||
| expect(isHealthy).toBe(true); | ||
| }); | ||
|
|
||
| it('should detect undercollateralized position', () => { | ||
| const healthFactor = '0.85'; | ||
| const liquidationThreshold = '1.0'; | ||
| const healthFactorNum = parseFloat(healthFactor); | ||
| const liquidationThresholdNum = parseFloat(liquidationThreshold); | ||
| const isLiquidatable = healthFactorNum < liquidationThresholdNum; | ||
| expect(isLiquidatable).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and inspect the relevant section with line numbers.
sed -n '1,220p' packages/integration-tests/src/defi.test.ts
# Check whether the integration-tests package references the Blend SDK anywhere.
rg -n "`@blend-capital/blend-sdk`|Blend Protocol Integration|liquidationThreshold|healthFactor" packages/integration-tests -SRepository: Galaxy-KJ/Galaxy-DevKit
Length of output: 4212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect package metadata for dependencies and test intent.
sed -n '1,220p' packages/integration-tests/package.json
# Find any Blend-related code in the repository for context.
rg -n "blend-sdk|Blend Protocol|blend-capital|liquidation threshold|health factor" -S .Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If available, show surrounding package/workspace files to understand scope.
git ls-files 'packages/integration-tests/**' 'package.json' 'pnpm-workspace.yaml' 'turbo.json' 'nx.json' | sed -n '1,200p'Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 432
Rename this block or wire in real Blend calls
This only compares hardcoded floats; it doesn’t exercise @blend-capital/blend-sdk or any RPC path, so the “Blend Protocol Integration” name is misleading. If this is just placeholder math coverage, rename the describe block; otherwise replace it with an actual Blend SDK/integration flow.
🤖 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/integration-tests/src/defi.test.ts` around lines 45 - 63, The Blend
Protocol Integration block in the test suite is only validating hardcoded float
comparisons, so either rename the describe block and its test names to reflect
placeholder math coverage, or replace the assertions with a real Blend SDK/RPC
integration flow that exercises `@blend-capital/blend-sdk` instead of local
parsing logic. Update the relevant describe block and the two it cases so the
naming matches the actual behavior being tested.
| it('should detect price deviation anomaly', () => { | ||
| const currentPrice = 105.0; | ||
| const referencePrice = 100.0; | ||
| const deviationThreshold = 0.05; | ||
|
|
||
| const deviation = | ||
| Math.abs(currentPrice - referencePrice) / referencePrice; | ||
| const isAnomaly = deviation > deviationThreshold; | ||
|
|
||
| expect(deviation).toBeCloseTo(0.05, 2); | ||
| expect(isAnomaly).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify test name or adjust threshold for anomaly detection.
The test name "should detect price deviation anomaly" implies the deviation should be flagged, but the assertion expects isAnomaly to be false because deviation (0.05) is not strictly greater than threshold (0.05). Rename to "should not flag deviation at exactly threshold" or change threshold to 0.049 to match the test name intent.
- it('should detect price deviation anomaly', () => {
+ it('should not flag deviation at exactly the threshold', () => {
const currentPrice = 105.0;
const referencePrice = 100.0;
const deviationThreshold = 0.05;📝 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.
| it('should detect price deviation anomaly', () => { | |
| const currentPrice = 105.0; | |
| const referencePrice = 100.0; | |
| const deviationThreshold = 0.05; | |
| const deviation = | |
| Math.abs(currentPrice - referencePrice) / referencePrice; | |
| const isAnomaly = deviation > deviationThreshold; | |
| expect(deviation).toBeCloseTo(0.05, 2); | |
| expect(isAnomaly).toBe(false); | |
| }); | |
| it('should not flag deviation at exactly the threshold', () => { | |
| const currentPrice = 105.0; | |
| const referencePrice = 100.0; | |
| const deviationThreshold = 0.05; | |
| const deviation = | |
| Math.abs(currentPrice - referencePrice) / referencePrice; | |
| const isAnomaly = deviation > deviationThreshold; | |
| expect(deviation).toBeCloseTo(0.05, 2); | |
| expect(isAnomaly).toBe(false); | |
| }); |
🤖 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/integration-tests/src/oracle.test.ts` around lines 25 - 36, The test
case in oracle.test.ts has a mismatch between its name and the assertion logic:
the current `should detect price deviation anomaly` title implies a positive
anomaly result, but `isAnomaly` is expected to be false when
`deviationThreshold` equals the computed deviation. Update the test in the
relevant `it(...)` block to either rename it to reflect the exact-threshold
non-anomaly behavior or adjust `deviationThreshold` so the `isAnomaly`
expectation matches the intended anomaly detection scenario.
2 participants
Feat: Integration Infrastructure, Risk Scoring, and Soroban Timelock & Vesting Contracts (Closes #266, Closes #305, Closes #310, Closes #311)
Overview
This pull request delivers foundational testing infrastructure alongside critical protocol and ecosystem features.
First, it establishes a cross-package integration testing package tailored for Stellar Testnet interactions. Second, it adds an automated wallet portfolio risk-scoring system to preempt liquidation events. Finally, it implements two foundational Soroban smart contracts: a security-focused Timelock contract and a linear Token Vesting contract.
Feature Summary
Technical Implementation
packages/integration-tests/setup/testnet.tsexposingsetupTestnetEnv(). The bootstrapper utilizes@stellar/stellar-sdkto provision unique ephemeral Keypairs and requests test assets from Friendbot. Integration tests run within a dedicated Jest project configuration viajest.config.jstargeting live protocol behaviors across Blend and Soroswap.RiskScoringServiceinsidepackages/core/wallet. The engine evaluates connected wallets by pulling balance lists and Blend health metrics, translating asset volatility and high-concentration positions into a normalized score (queue_transaction. Execution viaexecute_transactionstrictly validates that the current ledger time exceeds the registeredbeneficiarymapping toVestingSchedule). Theclaim_tokensmethod uses the current timestamp to verify if the cliff has been passed, calculating claimable tokens linearly using the formula:Test Coverage
RiskScoringService.Checklists
Summary by CodeRabbit
New Features
Tests