#125 Protocol Fee Collection & Treasury Management#137
Open
Georgebingi wants to merge 5 commits intoanonfedora:masterfrom
Open
#125 Protocol Fee Collection & Treasury Management#137Georgebingi wants to merge 5 commits intoanonfedora:masterfrom
Georgebingi wants to merge 5 commits intoanonfedora:masterfrom
Conversation
📝 WalkthroughWalkthroughProtocol treasury contract initialization now accepts and stores a governance address parameter, with a new governance-authorized withdrawal function for accessing accumulated fees. A background liquidation service monitors active loans, calculates loan-to-value ratios against real-time collateral balances, and triggers escrow liquidations when LTV exceeds a 0.85 threshold. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant LiquidationService
participant Database
participant BlockchainService
participant ContractService
participant WebSocketService
Scheduler->>LiquidationService: performLiquidationChecks()
LiquidationService->>Database: fetch all loans<br/>(status='ACTIVE')
Database-->>LiquidationService: loans with borrower,<br/>lender, collateral
loop For each loan
LiquidationService->>BlockchainService: getAccountBalance()<br/>(collateral asset code)
BlockchainService-->>LiquidationService: collateral value
LiquidationService->>LiquidationService: calculate LTV ratio<br/>(loanAmount / collateralValue)
alt LTV > 0.85 threshold
LiquidationService->>LiquidationService: triggerLiquidation()
LiquidationService->>ContractService: submitXDR()<br/>(refund_escrow call)
ContractService-->>LiquidationService: success
LiquidationService->>Database: update loan status<br/>to 'DEFAULTED'
LiquidationService->>WebSocketService: broadcast<br/>'LIQUIDATED' event
alt Failure during submission
ContractService-->>LiquidationService: error
loop Retry up to 3 times
LiquidationService->>ContractService: retry submitXDR()
end
LiquidationService->>WebSocketService: broadcast<br/>'LIQUIDATION_FAILED'
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/protocol-treasury/src/lib.rs (1)
65-88:⚠️ Potential issue | 🔴 CriticalTest code uses outdated
initializesignature, causing compilation failure.The
initializefunction signature was updated to require agovernanceparameter, but the test code at lines 370 and 419 still callsinitialize(&admin)with only one argument. This will fail to compile.🐛 Proposed fix for the test code
Update the
setup()function around line 370:- client.initialize(&admin); + let governance = Address::generate(&env); + client.initialize(&admin, &governance);Update the
TestEnvstruct to store the governance address for test assertions, and update thetest_initialize_already_initializedtest at line 419:- t.client.initialize(&t.admin); + let governance = Address::generate(&t.env); + t.client.initialize(&t.admin, &governance);Additionally, update
test_initializeto verify the governance address is stored correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/protocol-treasury/src/lib.rs` around lines 65 - 88, The tests call the old initialize signature; update test setup and assertions to pass and validate the new governance parameter: change the setup() helper to call initialize(env, admin, governance) (passing a governance Address), add a governance field to the TestEnv struct to store that Address for later checks, update test_initialize_already_initialized to call initialize with both admin and governance and assert AlreadyInitialized still triggers, and update test_initialize to verify the stored governance value (alongside admin and fee_bps) in contract storage/events.
🧹 Nitpick comments (2)
contracts/escrow-manager/src/lib.rs (2)
394-394: Consider using checked arithmetic for overflow protection.The multiplication
escrow.amount * fee_bps as i128could theoretically overflow for extremely large escrow amounts, though this is unlikely in practice given i128's range. Using checked arithmetic would be more defensive.♻️ Optional: Use checked multiplication
- let fee_amount = (escrow.amount * fee_bps as i128) / 10000; + let fee_amount = escrow.amount + .checked_mul(fee_bps as i128) + .and_then(|v| v.checked_div(10000)) + .unwrap_or(0);Note: The
estimate_path_paymentfunction at lines 517-520 already uses this pattern withchecked_mulandchecked_div.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` at line 394, The calculation of fee_amount uses unchecked multiplication which can overflow: replace the direct expression in the escrow fee computation (fee_amount = (escrow.amount * fee_bps as i128) / 10000) with checked arithmetic using escrow.amount.checked_mul(fee_bps as i128) and then .checked_div(10000), mirroring the pattern used in estimate_path_payment; handle the Option by returning an appropriate error (or early return) when checked_* yields None so overflows are safely rejected.
622-632: Test coverage gap: no tests verify fee collection with non-zero fees.The
MockTreasuryalways returns0fee (line 628), so existing tests don't actually validate the fee deduction logic. The test at line 934 expects the seller to receive5000(full escrow amount), which passes only because no fee is deducted.Consider adding tests that:
- Use a mock treasury returning non-zero
fee_bps- Verify seller receives
net_amount(e.g., 4975 for 50 bps fee on 5000)- Verify treasury receives the fee amount
- Verify
deposit_feeis called with correct parameters🧪 Suggested test helper and test case
// Add a configurable mock treasury #[contract] pub struct MockTreasuryWithFee; #[contractimpl] impl MockTreasuryWithFee { pub fn get_fee_bps(env: Env) -> u32 { env.storage().instance().get(&symbol_short!("fee_bps")).unwrap_or(50u32) } pub fn set_fee_bps(env: Env, fee_bps: u32) { env.storage().instance().set(&symbol_short!("fee_bps"), &fee_bps); } pub fn deposit_fee(env: Env, asset: Address, amount: i128) { // Track deposits for verification let key = (symbol_short!("deposits"), asset.clone()); let current: i128 = env.storage().persistent().get(&key).unwrap_or(0); env.storage().persistent().set(&key, &(current + amount)); } pub fn get_deposits(env: Env, asset: Address) -> i128 { let key = (symbol_short!("deposits"), asset); env.storage().persistent().get(&key).unwrap_or(0) } } #[test] fn test_release_funds_with_protocol_fee() { // Setup with MockTreasuryWithFee returning 50 bps (0.5%) // Create escrow with 5000 amount // Release funds // Assert: seller receives 4975, treasury receives 25 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 622 - 632, Add a new configurable mock treasury and tests that exercise non-zero fees: replace or augment MockTreasury with a MockTreasuryWithFee that implements get_fee_bps(env) (reads a stored fee_bps, default 50), set_fee_bps(env, fee_bps) to configure it, deposit_fee(env, asset, amount) to record deposited fees (e.g., in persistent storage keyed by asset), and get_deposits(env, asset) to read recorded fees; then write a test (e.g., test_release_funds_with_protocol_fee) that sets fee_bps to 50, creates an escrow for 5000, calls the release path in the escrow manager, and asserts the seller balance equals 5000 - fee (4975) and that MockTreasuryWithFee.get_deposits(asset) equals the fee (25), and ensure deposit_fee was invoked with the correct asset and amount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/protocol-treasury/src/lib.rs`:
- Around line 303-337: withdraw_treasury currently reduces the same persistent
total_fees value that claim_share uses to compute entitlements, causing
contributor entitlements to shrink below already_claimed; fix by separating
accumulated fees from withdrawable/reserved amounts: introduce and persist a
distinct accumulator (e.g. symbol_short!("fees_accumulated") or a per-asset pair
like (symbol_short!("fees_accumulated"), asset)) that claim_share uses to
compute entitled = (fees_accumulated * share_weight) / total_weight, and track
either total_withdrawn or total_claimed (e.g. (symbol_short!("fees_withdrawn"),
asset) or (symbol_short!("fees_claimed"), asset)) so withdraw_treasury checks
that fees_accumulated - total_withdrawn - amount >= total_claimed_reserved (or
simply disallows withdrawing more than fees_accumulated - total_withdrawn) and
then increments total_withdrawn when transferring; update withdraw_treasury,
claim_share, and the fee_key usage accordingly so entitlement math reads from
fees_accumulated while withdrawals only reduce withdrawable balance.
---
Outside diff comments:
In `@contracts/protocol-treasury/src/lib.rs`:
- Around line 65-88: The tests call the old initialize signature; update test
setup and assertions to pass and validate the new governance parameter: change
the setup() helper to call initialize(env, admin, governance) (passing a
governance Address), add a governance field to the TestEnv struct to store that
Address for later checks, update test_initialize_already_initialized to call
initialize with both admin and governance and assert AlreadyInitialized still
triggers, and update test_initialize to verify the stored governance value
(alongside admin and fee_bps) in contract storage/events.
---
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Line 394: The calculation of fee_amount uses unchecked multiplication which
can overflow: replace the direct expression in the escrow fee computation
(fee_amount = (escrow.amount * fee_bps as i128) / 10000) with checked arithmetic
using escrow.amount.checked_mul(fee_bps as i128) and then .checked_div(10000),
mirroring the pattern used in estimate_path_payment; handle the Option by
returning an appropriate error (or early return) when checked_* yields None so
overflows are safely rejected.
- Around line 622-632: Add a new configurable mock treasury and tests that
exercise non-zero fees: replace or augment MockTreasury with a
MockTreasuryWithFee that implements get_fee_bps(env) (reads a stored fee_bps,
default 50), set_fee_bps(env, fee_bps) to configure it, deposit_fee(env, asset,
amount) to record deposited fees (e.g., in persistent storage keyed by asset),
and get_deposits(env, asset) to read recorded fees; then write a test (e.g.,
test_release_funds_with_protocol_fee) that sets fee_bps to 50, creates an escrow
for 5000, calls the release path in the escrow manager, and asserts the seller
balance equals 5000 - fee (4975) and that
MockTreasuryWithFee.get_deposits(asset) equals the fee (25), and ensure
deposit_fee was invoked with the correct asset and amount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b598fa3b-5245-4797-8253-77b09c18134c
📒 Files selected for processing (2)
contracts/escrow-manager/src/lib.rscontracts/protocol-treasury/src/lib.rs
Comment on lines
+303
to
+337
|
|
||
| /// Governance-only withdrawal of treasury funds to DAO treasury. | ||
| /// Protected by governance contract authorization. | ||
| pub fn withdraw_treasury(env: Env, asset: Address, amount: i128) -> Result<(), ContractError> { | ||
| let governance: Address = env | ||
| .storage() | ||
| .instance() | ||
| .get(&symbol_short!("governance")) | ||
| .ok_or(ContractError::Unauthorized)?; | ||
|
|
||
| governance.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| return Err(ContractError::ZeroAmount); | ||
| } | ||
|
|
||
| let fee_key = (symbol_short!("fees"), asset.clone()); | ||
| let total_fees: i128 = env.storage().persistent().get(&fee_key).unwrap_or(0); | ||
|
|
||
| if total_fees < amount { | ||
| return Err(ContractError::NoFeesAvailable); | ||
| } | ||
|
|
||
| // Transfer tokens to governance | ||
| let token_client = token::Client::new(&env, &asset); | ||
| token_client.transfer(&env.current_contract_address(), &governance, &amount); | ||
|
|
||
| // Update recorded fees | ||
| env.storage().persistent().set(&fee_key, &(total_fees - amount)); | ||
|
|
||
| env.events() | ||
| .publish((symbol_short!("trs_wd"),), (asset.clone(), amount)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical accounting conflict between withdraw_treasury and claim_share.
The withdraw_treasury function decrements total_fees (line 331), but claim_share relies on this same value to calculate contributor entitlements (line 256). This creates an accounting conflict:
- Contributor A is entitled to
(total_fees * share_weight) / total_weight - Governance withdraws some fees, reducing
total_fees - Contributor A's
entitledamount decreases, potentially below theiralready_claimedamount - Contributor A can no longer claim their expected fees (gets
NoFeesAvailable)
Example scenario:
- Total fees = 1000, Contributor has 100% share, claims 500
- Governance withdraws 600, total_fees becomes 400
- Contributor's new entitled = 400, but already_claimed = 500
claimable = 400 - 500 = -100→ fails withNoFeesAvailable
Consider separating the concepts of "total accumulated fees" (for entitlement calculation) from "withdrawable balance" (total fees minus total already claimed), or adding a check that withdrawal cannot reduce total_fees below the sum of all contributor claims.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/protocol-treasury/src/lib.rs` around lines 303 - 337,
withdraw_treasury currently reduces the same persistent total_fees value that
claim_share uses to compute entitlements, causing contributor entitlements to
shrink below already_claimed; fix by separating accumulated fees from
withdrawable/reserved amounts: introduce and persist a distinct accumulator
(e.g. symbol_short!("fees_accumulated") or a per-asset pair like
(symbol_short!("fees_accumulated"), asset)) that claim_share uses to compute
entitled = (fees_accumulated * share_weight) / total_weight, and track either
total_withdrawn or total_claimed (e.g. (symbol_short!("fees_withdrawn"), asset)
or (symbol_short!("fees_claimed"), asset)) so withdraw_treasury checks that
fees_accumulated - total_withdrawn - amount >= total_claimed_reserved (or simply
disallows withdrawing more than fees_accumulated - total_withdrawn) and then
increments total_withdrawn when transferring; update withdraw_treasury,
claim_share, and the fee_key usage accordingly so entitlement math reads from
fees_accumulated while withdrawals only reduce withdrawable balance.
Owner
|
Fix failing ci @Georgebingi and reviewed suggestions |
|
@Georgebingi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 388-395: The release path currently re-reads
ProtocolTreasury::get_fee_bps (env.invoke_contract with Symbol "get_fee_bps")
which allows fee changes after locking; modify the Escrow data structure to
include a stored fee snapshot (e.g., fee_bps or fee_amount) set during
create_escrow, persist that value into Escrow, and replace the runtime call in
the release logic (where protocol_fee is computed using treasury_opt and
get_fee_bps) to use the stored Escrow fee snapshot (e.g., escrow.fee_bps or
escrow.fee_amount) when computing protocol_fee and validating
min_destination_amount so the fee is frozen at creation.
- Around line 402-408: The treasury ledger is forgeable because
ProtocolTreasury::deposit_fee accepts unauthenticated calls and merely
increments storage; change the logic so deposit_fee enforces caller
authentication and actually receives tokens (or is only callable by the escrow
manager after a successful token transfer). Update ProtocolTreasury::deposit_fee
to check the invoker (e.g., require that env.invoker() or the calling contract
equals the escrow manager/authorized source) and to pull or validate funds (use
token.transfer_from or require a token escrow/proof) before updating storage and
emitting the event; also ensure the escrow-manager still performs the token
transfer step (use the same token symbol and fee_amount) before invoking
Symbol::new(..., "deposit_fee") so recorded fees reflect real asset movement.
In `@server/src/services/liquidation.service.ts`:
- Around line 36-38: The scheduler block is broken: the cron.schedule(...) call
is commented out while the callback body (await
this.performLiquidationChecks();) and closing `});` remain, producing invalid
syntax; fix by restoring a proper cron job assignment—uncomment and use
this.cronJob = cron.schedule(`*/${CHECK_INTERVAL_MINUTES} * * * *`, async () =>
{ await this.performLiquidationChecks(); });—or if you don't want a cron, remove
the dangling `await this.performLiquidationChecks();` and the trailing `});`.
Ensure references to cron.schedule, CHECK_INTERVAL_MINUTES, this.cronJob, and
this.performLiquidationChecks() are updated accordingly.
- Around line 135-145: The call to EscrowManager::refund_escrow is incorrect:
refund_escrow accepts a single u64 escrow_id and performs expiry checks, but the
code (buildContractInvokeXDR invoking 'refund_escrow' with loanIdBytes,
borrower/lender) supplies three args and encodes addresses as raw UTF‑8; also
Loan has no persisted escrow id. Fix by either (A) persisting the on‑chain
escrow id on the Loan model and changing the invoke to call the proper
liquidation entrypoint (not 'refund_escrow') with a single u64 escrow id (update
the code using buildContractInvokeXDR and loanIdBytes -> escrowIdU64), or (B) if
using borrower/lender in the contract, change the invoke to the contract's
actual liquidation method and encode borrowerId/lenderId as proper Soroban
ScAddress/ed25519 public keys (not TextEncoder bytes) when building args; update
the Loan model only if you choose option A so you can pass the correct escrow
identifier to buildContractInvokeXDR.
🪄 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: 2f72af6a-75c1-4b0d-a6c5-7e3c1b967441
📒 Files selected for processing (2)
contracts/escrow-manager/src/lib.rsserver/src/services/liquidation.service.ts
contracts/escrow-manager/src/lib.rs
Outdated
Comment on lines
+388
to
+395
| let protocol_fee = if let Some(treasury) = treasury_opt { | ||
| // Query fee_bps from ProtocolTreasury | ||
| let fee_bps_args: soroban_sdk::Vec<Val> = soroban_sdk::Vec::new(&env); | ||
| let fee_bps: u32 = | ||
| env.invoke_contract(&treasury, &Symbol::new(&env, "get_fee_bps"), fee_bps_args); | ||
|
|
||
| // Calculate fee on the escrow amount | ||
| let fee_amount = (escrow.amount * fee_bps as i128) / 10000; |
There was a problem hiding this comment.
Freeze the fee rate at escrow creation.
This path re-reads get_fee_bps during release, so a fee update after funds are locked can change the seller payout or make a previously valid min_destination_amount revert. Store the agreed fee bps (or fee amount) in Escrow during create_escrow and use that snapshot here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/escrow-manager/src/lib.rs` around lines 388 - 395, The release path
currently re-reads ProtocolTreasury::get_fee_bps (env.invoke_contract with
Symbol "get_fee_bps") which allows fee changes after locking; modify the Escrow
data structure to include a stored fee snapshot (e.g., fee_bps or fee_amount)
set during create_escrow, persist that value into Escrow, and replace the
runtime call in the release logic (where protocol_fee is computed using
treasury_opt and get_fee_bps) to use the stored Escrow fee snapshot (e.g.,
escrow.fee_bps or escrow.fee_amount) when computing protocol_fee and validating
min_destination_amount so the fee is frozen at creation.
contracts/escrow-manager/src/lib.rs
Outdated
Comment on lines
+402
to
+408
| // Record the fee deposit in treasury | ||
| let deposit_args: soroban_sdk::Vec<Val> = soroban_sdk::Vec::from_array( | ||
| &env, | ||
| [escrow.asset.into_val(&env), fee_amount.into_val(&env)], | ||
| ); | ||
| let _: () = | ||
| env.invoke_contract(&treasury, &Symbol::new(&env, "deposit_fee"), deposit_args); |
There was a problem hiding this comment.
The treasury fee ledger is still forgeable.
This integration depends on ProtocolTreasury::deposit_fee, but that entrypoint currently only increments storage and emits an event. Because it does not authenticate the caller or pull tokens, anyone can inflate the recorded fee totals without transferring assets, so this is not a trustworthy on-chain source of truth yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/escrow-manager/src/lib.rs` around lines 402 - 408, The treasury
ledger is forgeable because ProtocolTreasury::deposit_fee accepts
unauthenticated calls and merely increments storage; change the logic so
deposit_fee enforces caller authentication and actually receives tokens (or is
only callable by the escrow manager after a successful token transfer). Update
ProtocolTreasury::deposit_fee to check the invoker (e.g., require that
env.invoker() or the calling contract equals the escrow manager/authorized
source) and to pull or validate funds (use token.transfer_from or require a
token escrow/proof) before updating storage and emitting the event; also ensure
the escrow-manager still performs the token transfer step (use the same token
symbol and fee_amount) before invoking Symbol::new(..., "deposit_fee") so
recorded fees reflect real asset movement.
Comment on lines
+36
to
+38
| // this.cronJob = cron.schedule(`*/${CHECK_INTERVAL_MINUTES} * * * *`, async () => { | ||
| await this.performLiquidationChecks(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '30,42p' server/src/services/liquidation.service.tsRepository: anonfedora/stellovault
Length of output: 457
Fix the broken scheduler block.
The cron.schedule( call is commented out while the callback body and closing }); remain active, creating invalid syntax. Either uncomment the full scheduler line or remove the dangling await and closing brace.
🧰 Tools
🪛 Biome (2.4.9)
[error] 38-38: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found ')'.
(parse)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/liquidation.service.ts` around lines 36 - 38, The
scheduler block is broken: the cron.schedule(...) call is commented out while
the callback body (await this.performLiquidationChecks();) and closing `});`
remain, producing invalid syntax; fix by restoring a proper cron job
assignment—uncomment and use this.cronJob =
cron.schedule(`*/${CHECK_INTERVAL_MINUTES} * * * *`, async () => { await
this.performLiquidationChecks(); });—or if you don't want a cron, remove the
dangling `await this.performLiquidationChecks();` and the trailing `});`. Ensure
references to cron.schedule, CHECK_INTERVAL_MINUTES, this.cronJob, and
this.performLiquidationChecks() are updated accordingly.
Comment on lines
+135
to
+145
| // Build liquidation invoke (assuming EscrowManager has liquidate_loan method) | ||
| // Params: loan_id (as bytes), borrower, lender | ||
| const loanIdBytes = xdr.ScVal.scvBytes(new TextEncoder().encode(loanId)); | ||
| const xdrResult = await contractService.buildContractInvokeXDR( | ||
| escrowManagerId, | ||
| 'refund_escrow', // Triggers liquidation by refunding expired/high-risk escrow | ||
| [ | ||
| loanIdBytes, | ||
| xdr.ScVal.scvAddress(xdr.PublicKey.publicKeyTypeEd25519(new TextEncoder().encode(borrowerId))), | ||
| xdr.ScVal.scvAddress(xdr.PublicKey.publicKeyTypeEd25519(new TextEncoder().encode(lenderId))), | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf 'Liquidation invocation:\n'
sed -n '135,147p' server/src/services/liquidation.service.ts
printf '\nEscrowManager refund_escrow signature and precondition:\n'
sed -n '530,545p' contracts/escrow-manager/src/lib.rs
printf '\nLoan/escrow identifiers referenced in the repo:\n'
rg -n -C2 '\bescrowId\b|\bloanId\b|\brefund_escrow\b' || trueRepository: anonfedora/stellovault
Length of output: 1786
🏁 Script executed:
#!/bin/bash
printf 'Liquidation invocation:\n'
sed -n '135,147p' server/src/services/liquidation.service.ts
printf '\nEscrowManager refund_escrow signature and precondition:\n'
sed -n '530,545p' contracts/escrow-manager/src/lib.rs
printf '\nLoan/escrow identifiers referenced in the repo:\n'
rg -n -C2 '\bescrowId\b|\bloanId\b|\brefund_escrow\b' || trueRepository: anonfedora/stellovault
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Check Loan model schema and definition
echo "=== Loan Model Schema ==="
rg -A 20 'model Loan \{' server/prisma/schema.prisma
echo -e "\n=== Escrow Model Schema ==="
rg -A 20 'model Escrow \{' server/prisma/schema.prisma
echo -e "\n=== LoanService Liquidation Method ==="
rg -B 5 -A 15 'triggerLiquidation' server/src/services/liquidation.service.ts | head -40Repository: anonfedora/stellovault
Length of output: 3336
refund_escrow is the wrong contract call for liquidation and will fail at submission.
The EscrowManager::refund_escrow contract method takes a single escrow_id: u64, but the invocation submits three arguments (loanIdBytes, borrowerAddress, lenderAddress). Additionally, the Loan model does not store an escrow identifier—there is no escrowId field to map loans to on-chain escrow accounts. The borrowerId and lenderId are also encoded as raw UTF-8 bytes via TextEncoder, which is not a valid Soroban ScAddress representation. Finally, refund_escrow enforces expiry checks and will reject non-expired escrows, making it incompatible with high-LTV liquidation semantics.
The liquidation flow must either:
- Persist the on-chain escrow ID in the
Loanmodel and call a contract entrypoint designed for liquidation, or - Refactor to correctly encode borrower/lender as Soroban account addresses and match the actual contract signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/liquidation.service.ts` around lines 135 - 145, The call
to EscrowManager::refund_escrow is incorrect: refund_escrow accepts a single u64
escrow_id and performs expiry checks, but the code (buildContractInvokeXDR
invoking 'refund_escrow' with loanIdBytes, borrower/lender) supplies three args
and encodes addresses as raw UTF‑8; also Loan has no persisted escrow id. Fix by
either (A) persisting the on‑chain escrow id on the Loan model and changing the
invoke to call the proper liquidation entrypoint (not 'refund_escrow') with a
single u64 escrow id (update the code using buildContractInvokeXDR and
loanIdBytes -> escrowIdU64), or (B) if using borrower/lender in the contract,
change the invoke to the contract's actual liquidation method and encode
borrowerId/lenderId as proper Soroban ScAddress/ed25519 public keys (not
TextEncoder bytes) when building args; update the Loan model only if you choose
option A so you can pass the correct escrow identifier to
buildContractInvokeXDR.
Owner
|
Hey, fix merge conflicts and failing ci |
Owner
|
Hi, are you active? |
Author
|
Yea I'll be done soon
…On Sat, Mar 28, 2026, 5:54 PM Eleazar Shekoaga Musa < ***@***.***> wrote:
*anonfedora* left a comment (anonfedora/stellovault#137)
<#137?email_source=notifications&email_token=AVFRPG3O4FSBUQFHAHFX64L4S77T7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJUHA2DCNRUHE2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4148416494>
Hi, are you active?
—
Reply to this email directly, view it on GitHub
<#137?email_source=notifications&email_token=AVFRPG3O4FSBUQFHAHFX64L4S77T7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJUHA2DCNRUHE2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4148416494>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVFRPG4LX7TZ2FOXKA66SU34S77T7AVCNFSM6AAAAACW7SK6MOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCNBYGQYTMNBZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Owner
|
Hi, CI for both contracts and Server are failing |
added 2 commits
March 30, 2026 01:14
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
server/src/services/liquidation.service.ts (2)
36-36:⚠️ Potential issue | 🔴 CriticalLine 36 still leaves
start()syntactically invalid.The
cron.schedule(opener is commented out, but the callback body and closing braces are still embedded after literal\nescapes on the same physical line, so the method/class no longer parse.#!/bin/bash python - <<'PY' from pathlib import Path path = Path("server/src/services/liquidation.service.ts") for i, line in enumerate(path.read_text().splitlines(), 1): if 30 <= i <= 38: print(f"{i}: {line!r}") PYExpected: Line 36 shows literal
\\nsequences inside a//comment, which swallowsawait this.performLiquidationChecks();and the closing}forstart().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/liquidation.service.ts` at line 36, The start() method is syntactically broken because the cron.schedule opener was commented out but the callback body and closing braces (including await this.performLiquidationChecks() and the end of the method) remain as literal "\n" text; fix by either fully restoring the multi-line cron.schedule block or fully commenting/removing the entire cron.schedule invocation so braces and await are valid; specifically update the code around start(), cron.schedule, cronJob, CHECK_INTERVAL_MINUTES, and performLiquidationChecks so the schedule call is a proper multi-line call (cron.schedule(`*/${CHECK_INTERVAL_MINUTES} * * * *`, async () => { await this.performLiquidationChecks(); } ) ) or remove those lines and ensure this.isRunning = true and the method closing brace remain syntactically correct.
132-139:⚠️ Potential issue | 🔴 CriticalThis still calls the wrong contract entrypoint for liquidation.
contracts/escrow-manager/src/lib.rsexposesrefund_escrow(env, escrow_id: u64), but Line 132 buildsrefund_escrowwith three args and passesloanIdbytes instead of the on-chain escrow id. Even if the arg count were fixed,refund_escrowrejects non-expired escrows, so this path cannot implement high-LTV liquidation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/liquidation.service.ts` around lines 132 - 139, The code currently calls contractService.buildContractInvokeXDR(escrowManagerId, 'refund_escrow', [...]) with three args (loanIdBytes and two addresses), but the on-chain function refund_escrow(env, escrow_id: u64) takes a single u64 escrow id and also rejects non-expired escrows; replace this call with the correct liquidation entrypoint (e.g., 'liquidate_escrow' or whatever function name is defined in contracts/escrow-manager/src/lib.rs) and pass the on-chain escrow id as a single xdr u64 argument (use xdr.ScVal.scvU64 or the project’s equivalent after converting the escrow id to number/BigInt); remove the borrowerId/lenderId address args and ensure you locate and use the canonical escrow id (not loanIdBytes) when calling buildContractInvokeXDR from this service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/services/liquidation.service.ts`:
- Around line 85-88: The balance lookup in liquidation.service.ts awaiting
blockchainService.getAccountBalance (which calls blockchainService.loadAccount)
can block the whole liquidation pass; wrap the call that produces
collateralValueStr in a bounded timeout and retry/backoff loop: implement a
helper that attempts blockchainService.getAccountBalance with a per-attempt
timeout (e.g., Promise.race + timer) and on failure retries with exponential
backoff and a max attempts limit, and replace the direct await with this helper
so triggerLiquidation cannot be stalled by a slow Horizon loadAccount call.
- Around line 144-168: The current catch around the whole function can re-run
submitXDR() and double-submit a successful on-chain liquidation; change the flow
so only the contract submission is retried: call submitXDR(xdrResult) inside its
own try/catch that implements the retry logic using retryCount, MAX_RETRIES and
LIQUIDATION_RETRY_DELAY_MS and invokes triggerLiquidation only for resubmitting
transactions, then after a successful submitXDR proceed to update
prisma.loan.update and websocketService.broadcastLoanUpdated inside a separate
try/catch that logs or schedules DB/websocket-only retries (but does NOT call
triggerLiquidation or re-run submitXDR) so database/websocket failures don’t
cause a second on-chain submission.
- Around line 90-92: The code computes currentLTV from raw token quantity
(collateralValueStr) instead of valuing collateral in loan currency; update the
path around getAccountBalance()/collateralValueStr to fetch the collateral price
in the loan currency (via your pricing/oracle service), multiply the returned
collateral amount by that price to produce a fiat/loan-currency collateralValue,
then replace the existing currentLTV calculation
(loanAmount.div(collateralValue).abs()) to use that priced collateralValue;
ensure you reference the same symbols (getAccountBalance, collateralValueStr,
collateralValue, loanAmount, currentLTV) so callers and tests still match.
---
Duplicate comments:
In `@server/src/services/liquidation.service.ts`:
- Line 36: The start() method is syntactically broken because the cron.schedule
opener was commented out but the callback body and closing braces (including
await this.performLiquidationChecks() and the end of the method) remain as
literal "\n" text; fix by either fully restoring the multi-line cron.schedule
block or fully commenting/removing the entire cron.schedule invocation so braces
and await are valid; specifically update the code around start(), cron.schedule,
cronJob, CHECK_INTERVAL_MINUTES, and performLiquidationChecks so the schedule
call is a proper multi-line call (cron.schedule(`*/${CHECK_INTERVAL_MINUTES} * *
* *`, async () => { await this.performLiquidationChecks(); } ) ) or remove those
lines and ensure this.isRunning = true and the method closing brace remain
syntactically correct.
- Around line 132-139: The code currently calls
contractService.buildContractInvokeXDR(escrowManagerId, 'refund_escrow', [...])
with three args (loanIdBytes and two addresses), but the on-chain function
refund_escrow(env, escrow_id: u64) takes a single u64 escrow id and also rejects
non-expired escrows; replace this call with the correct liquidation entrypoint
(e.g., 'liquidate_escrow' or whatever function name is defined in
contracts/escrow-manager/src/lib.rs) and pass the on-chain escrow id as a single
xdr u64 argument (use xdr.ScVal.scvU64 or the project’s equivalent after
converting the escrow id to number/BigInt); remove the borrowerId/lenderId
address args and ensure you locate and use the canonical escrow id (not
loanIdBytes) when calling buildContractInvokeXDR from this service.
🪄 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: 971f1b0a-e025-449a-b8b3-189b0ec1986b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
TODO.mdcontracts/protocol-treasury/src/lib.rsserver/src/services/liquidation.service.ts
✅ Files skipped from review due to trivial changes (1)
- TODO.md
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/protocol-treasury/src/lib.rs
Comment on lines
+85
to
+88
| const collateralValueStr = await blockchainService.getAccountBalance( | ||
| collateralWallet, | ||
| collateralAssetCode, | ||
| ); |
There was a problem hiding this comment.
Balance lookups still have no timeout boundary.
server/src/services/blockchain.service.ts makes a single loadAccount() call. Awaiting it inline here means one slow Horizon response can stall the entire liquidation pass, and the retry logic in triggerLiquidation() never helps because the scan is stuck before it gets there. Add a bounded timeout and retry/backoff around this fetch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/liquidation.service.ts` around lines 85 - 88, The balance
lookup in liquidation.service.ts awaiting blockchainService.getAccountBalance
(which calls blockchainService.loadAccount) can block the whole liquidation
pass; wrap the call that produces collateralValueStr in a bounded timeout and
retry/backoff loop: implement a helper that attempts
blockchainService.getAccountBalance with a per-attempt timeout (e.g.,
Promise.race + timer) and on failure retries with exponential backoff and a max
attempts limit, and replace the direct await with this helper so
triggerLiquidation cannot be stalled by a slow Horizon loadAccount call.
Comment on lines
+90
to
+92
| const collateralValue = new Decimal(collateralValueStr || '0'); | ||
|
|
||
| const currentLTV = loanAmount.div(collateralValue).abs(); |
There was a problem hiding this comment.
currentLTV is computed from token quantity, not collateral value.
getAccountBalance() returns the collateral amount, so this path compares loan size to raw asset units. That only works for permanently 1:1 assets; any non-pegged collateral will liquidate too early or too late. Price the collateral in the loan currency first, then use that value in the LTV formula.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/liquidation.service.ts` around lines 90 - 92, The code
computes currentLTV from raw token quantity (collateralValueStr) instead of
valuing collateral in loan currency; update the path around
getAccountBalance()/collateralValueStr to fetch the collateral price in the loan
currency (via your pricing/oracle service), multiply the returned collateral
amount by that price to produce a fiat/loan-currency collateralValue, then
replace the existing currentLTV calculation
(loanAmount.div(collateralValue).abs()) to use that priced collateralValue;
ensure you reference the same symbols (getAccountBalance, collateralValueStr,
collateralValue, loanAmount, currentLTV) so callers and tests still match.
Author
|
please merge |
Owner
|
CI for Contracts and server are failing |
Owner
|
Hi @Georgebingi fix failing CI so I can merge, please |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #125
close #118
Summary by CodeRabbit
Release Notes