feat: Implement Escrow Manager Core Functions & Resolve Server Config Issues#226
feat: Implement Escrow Manager Core Functions & Resolve Server Config Issues#226Hillzo wants to merge 2 commits into
Conversation
|
@Hillzo 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! 🚀 |
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
participant Lender as Lender
participant Escrow as Escrow Contract
participant OracleAdapter as Oracle Adapter
participant Settlement as Settlement Logic
Lender->>Escrow: fund_escrow(escrow_id, amount)
activate Escrow
Escrow->>Escrow: validate Initialized status
Escrow->>Escrow: transfer tokens, set funded_amount
Escrow->>Lender: emit esc_fndd
Escrow->>Escrow: transition to Active
deactivate Escrow
Note over OracleAdapter: Oracle confirms condition
OracleAdapter->>Escrow: confirm_condition(escrow_id, oracle_id, data, signature)
activate Escrow
Escrow->>OracleAdapter: validate oracle auth & forward confirmation
OracleAdapter-->>Escrow: confirmation result
Escrow->>Escrow: trigger release_funds_on_confirmation / auth checks
Escrow->>Settlement: execute payout, fees, collateral unlock
Escrow->>Escrow: transition to Released
Escrow->>Lender: emit esc_rel
deactivate Escrow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/escrow-manager/src/lib.rs (4)
1889-1907:⚠️ Potential issue | 🔴 CriticalCritical:
expiryis undefined —create_multi_oracle_escrowwill not compile.Line 1898 reads
expiry_ts: expiry,but noexpirybinding exists in this function. Declare it from the ledger timestamp like the other helpers.🔧 Proposed fix
fn create_multi_oracle_escrow(t: &TestEnv, threshold: u32, oracle_set: Vec<Address>) -> u64 { + let expiry = t.env.ledger().timestamp() + 3600; let id = t.escrow_client.create_escrow(&EscrowConfig {🤖 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 1889 - 1907, The helper create_multi_oracle_escrow has an undefined symbol expiry used at expiry_ts: expiry; fix by declaring expiry from the test ledger timestamp as done in other helpers: obtain expiry (e.g., let expiry = t.ledger().timestamp + <desired_offset>) inside create_multi_oracle_escrow before calling escrow_client.create_escrow; update the function (create_multi_oracle_escrow) so expiry is in scope and used for expiry_ts when calling t.escrow_client.create_escrow.
276-310:⚠️ Potential issue | 🔴 CriticalCritical:
coll_regis undefined — compilation failure, and inconsistent with the PR intent.Line 309 uses
&coll_reg, butcoll_regis never declared in this function. Per the AI summary,create_escrowwas supposed to stop locking collateral and transferring lender tokens, moving those responsibilities tofund_escrow. The leftoverlock_collateralinvoke call is both a compile error and a logic inconsistency: if creation no longer locks collateral,fund_escrowmust perform that lock (it currently does not).Decide one of two strategies and apply consistently:
- Remove collateral locking from
create_escrowentirely and do it infund_escrow(matches PR description).- Keep it here, but fetch the registry address from instance storage first (as other methods do).
🔧 Option 1 (recommended, matches PR description)
- let lock_args: Vec<Val> = Vec::from_array(&env, [config.collateral_id.into_val(&env)]); - env.invoke_contract::<Val>(&coll_reg, &Symbol::new(&env, "lock_collateral"), lock_args); - let escrow_id: u64 = env .storage() .instance() .get(&symbol_short!("next_id")) .unwrap_or(1);Then, in
fund_escrow, invokelock_collateralon the CollateralRegistry after the token transfer succeeds.🤖 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 276 - 310, create_escrow contains an undefined coll_reg and incorrectly calls lock_collateral; remove the collateral locking and the env.invoke_contract call from create_escrow so it only creates and stores the escrow metadata, and update fund_escrow to perform the collateral lock after the lender token transfer succeeds: in fund_escrow fetch the CollateralRegistry address the same way other methods do, then call lock_collateral on that contract (using the existing invocation pattern) and only mark the escrow funded after the lock succeeds; ensure all references to coll_reg in create_escrow are deleted to fix the compile error.
1809-1846:⚠️ Potential issue | 🔴 CriticalCritical:
test_path_payment_slippage_exceededno longer compiles and, if fixed, will now assert the wrong error.Two problems introduced by the lifecycle change:
- The
EscrowConfigliteral at lines 1823–1836 is missingrelease_code_hash: None, so it will not compile now that the struct has that required field.- After fixing that, this test does not call
fund_escrow, so the escrow stays inInitialized.release_funds_on_confirmationfirst checksstatus == Activeand will returnEscrowNotActive (#4)long before the slippage check runs. The#[should_panic(expected = "HostError: Error(Contract,#9)")]will fail.🔧 Proposed fix
required_confirmations: 0u32, oracle_set: Vec::new(&t.env), + release_code_hash: None, }); + t.escrow_client.fund_escrow(&escrow_id, &5000i128); // Set exchange rate: 0.95 (5% loss in conversion) t.escrow_client.set_test_exchange_rate(&950_000i128);🤖 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 1809 - 1846, The test test_path_payment_slippage_exceeded fails to compile because the EscrowConfig literal is missing the new required field release_code_hash and even if compiled it will hit EscrowNotActive instead of the slippage error because the escrow is never funded; update the EscrowConfig passed to escrow_client.create_escrow by adding release_code_hash: None, and after set_oracle_confirmation call the funding helper (e.g. t.escrow_client.fund_escrow(&escrow_id)) so the escrow moves to Active before invoking t.escrow_client.release_funds_on_confirmation(&escrow_id) to trigger the slippage check that produces the expected Contract error.
11-14:⚠️ Potential issue | 🔴 CriticalCritical:
BytesNis used but not imported — compilation will fail.
BytesN<32>is referenced at lines 81 and 109 (release_code_hash: Option<BytesN<32>>), but it is missing from thesoroban_sdk::{...}import list.🔧 Proposed fix
use soroban_sdk::{ - contract, contractimpl, contracttype, symbol_short, token, Address, Bytes, Env, IntoVal, - Symbol, Val, Vec, + contract, contractimpl, contracttype, symbol_short, token, Address, Bytes, BytesN, Env, + IntoVal, Symbol, Val, Vec, };🤖 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 11 - 14, The import list in the soroban_sdk prelude is missing BytesN which causes compilation errors where BytesN<32> is referenced (e.g., the release_code_hash: Option<BytesN<32>> fields); fix by adding BytesN to the soroban_sdk use statement so the type is available for the struct/field definitions (ensure the existing use line that contains Address, Bytes, Env, etc. also includes BytesN).
🧹 Nitpick comments (3)
server/tsconfig.json (2)
25-29:src/**/*.rsexclude is effectively a no-op.With
allowJs: true(notallowArbitraryExtensions), TypeScript only picks up.ts/.tsx/.js/.jsxfromincludeanyway, so excluding.rsfiles doesn't change compilation behavior. If Rust sources are being colocated underserver/src/, consider moving them outside the TSrootDir(e.g.,server/contracts/or a sibling crate directory) rather than relying on this exclude — mixing the two underrootDir: "./src"tends to confuse tooling (editors, linters, bundlers) downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/tsconfig.json` around lines 25 - 29, The "src/**/*.rs" entry in "exclude" is effectively a no-op because TypeScript's behavior (with allowJs) only considers files matched by "include" and the compiler's rootDir; update the project layout instead: move Rust sources out of the TypeScript root (e.g., relocate files from server/src to a sibling like server/contracts or a top-level Rust crate) or change tsconfig.json's "rootDir" so Rust files are not under it (do not rely on "exclude": ["src/**/*.rs"]); update any references to ensure build scripts and tooling point to the new Rust location and keep tsconfig.json's "allowJs", "include", and "rootDir" consistent.
18-20: Path alias is configured but not consumed.The
@prisma-generated/*alias is defined inserver/tsconfig.json, but none of the three existing imports use it:
server/src/config/prisma.ts:3server/src/services/oracle.service.ts:10server/src/services/governance.service.ts:2All continue to use relative paths (
../generated/prisma/client). Migrate the imports to the alias for consistency, or remove the unused alias.Also note:
ts-node,jest, and other runtime resolvers don't honortsconfigpaths without additional setup (e.g.,tsconfig-paths,jest.moduleNameMapper), and this configuration is not present. Confirm the alias resolves correctly in your runtime environment before relying on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/tsconfig.json` around lines 18 - 20, The tsconfig path alias "@prisma-generated/*" is declared but unused; either update the three imports that currently reference "../generated/prisma/client" (in the modules that import the Prisma client) to use the alias "@prisma-generated/client" for consistency, or remove the "paths" entry from server/tsconfig.json if you don't intend to use it. If you choose to adopt the alias, also ensure runtime tooling resolves it by adding tsconfig-paths registration for ts-node and mapping for tests (e.g., jest.moduleNameMapper) so imports like "@prisma-generated/client" work at runtime.contracts/escrow-manager/src/lib.rs (1)
1280-1305: Test name/intent mismatch after the create-then-fund split.
test_create_escrow_successnow invokescreate_test_escrow, which both creates and funds the escrow, then assertsstatus == Activeplus balance movements. This is really an end-to-end create+fund test — the original "just created" state (Initialized, no token transfer) is no longer covered anywhere. Consider adding a dedicated test that asserts the post-create_escrow/pre-fund_escrowinvariant: status isInitialized,funded_amount == 0, and the contract balance is unchanged. It would guard against regressions of the new state-machine split.🤖 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 1280 - 1305, The test name now hides that create_test_escrow both creates and funds an escrow; add a new unit test that calls create_escrow (not create_test_escrow) and asserts the post-create/pre-fund invariants: escrow.status == EscrowStatus::Initialized, escrow.funded_amount == 0, and the token contract balance for the escrow account is unchanged; keep the existing test_create_escrow_success as an end-to-end create+fund check (or rename it) and reference create_escrow, fund_escrow, EscrowStatus::Initialized, and funded_amount to locate the relevant code paths.
🤖 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 496-553: confirm_condition currently swallows all errors from
release_funds_on_confirmation and treats an empty escrow.oracle_set as "everyone
authorized"; update confirm_condition so (1) the oracle authorization loop
denies access if escrow.oracle_set.is_empty() when escrow.required_confirmation
> 0 (return an appropriate error like ContractError::Unauthorized or
ContractError::InvalidOracleSet) instead of allowing any caller, and (2) call
Self::release_funds_on_confirmation(env, escrow_id) and match its Result so that
only ConsensusNotMet/ConfirmationNotMet are ignored (treated as OK), while any
other Err is propagated back from confirm_condition; use the existing symbols
confirm_condition, release_funds_on_confirmation, oracle_set, and
required_confirmation to locate and implement these checks and error handling.
- Around line 369-406: The fund_escrow implementation must enforce exact
payment, proper status errors, auth ordering, lock collateral, and safe
check-effects-interactions: call escrow.lender.require_auth() at the start of
fund_escrow; validate that amount == escrow.amount (reject over/under funding)
and if escrow.status != EscrowStatus::Initialized return
ContractError::InvalidStatus (not AlreadyFunded); change the state to
escrow.status = EscrowStatus::Active (and persist it or otherwise mark it
locked) before calling token::Client::transfer to prevent re-entry, then perform
the transfer, set escrow.funded_amount = amount and also record/lock collateral
as required by create_escrow’s new semantics, persist the updated Escrow via
env.storage().persistent().set(&escrow_id, &escrow) and finally emit the event.
- Around line 409-450: The release_escrow function currently skips auth when a
matching release_code is provided and compares different hash types incorrectly;
fix by always enforcing authorization (e.g., call escrow.buyer.require_auth() or
a check that allows buyer/seller/admin before any payout) before calling
Self::execute_release_payout/collect_protocol_fee/unlock_collateral, and treat
the release_code check as an additional optional guard (not a replacement for
require_auth). Also correct the hash comparison by converting the returned
Hash<32> to BytesN<32> (use .to_bytes() / .into() on
env.crypto().keccak256(&code) or convert escrow.release_code_hash appropriately)
when comparing against escrow.release_code_hash. Ensure you still update
escrow.status and persist after the auth+validation and before emitting the
event.
- Around line 453-489: The add_oracle function currently calls non-existent
env.has_auth() and uses require_auth() incorrectly; change the signature to
accept caller: Address, remove env.has_auth() usage, and call
caller.require_auth() unconditionally, then verify caller == escrow.buyer or
caller == admin (where admin is read from instance storage) before proceeding;
also add a guard that aborts unless escrow.status is either Initialized or
Active (disallow when Released, Refunded, or Disputed) and keep the existing
duplicate-check and push to escrow.oracle_set before persisting and emitting the
event.
---
Outside diff comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 1889-1907: The helper create_multi_oracle_escrow has an undefined
symbol expiry used at expiry_ts: expiry; fix by declaring expiry from the test
ledger timestamp as done in other helpers: obtain expiry (e.g., let expiry =
t.ledger().timestamp + <desired_offset>) inside create_multi_oracle_escrow
before calling escrow_client.create_escrow; update the function
(create_multi_oracle_escrow) so expiry is in scope and used for expiry_ts when
calling t.escrow_client.create_escrow.
- Around line 276-310: create_escrow contains an undefined coll_reg and
incorrectly calls lock_collateral; remove the collateral locking and the
env.invoke_contract call from create_escrow so it only creates and stores the
escrow metadata, and update fund_escrow to perform the collateral lock after the
lender token transfer succeeds: in fund_escrow fetch the CollateralRegistry
address the same way other methods do, then call lock_collateral on that
contract (using the existing invocation pattern) and only mark the escrow funded
after the lock succeeds; ensure all references to coll_reg in create_escrow are
deleted to fix the compile error.
- Around line 1809-1846: The test test_path_payment_slippage_exceeded fails to
compile because the EscrowConfig literal is missing the new required field
release_code_hash and even if compiled it will hit EscrowNotActive instead of
the slippage error because the escrow is never funded; update the EscrowConfig
passed to escrow_client.create_escrow by adding release_code_hash: None, and
after set_oracle_confirmation call the funding helper (e.g.
t.escrow_client.fund_escrow(&escrow_id)) so the escrow moves to Active before
invoking t.escrow_client.release_funds_on_confirmation(&escrow_id) to trigger
the slippage check that produces the expected Contract error.
- Around line 11-14: The import list in the soroban_sdk prelude is missing
BytesN which causes compilation errors where BytesN<32> is referenced (e.g., the
release_code_hash: Option<BytesN<32>> fields); fix by adding BytesN to the
soroban_sdk use statement so the type is available for the struct/field
definitions (ensure the existing use line that contains Address, Bytes, Env,
etc. also includes BytesN).
---
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 1280-1305: The test name now hides that create_test_escrow both
creates and funds an escrow; add a new unit test that calls create_escrow (not
create_test_escrow) and asserts the post-create/pre-fund invariants:
escrow.status == EscrowStatus::Initialized, escrow.funded_amount == 0, and the
token contract balance for the escrow account is unchanged; keep the existing
test_create_escrow_success as an end-to-end create+fund check (or rename it) and
reference create_escrow, fund_escrow, EscrowStatus::Initialized, and
funded_amount to locate the relevant code paths.
In `@server/tsconfig.json`:
- Around line 25-29: The "src/**/*.rs" entry in "exclude" is effectively a no-op
because TypeScript's behavior (with allowJs) only considers files matched by
"include" and the compiler's rootDir; update the project layout instead: move
Rust sources out of the TypeScript root (e.g., relocate files from server/src to
a sibling like server/contracts or a top-level Rust crate) or change
tsconfig.json's "rootDir" so Rust files are not under it (do not rely on
"exclude": ["src/**/*.rs"]); update any references to ensure build scripts and
tooling point to the new Rust location and keep tsconfig.json's "allowJs",
"include", and "rootDir" consistent.
- Around line 18-20: The tsconfig path alias "@prisma-generated/*" is declared
but unused; either update the three imports that currently reference
"../generated/prisma/client" (in the modules that import the Prisma client) to
use the alias "@prisma-generated/client" for consistency, or remove the "paths"
entry from server/tsconfig.json if you don't intend to use it. If you choose to
adopt the alias, also ensure runtime tooling resolves it by adding
tsconfig-paths registration for ts-node and mapping for tests (e.g.,
jest.moduleNameMapper) so imports like "@prisma-generated/client" work at
runtime.
🪄 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: 895fcded-0316-4a92-83c6-8f29bacc39ca
📒 Files selected for processing (2)
contracts/escrow-manager/src/lib.rsserver/tsconfig.json
| /// Fund an initialized escrow. | ||
| pub fn fund_escrow(env: Env, escrow_id: u64, amount: i128) -> Result<(), ContractError> { | ||
| let mut escrow: Escrow = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&escrow_id) | ||
| .ok_or(ContractError::EscrowNotFound)?; | ||
|
|
||
| if escrow.status != EscrowStatus::Initialized { | ||
| return Err(ContractError::AlreadyFunded); | ||
| } | ||
|
|
||
| if amount < escrow.amount { | ||
| return Err(ContractError::InvalidAmount); | ||
| } | ||
|
|
||
| escrow.lender.require_auth(); | ||
|
|
||
| // Transfer funds from lender to this contract | ||
| let token_client = token::Client::new(&env, &escrow.asset); | ||
| token_client.transfer( | ||
| &escrow.lender, | ||
| &env.current_contract_address(), | ||
| &amount, | ||
| ); | ||
|
|
||
| escrow.funded_amount = amount; | ||
| escrow.status = EscrowStatus::Active; | ||
|
|
||
| env.storage().persistent().set(&escrow_id, &escrow); | ||
|
|
||
| env.events().publish( | ||
| (symbol_short!("esc_fndd"),), | ||
| (escrow_id, amount), | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical: fund_escrow has multiple correctness issues.
- Over-funding allowed.
if amount < escrow.amountpermitsamount > escrow.amount, transferring more than the agreed escrow out of the lender with no mechanism to return the surplus. Use exact equality. - Misleading error code. Returning
AlreadyFundedfor any non-Initializedstatus is wrong when the escrow isReleased,Refunded, orDisputed. UseInvalidStatus(already added to the enum) for the general case. - Auth ordering.
escrow.lender.require_auth()is called after validation reads, which is fine, but place it before any state mutation and ideally before reading mutable state to fail fast against unauthenticated calls. - Missing collateral lock. If
create_escrowno longer locks collateral (per PR description), that lock should happen here after the transfer succeeds. - **
funded_amountis overwritten on a single call — fine — but you may want to guard against re-entry by transitioning status toActivebefore the externaltransfercall to preserve check-effects-interactions ordering, or at minimum be aware thattoken::Client::transferinvokes another contract.
🔧 Proposed fix
pub fn fund_escrow(env: Env, escrow_id: u64, amount: i128) -> Result<(), ContractError> {
let mut escrow: Escrow = env
.storage()
.persistent()
.get(&escrow_id)
.ok_or(ContractError::EscrowNotFound)?;
+ escrow.lender.require_auth();
+
if escrow.status != EscrowStatus::Initialized {
- return Err(ContractError::AlreadyFunded);
+ return Err(match escrow.status {
+ EscrowStatus::Active | EscrowStatus::Released => ContractError::AlreadyFunded,
+ _ => ContractError::InvalidStatus,
+ });
}
- if amount < escrow.amount {
+ if amount != escrow.amount {
return Err(ContractError::InvalidAmount);
}
- escrow.lender.require_auth();
-
// Transfer funds from lender to this contract
let token_client = token::Client::new(&env, &escrow.asset);
token_client.transfer(
&escrow.lender,
&env.current_contract_address(),
&amount,
);📝 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.
| /// Fund an initialized escrow. | |
| pub fn fund_escrow(env: Env, escrow_id: u64, amount: i128) -> Result<(), ContractError> { | |
| let mut escrow: Escrow = env | |
| .storage() | |
| .persistent() | |
| .get(&escrow_id) | |
| .ok_or(ContractError::EscrowNotFound)?; | |
| if escrow.status != EscrowStatus::Initialized { | |
| return Err(ContractError::AlreadyFunded); | |
| } | |
| if amount < escrow.amount { | |
| return Err(ContractError::InvalidAmount); | |
| } | |
| escrow.lender.require_auth(); | |
| // Transfer funds from lender to this contract | |
| let token_client = token::Client::new(&env, &escrow.asset); | |
| token_client.transfer( | |
| &escrow.lender, | |
| &env.current_contract_address(), | |
| &amount, | |
| ); | |
| escrow.funded_amount = amount; | |
| escrow.status = EscrowStatus::Active; | |
| env.storage().persistent().set(&escrow_id, &escrow); | |
| env.events().publish( | |
| (symbol_short!("esc_fndd"),), | |
| (escrow_id, amount), | |
| ); | |
| Ok(()) | |
| } | |
| /// Fund an initialized escrow. | |
| pub fn fund_escrow(env: Env, escrow_id: u64, amount: i128) -> Result<(), ContractError> { | |
| let mut escrow: Escrow = env | |
| .storage() | |
| .persistent() | |
| .get(&escrow_id) | |
| .ok_or(ContractError::EscrowNotFound)?; | |
| escrow.lender.require_auth(); | |
| if escrow.status != EscrowStatus::Initialized { | |
| return Err(match escrow.status { | |
| EscrowStatus::Active | EscrowStatus::Released => ContractError::AlreadyFunded, | |
| _ => ContractError::InvalidStatus, | |
| }); | |
| } | |
| if amount != escrow.amount { | |
| return Err(ContractError::InvalidAmount); | |
| } | |
| // Transfer funds from lender to this contract | |
| let token_client = token::Client::new(&env, &escrow.asset); | |
| token_client.transfer( | |
| &escrow.lender, | |
| &env.current_contract_address(), | |
| &amount, | |
| ); | |
| escrow.funded_amount = amount; | |
| escrow.status = EscrowStatus::Active; | |
| env.storage().persistent().set(&escrow_id, &escrow); | |
| env.events().publish( | |
| (symbol_short!("esc_fndd"),), | |
| (escrow_id, amount), | |
| ); | |
| Ok(()) | |
| } |
🤖 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 369 - 406, The fund_escrow
implementation must enforce exact payment, proper status errors, auth ordering,
lock collateral, and safe check-effects-interactions: call
escrow.lender.require_auth() at the start of fund_escrow; validate that amount
== escrow.amount (reject over/under funding) and if escrow.status !=
EscrowStatus::Initialized return ContractError::InvalidStatus (not
AlreadyFunded); change the state to escrow.status = EscrowStatus::Active (and
persist it or otherwise mark it locked) before calling token::Client::transfer
to prevent re-entry, then perform the transfer, set escrow.funded_amount =
amount and also record/lock collateral as required by create_escrow’s new
semantics, persist the updated Escrow via
env.storage().persistent().set(&escrow_id, &escrow) and finally emit the event.
| pub fn release_escrow( | ||
| env: Env, | ||
| escrow_id: u64, | ||
| release_code: Option<Bytes>, | ||
| ) -> Result<(), ContractError> { | ||
| let mut escrow: Escrow = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&escrow_id) | ||
| .ok_or(ContractError::EscrowNotFound)?; | ||
|
|
||
| if escrow.status != EscrowStatus::Active { | ||
| return Err(ContractError::EscrowNotActive); | ||
| } | ||
|
|
||
| // Validate release code if configured | ||
| if let Some(hash) = &escrow.release_code_hash { | ||
| if let Some(code) = release_code { | ||
| if env.crypto().keccak256(&code) != *hash { | ||
| return Err(ContractError::InvalidReleaseCode); | ||
| } | ||
| } else { | ||
| return Err(ContractError::InvalidReleaseCode); | ||
| } | ||
| } else { | ||
| // If no release code is configured, only the buyer or an oracle can release | ||
| // In this specific function, we assume it's a manual release by buyer | ||
| escrow.buyer.require_auth(); | ||
| } | ||
|
|
||
| Self::execute_release_payout(&env, escrow_id, &escrow)?; | ||
| Self::collect_protocol_fee(&env, escrow_id, &escrow); | ||
| Self::unlock_collateral(&env, escrow.collateral_id)?; | ||
|
|
||
| escrow.status = EscrowStatus::Released; | ||
| env.storage().persistent().set(&escrow_id, &escrow); | ||
|
|
||
| env.events() | ||
| .publish((symbol_short!("esc_rel"),), (escrow_id,)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In soroban-sdk 22, what does env.crypto().keccak256(&bytes) return — Hash<32> or BytesN<32> — and how do you compare it against a stored BytesN<32>?
💡 Result:
In soroban-sdk 22, env.crypto.keccak256(&bytes) returns Hash<32>. Hash<32> is a wrapper around BytesN<32> that guarantees the bytes come from a secure cryptographic hash function. To compare it against a stored BytesN<32>, convert the Hash<32> to BytesN<32> using .to_bytes and then use ==: let computed: Hash<32> = env.crypto.keccak256(&input_bytes); let stored: BytesN<32> = // ... load from storage if computed.to_bytes == stored { // equal } Hash<32> also implements Into<BytesN<32>> and From<Hash<32>> for BytesN<32>, so computed.to_bytes or let bn: BytesN<32> = computed.into; works. Both types derive Eq and PartialEq, enabling direct byte comparison after conversion. Note that Hash should not be stored directly in storage.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/crypto/struct.Crypto.html
- 2: https://mintlify.com/stellar/rs-soroban-sdk/api/modules/crypto
- 3: https://docs.rs/soroban-sdk/latest/soroban_sdk/crypto/struct.Hash.html
- 4: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/crypto.rs.html
- 5: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.BytesN.html
🏁 Script executed:
cat -n contracts/escrow-manager/src/lib.rs | sed -n '409,450p'Repository: anonfedora/stellovault
Length of output: 1830
🏁 Script executed:
rg -A 20 "struct Escrow" contracts/escrow-manager/src/Repository: anonfedora/stellovault
Length of output: 4328
🏁 Script executed:
head -50 contracts/escrow-manager/src/lib.rs | cat -nRepository: anonfedora/stellovault
Length of output: 1722
🏁 Script executed:
rg "impl.*PartialEq" contracts/ --type rust -A 3 | head -40Repository: anonfedora/stellovault
Length of output: 48
🏁 Script executed:
rg "keccak256" contracts/ --type rust -B 2 -A 2Repository: anonfedora/stellovault
Length of output: 493
🏁 Script executed:
rg "release_code_hash" contracts/ --type rust -B 1 -A 1Repository: anonfedora/stellovault
Length of output: 3213
Critical security issue: release_escrow bypasses authentication when a valid release code is provided.
The control flow at lines 425–437 only enforces require_auth() in the else branch (when no release code is configured). If a release_code_hash is set and the caller supplies a matching preimage, the function proceeds directly to payout without any authentication check. This allows anyone who discovers the release code—which will become public once the transaction is confirmed on-chain—to drain the escrow. The only replay protection is the status transition to Released, which is insufficient for a sensitive operation.
Additionally, line 427 compares env.crypto().keccak256(&code) != *hash directly, comparing Hash<32> against BytesN<32>. According to soroban-sdk documentation, Hash<32> must be converted to BytesN<32> using .to_bytes() or .into() before comparison.
Recommended fix:
- Always require authentication from an authorized role (buyer, seller, or admin) before any release action.
- Validate the release code as an additional optional check, not as a replacement for auth.
🔧 Suggested direction
- // Validate release code if configured
- if let Some(hash) = &escrow.release_code_hash {
- if let Some(code) = release_code {
- if env.crypto().keccak256(&code) != *hash {
- return Err(ContractError::InvalidReleaseCode);
- }
- } else {
- return Err(ContractError::InvalidReleaseCode);
- }
- } else {
- // If no release code is configured, only the buyer or an oracle can release
- // In this specific function, we assume it's a manual release by buyer
- escrow.buyer.require_auth();
- }
+ // Manual release always requires buyer auth.
+ escrow.buyer.require_auth();
+
+ // If a release code is configured, also require it to be presented correctly.
+ if let Some(hash) = &escrow.release_code_hash {
+ let code = release_code.ok_or(ContractError::InvalidReleaseCode)?;
+ let computed: BytesN<32> = env.crypto().keccak256(&code).into();
+ if &computed != hash {
+ return Err(ContractError::InvalidReleaseCode);
+ }
+ }🤖 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 409 - 450, The
release_escrow function currently skips auth when a matching release_code is
provided and compares different hash types incorrectly; fix by always enforcing
authorization (e.g., call escrow.buyer.require_auth() or a check that allows
buyer/seller/admin before any payout) before calling
Self::execute_release_payout/collect_protocol_fee/unlock_collateral, and treat
the release_code check as an additional optional guard (not a replacement for
require_auth). Also correct the hash comparison by converting the returned
Hash<32> to BytesN<32> (use .to_bytes() / .into() on
env.crypto().keccak256(&code) or convert escrow.release_code_hash appropriately)
when comparing against escrow.release_code_hash. Ensure you still update
escrow.status and persist after the auth+validation and before emitting the
event.
| pub fn add_oracle(env: Env, escrow_id: u64, oracle_address: Address) -> Result<(), ContractError> { | ||
| let mut escrow: Escrow = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&escrow_id) | ||
| .ok_or(ContractError::EscrowNotFound)?; | ||
|
|
||
| // Only buyer or admin can add oracles | ||
| let admin: Address = env | ||
| .storage() | ||
| .instance() | ||
| .get(&symbol_short!("admin")) | ||
| .ok_or(ContractError::Unauthorized)?; | ||
|
|
||
| if env.has_auth() { | ||
| admin.require_auth(); | ||
| } else { | ||
| escrow.buyer.require_auth(); | ||
| } | ||
|
|
||
| // Check if oracle already exists | ||
| for existing in escrow.oracle_set.iter() { | ||
| if existing == oracle_address { | ||
| return Ok(()); // Already added | ||
| } | ||
| } | ||
|
|
||
| escrow.oracle_set.push_back(oracle_address); | ||
| env.storage().persistent().set(&escrow_id, &escrow); | ||
|
|
||
| env.events().publish( | ||
| (symbol_short!("orc_add"),), | ||
| (escrow_id, oracle_address), | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the soroban-sdk 22 Env type expose a method named has_auth (or similar) to check whether any address has currently authorized the invocation?
💡 Result:
No, the soroban-sdk v22 Env type does not expose a method named has_auth or any similar method to directly check whether any address has currently authorized the invocation. In Soroban, authorization is enforced declaratively through Address::require_auth or Address::require_auth_for_args, which panic if authorization is missing. The Env provides auths which returns the list of authorization trees seen during the last contract invocation (primarily for testing/verification), and mock_all_auths for testing. There is no method to proactively query current authorization status for an arbitrary address before calling require_auth. TokenClient and StellarAssetClient have authorized/try_authorized but these check token-specific authorization flags, not general invocation authorization.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Env.html
- 2: https://docs.rs/crate/soroban-sdk/22.0.9
- 3: https://www.uitscomputer.club
- 4: https://developers.stellar.org/docs/build/smart-contracts/example-contracts/auth
- 5: https://mintlify.com/stellar/rs-soroban-sdk/api/types/env
- 6: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Address.html
- 7: https://www.mintlify.com/stellar/rs-soroban-sdk/core/environment
- 8: https://www.mintlify.com/stellar/rs-soroban-sdk/core/authentication
- 9: https://mintlify.com/stellar/rs-soroban-sdk/api/types/address
🏁 Script executed:
# First, let's examine the exact file and lines mentioned
head -500 contracts/escrow-manager/src/lib.rs | tail -100Repository: anonfedora/stellovault
Length of output: 3240
🏁 Script executed:
# Also check the EscrowStatus definition to verify terminal states
rg "enum EscrowStatus" contracts/escrow-manager/src/lib.rs -A 10Repository: anonfedora/stellovault
Length of output: 269
🏁 Script executed:
# Check if there are other uses of authorization patterns in the contract
rg "require_auth" contracts/escrow-manager/src/lib.rs -B 2 -A 2Repository: anonfedora/stellovault
Length of output: 2559
Critical: env.has_auth() does not exist in soroban-sdk and the authorization logic is incorrect; also missing guard for terminal escrow states.
The code will not compile—env.has_auth() is not exposed by the Soroban SDK v22. The SDK provides Address::require_auth() to enforce authorization, but no method to proactively probe whether any address has authenticated. Additionally, the authorization pattern is semantically wrong: require_auth() enforces auth (panics if missing), so calling it conditionally on an unrelated predicate doesn't route the check intelligently. The correct pattern—already used elsewhere in this contract (e.g., dispute_escrow, renew_dispute_ttl)—is to accept caller: Address as an argument, call caller.require_auth() unconditionally, then check if the caller is authorized.
Finally, the function allows adding oracles to escrows in any state. This should be restricted to Initialized and Active states; once an escrow reaches Released, Refunded, or Disputed, no further mutations to the oracle set should be allowed.
🔧 Suggested redesign
- pub fn add_oracle(env: Env, escrow_id: u64, oracle_address: Address) -> Result<(), ContractError> {
+ pub fn add_oracle(
+ env: Env,
+ escrow_id: u64,
+ caller: Address,
+ oracle_address: Address,
+ ) -> Result<(), ContractError> {
+ caller.require_auth();
+
let mut escrow: Escrow = env
.storage()
.persistent()
.get(&escrow_id)
.ok_or(ContractError::EscrowNotFound)?;
- // Only buyer or admin can add oracles
+ if !matches!(escrow.status, EscrowStatus::Initialized | EscrowStatus::Active) {
+ return Err(ContractError::InvalidStatus);
+ }
+
let admin: Address = env
.storage()
.instance()
.get(&symbol_short!("admin"))
.ok_or(ContractError::Unauthorized)?;
- if env.has_auth() {
- admin.require_auth();
- } else {
- escrow.buyer.require_auth();
- }
+ if caller != admin && caller != escrow.buyer {
+ return Err(ContractError::Unauthorized);
+ }🤖 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 453 - 489, The add_oracle
function currently calls non-existent env.has_auth() and uses require_auth()
incorrectly; change the signature to accept caller: Address, remove
env.has_auth() usage, and call caller.require_auth() unconditionally, then
verify caller == escrow.buyer or caller == admin (where admin is read from
instance storage) before proceeding; also add a guard that aborts unless
escrow.status is either Initialized or Active (disallow when Released, Refunded,
or Disputed) and keep the existing duplicate-check and push to escrow.oracle_set
before persisting and emitting the event.
| pub fn confirm_condition( | ||
| env: Env, | ||
| escrow_id: u64, | ||
| oracle_id: Address, | ||
| confirmation_data: Bytes, | ||
| signature: Bytes, | ||
| ) -> Result<(), ContractError> { | ||
| oracle_id.require_auth(); | ||
|
|
||
| let escrow: Escrow = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&escrow_id) | ||
| .ok_or(ContractError::EscrowNotFound)?; | ||
|
|
||
| // Verify oracle is in the authorized set | ||
| let mut authorized = false; | ||
| for oracle in escrow.oracle_set.iter() { | ||
| if oracle == oracle_id { | ||
| authorized = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !authorized && !escrow.oracle_set.is_empty() { | ||
| return Err(ContractError::Unauthorized); | ||
| } | ||
|
|
||
| // Record confirmation in OracleAdapter (via cross-contract call) | ||
| let oracle_adapter: Address = env | ||
| .storage() | ||
| .instance() | ||
| .get(&symbol_short!("oracle")) | ||
| .ok_or(ContractError::Unauthorized)?; | ||
|
|
||
| let escrow_id_bytes = Bytes::from_slice(&env, &escrow_id.to_be_bytes()); | ||
| let confirm_args: Vec<Val> = Vec::from_array( | ||
| &env, | ||
| [ | ||
| oracle_id.into_val(&env), | ||
| escrow_id_bytes.into_val(&env), | ||
| escrow.required_confirmation.into_val(&env), | ||
| confirmation_data.into_val(&env), | ||
| signature.into_val(&env), | ||
| ], | ||
| ); | ||
|
|
||
| env.invoke_contract::<Val>( | ||
| &oracle_adapter, | ||
| &Symbol::new(&env, "confirm_event"), | ||
| confirm_args, | ||
| ); | ||
|
|
||
| // Check if consensus is met and auto-release if so | ||
| let _ = Self::release_funds_on_confirmation(env, escrow_id); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Silent error swallowing on auto-release and fragile auth check.
Line 550: let _ = Self::release_funds_on_confirmation(env, escrow_id); discards every error returned by the release path — including genuine failures like SlippageExceeded, PathPaymentFailed, or storage errors — so an oracle caller would see "success" on confirm_condition even when the downstream release attempt (which did touch storage / invoked external contracts) actually failed. Only ConsensusNotMet/ConfirmationNotMet are "benign" here; propagate the rest.
Additionally, the authorized-oracle check at lines 512–522 treats "escrow has empty oracle_set" as "any oracle authorized". For single-oracle-mode escrows (required_confirmations == 0), this happens to be fine, but for explicitly-configured multi-oracle escrows you should probably never reach here with an empty set.
🔧 Suggested fix
- // Check if consensus is met and auto-release if so
- let _ = Self::release_funds_on_confirmation(env, escrow_id);
+ // Attempt auto-release; treat "not yet enough confirmations" as success,
+ // propagate any other error.
+ match Self::release_funds_on_confirmation(env, escrow_id) {
+ Ok(()) | Err(ContractError::ConsensusNotMet) | Err(ContractError::ConfirmationNotMet) => {}
+ Err(e) => return Err(e),
+ }
Ok(())
}📝 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.
| pub fn confirm_condition( | |
| env: Env, | |
| escrow_id: u64, | |
| oracle_id: Address, | |
| confirmation_data: Bytes, | |
| signature: Bytes, | |
| ) -> Result<(), ContractError> { | |
| oracle_id.require_auth(); | |
| let escrow: Escrow = env | |
| .storage() | |
| .persistent() | |
| .get(&escrow_id) | |
| .ok_or(ContractError::EscrowNotFound)?; | |
| // Verify oracle is in the authorized set | |
| let mut authorized = false; | |
| for oracle in escrow.oracle_set.iter() { | |
| if oracle == oracle_id { | |
| authorized = true; | |
| break; | |
| } | |
| } | |
| if !authorized && !escrow.oracle_set.is_empty() { | |
| return Err(ContractError::Unauthorized); | |
| } | |
| // Record confirmation in OracleAdapter (via cross-contract call) | |
| let oracle_adapter: Address = env | |
| .storage() | |
| .instance() | |
| .get(&symbol_short!("oracle")) | |
| .ok_or(ContractError::Unauthorized)?; | |
| let escrow_id_bytes = Bytes::from_slice(&env, &escrow_id.to_be_bytes()); | |
| let confirm_args: Vec<Val> = Vec::from_array( | |
| &env, | |
| [ | |
| oracle_id.into_val(&env), | |
| escrow_id_bytes.into_val(&env), | |
| escrow.required_confirmation.into_val(&env), | |
| confirmation_data.into_val(&env), | |
| signature.into_val(&env), | |
| ], | |
| ); | |
| env.invoke_contract::<Val>( | |
| &oracle_adapter, | |
| &Symbol::new(&env, "confirm_event"), | |
| confirm_args, | |
| ); | |
| // Check if consensus is met and auto-release if so | |
| let _ = Self::release_funds_on_confirmation(env, escrow_id); | |
| Ok(()) | |
| } | |
| pub fn confirm_condition( | |
| env: Env, | |
| escrow_id: u64, | |
| oracle_id: Address, | |
| confirmation_data: Bytes, | |
| signature: Bytes, | |
| ) -> Result<(), ContractError> { | |
| oracle_id.require_auth(); | |
| let escrow: Escrow = env | |
| .storage() | |
| .persistent() | |
| .get(&escrow_id) | |
| .ok_or(ContractError::EscrowNotFound)?; | |
| // Verify oracle is in the authorized set | |
| let mut authorized = false; | |
| for oracle in escrow.oracle_set.iter() { | |
| if oracle == oracle_id { | |
| authorized = true; | |
| break; | |
| } | |
| } | |
| if !authorized && !escrow.oracle_set.is_empty() { | |
| return Err(ContractError::Unauthorized); | |
| } | |
| // Record confirmation in OracleAdapter (via cross-contract call) | |
| let oracle_adapter: Address = env | |
| .storage() | |
| .instance() | |
| .get(&symbol_short!("oracle")) | |
| .ok_or(ContractError::Unauthorized)?; | |
| let escrow_id_bytes = Bytes::from_slice(&env, &escrow_id.to_be_bytes()); | |
| let confirm_args: Vec<Val> = Vec::from_array( | |
| &env, | |
| [ | |
| oracle_id.into_val(&env), | |
| escrow_id_bytes.into_val(&env), | |
| escrow.required_confirmation.into_val(&env), | |
| confirmation_data.into_val(&env), | |
| signature.into_val(&env), | |
| ], | |
| ); | |
| env.invoke_contract::<Val>( | |
| &oracle_adapter, | |
| &Symbol::new(&env, "confirm_event"), | |
| confirm_args, | |
| ); | |
| // Attempt auto-release; treat "not yet enough confirmations" as success, | |
| // propagate any other error. | |
| match Self::release_funds_on_confirmation(env, escrow_id) { | |
| Ok(()) | Err(ContractError::ConsensusNotMet) | Err(ContractError::ConfirmationNotMet) => {} | |
| Err(e) => return Err(e), | |
| } | |
| Ok(()) | |
| } |
🤖 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 496 - 553,
confirm_condition currently swallows all errors from
release_funds_on_confirmation and treats an empty escrow.oracle_set as "everyone
authorized"; update confirm_condition so (1) the oracle authorization loop
denies access if escrow.oracle_set.is_empty() when escrow.required_confirmation
> 0 (return an appropriate error like ContractError::Unauthorized or
ContractError::InvalidOracleSet) instead of allowing any caller, and (2) call
Self::release_funds_on_confirmation(env, escrow_id) and match its Result so that
only ConsensusNotMet/ConfirmationNotMet are ignored (treated as OK), while any
other Err is propagated back from confirm_condition; use the existing symbols
confirm_condition, release_funds_on_confirmation, oracle_set, and
required_confirmation to locate and implement these checks and error handling.
|
FIX issues for your implementation, please |
|
Fix ci, and merge conflicts, please |
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 (4)
contracts/escrow-manager/src/lib.rs (4)
304-305:⚠️ Potential issue | 🔴 CriticalCritical:
coll_regis undefined increate_escrow.The previous version looked up
coll_regfrom instance storage before invokinglock_collateral. That lookup has been removed but theinvoke_contractcall at line 305 still referencescoll_regas a variable, so the function won't compile. Restore the lookup (or factor it through the existingunlock_collateral-style helper).🔧 Proposed fix
+ let coll_reg: Address = env + .storage() + .instance() + .get(&symbol_short!("coll_reg")) + .ok_or(ContractError::Unauthorized)?; + let lock_args: Vec<Val> = Vec::from_array(&env, [config.collateral_id.into_val(&env)]); env.invoke_contract::<Val>(&coll_reg, &Symbol::new(&env, "lock_collateral"), lock_args);🤖 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 304 - 305, The call in create_escrow references an undefined coll_reg; restore the contract instance lookup from storage (as done previously) or reuse the existing helper used by unlock_collateral to obtain the collateral contract instance before invoking lock_collateral. Specifically, in create_escrow ensure you fetch the collateral registry instance (assign to coll_reg) from instance storage or call the helper that returns the contract instance, then build lock_args and call env.invoke_contract::<Val>(&coll_reg, &Symbol::new(&env, "lock_collateral"), lock_args) so coll_reg is defined.
1829-1842:⚠️ Potential issue | 🔴 Critical
test_path_payment_slippage_exceededis missing the newrelease_code_hashfield.Every other
EscrowConfigliteral in this file was updated whenrelease_code_hash: Option<BytesN<32>>was added, but this one was not, so the test fails to compile.🔧 Proposed fix
required_confirmations: 0u32, oracle_set: Vec::new(&t.env), + release_code_hash: None, });🤖 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 1829 - 1842, The EscrowConfig literal in test_path_payment_slippage_exceeded is missing the new release_code_hash field; update the EscrowConfig passed to t.escrow_client.create_escrow(...) to include release_code_hash: None (or an appropriate Option<BytesN<32>> value) so the struct literal matches the EscrowConfig definition and the test compiles.
1895-1913:⚠️ Potential issue | 🔴 Critical
create_multi_oracle_escrowreferences an undefinedexpiry.
expiry_ts: expiryat line 1904 is used without ever bindingexpiryin this helper (compare withcreate_test_escrow, which declareslet expiry = t.env.ledger().timestamp() + 3600;). The helper won't compile, and everytest_multi_oracle_*that depends on it will be broken.🔧 Proposed fix
fn create_multi_oracle_escrow(t: &TestEnv, threshold: u32, oracle_set: Vec<Address>) -> u64 { + let expiry = t.env.ledger().timestamp() + 3600; let id = t.escrow_client.create_escrow(&EscrowConfig {🤖 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 1895 - 1913, The helper create_multi_oracle_escrow uses expiry_ts: expiry but never declares expiry; fix by declaring an expiry timestamp before building the EscrowConfig (e.g. compute let expiry = t.env.ledger().timestamp() + 3600; or another appropriate offset) and use that expiry in EscrowConfig; update the function create_multi_oracle_escrow so the local variable expiry is defined (similar to create_test_escrow) prior to calling t.escrow_client.create_escrow.
11-14:⚠️ Potential issue | 🔴 CriticalCritical:
BytesNis used but not imported — this file will not compile.
Option<BytesN<32>>is referenced at lines 81 and 109, but theuse soroban_sdk::{...}block does not bringBytesNinto scope. AddBytesNto the import list.🔧 Proposed fix
use soroban_sdk::{ - contract, contractimpl, contracttype, symbol_short, token, Address, Bytes, Env, IntoVal, - Symbol, Val, Vec, + contract, contractimpl, contracttype, symbol_short, token, Address, Bytes, BytesN, Env, + IntoVal, Symbol, Val, Vec, };🤖 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 11 - 14, The code references Option<BytesN<32>> but BytesN is not imported, causing a compile error; update the soroban_sdk import list to include BytesN so the type Option<BytesN<32>> used in the contract (e.g., in the structs/functions that reference BytesN) resolves correctly and the file compiles.
🧹 Nitpick comments (1)
contracts/escrow-manager/src/lib.rs (1)
254-271: Stale doc comment forcreate_escrow.The doc still says creation "transfers funds from the lender into this contract", but with the new lifecycle the lender's funds are only locked once
fund_escrowis called. Update the doc to describe the two-stepcreate → fundflow and documentrelease_code_hashand the newInitializedstatus, otherwise integrators will misuse the API.🤖 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 254 - 271, Update the doc comment for create_escrow to reflect the new two-step lifecycle: creation now only sets up escrow metadata and locks collateral, while lender funds are locked when fund_escrow is called; mention the new Initialized status (e.g., status variant Initialized) and document the new release_code_hash parameter (what it represents and how it is used during release) so callers know to call fund_escrow after create_escrow and that no lender funds are transferred during creation.
🤖 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 1696-1740: The contract is missing a public getter for merchant
escrows: implement a method named get_merchant_escrows on the EscrowManager
contract that reads and returns the indexed vector stored under the
(symbol_short!("seller"), seller) index (the same index populated at lines that
call symbol_short!("seller") for storing escrow IDs) so the generated
EscrowManagerClient can call it; the function should accept a seller Address and
return the vector/collection type used for escrow IDs in storage, e.g., the
indexed Vec<...> type your contract uses, and simply fetch and return that value
(preserving the existing ID element type and error handling conventions used by
other getters).
---
Outside diff comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 304-305: The call in create_escrow references an undefined
coll_reg; restore the contract instance lookup from storage (as done previously)
or reuse the existing helper used by unlock_collateral to obtain the collateral
contract instance before invoking lock_collateral. Specifically, in
create_escrow ensure you fetch the collateral registry instance (assign to
coll_reg) from instance storage or call the helper that returns the contract
instance, then build lock_args and call env.invoke_contract::<Val>(&coll_reg,
&Symbol::new(&env, "lock_collateral"), lock_args) so coll_reg is defined.
- Around line 1829-1842: The EscrowConfig literal in
test_path_payment_slippage_exceeded is missing the new release_code_hash field;
update the EscrowConfig passed to t.escrow_client.create_escrow(...) to include
release_code_hash: None (or an appropriate Option<BytesN<32>> value) so the
struct literal matches the EscrowConfig definition and the test compiles.
- Around line 1895-1913: The helper create_multi_oracle_escrow uses expiry_ts:
expiry but never declares expiry; fix by declaring an expiry timestamp before
building the EscrowConfig (e.g. compute let expiry = t.env.ledger().timestamp()
+ 3600; or another appropriate offset) and use that expiry in EscrowConfig;
update the function create_multi_oracle_escrow so the local variable expiry is
defined (similar to create_test_escrow) prior to calling
t.escrow_client.create_escrow.
- Around line 11-14: The code references Option<BytesN<32>> but BytesN is not
imported, causing a compile error; update the soroban_sdk import list to include
BytesN so the type Option<BytesN<32>> used in the contract (e.g., in the
structs/functions that reference BytesN) resolves correctly and the file
compiles.
---
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 254-271: Update the doc comment for create_escrow to reflect the
new two-step lifecycle: creation now only sets up escrow metadata and locks
collateral, while lender funds are locked when fund_escrow is called; mention
the new Initialized status (e.g., status variant Initialized) and document the
new release_code_hash parameter (what it represents and how it is used during
release) so callers know to call fund_escrow after create_escrow and that no
lender funds are transferred during creation.
🪄 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: 5059efd5-c94f-460e-b2a0-173fcc70ee21
📒 Files selected for processing (1)
contracts/escrow-manager/src/lib.rs
| #[test] | ||
| fn test_get_merchant_escrows() { | ||
| let t = setup(); | ||
|
|
||
| // Create multiple escrows with the same seller | ||
| let escrow_id1 = create_test_escrow(&t); | ||
| let escrow_id2 = create_test_escrow(&t); | ||
|
|
||
| // Create an escrow with a different seller | ||
| let different_seller = Address::generate(&t.env); | ||
| let expiry = t.env.ledger().timestamp() + 3600; | ||
| let escrow_id3 = t.escrow_client.create_escrow(&EscrowConfig { | ||
| buyer: t.buyer.clone(), | ||
| seller: different_seller.clone(), | ||
| lender: t.lender.clone(), | ||
| collateral_id: 1u64, | ||
| amount: 5000i128, | ||
| asset: t.token_addr.clone(), | ||
| required_confirmation: 2u32, | ||
| expiry_ts: expiry, | ||
| destination_asset: t.token_addr.clone(), | ||
| min_destination_amount: 5000i128, | ||
| required_confirmations: 0u32, | ||
| oracle_set: Vec::new(&t.env), | ||
| release_code_hash: None, | ||
| }); | ||
| t.escrow_client.fund_escrow(&escrow_id3, &5000i128); | ||
|
|
||
| // Query escrows for the original seller | ||
| let seller_escrows = t.escrow_client.get_merchant_escrows(&t.seller); | ||
| assert_eq!(seller_escrows.len(), 2); | ||
| assert!(seller_escrows.iter().any(|id| id == escrow_id1)); | ||
| assert!(seller_escrows.iter().any(|id| id == escrow_id2)); | ||
| assert!(!seller_escrows.iter().any(|id| id == escrow_id3)); | ||
|
|
||
| // Query escrows for the different seller | ||
| let different_seller_escrows = t.escrow_client.get_merchant_escrows(&different_seller); | ||
| assert_eq!(different_seller_escrows.len(), 1); | ||
| assert_eq!(different_seller_escrows.get(0).unwrap(), escrow_id3); | ||
|
|
||
| // Query escrows for an address with no escrows | ||
| let no_escrows_address = Address::generate(&t.env); | ||
| let no_escrows = t.escrow_client.get_merchant_escrows(&no_escrows_address); | ||
| assert_eq!(no_escrows.len(), 0); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether get_merchant_escrows is defined anywhere in the contracts tree.
rg -nP '\bfn\s+get_merchant_escrows\b' contracts/
ast-grep --pattern $'pub fn get_merchant_escrows($$$) { $$$ }'Repository: anonfedora/stellovault
Length of output: 48
🏁 Script executed:
# First, let's check the structure of the escrow-manager directory
find contracts/escrow-manager -type f -name "*.rs" | head -20Repository: anonfedora/stellovault
Length of output: 191
🏁 Script executed:
# Get the size of lib.rs to determine how to read it
wc -l contracts/escrow-manager/src/lib.rsRepository: anonfedora/stellovault
Length of output: 107
🏁 Script executed:
cat -n contracts/escrow-manager/src/lib.rsRepository: anonfedora/stellovault
Length of output: 50378
🏁 Script executed:
sed -n '1696,1740p' contracts/escrow-manager/src/lib.rsRepository: anonfedora/stellovault
Length of output: 2021
🏁 Script executed:
sed -n '142,1016p' contracts/escrow-manager/src/lib.rs | grep -n 'pub fn'Repository: anonfedora/stellovault
Length of output: 1343
Add missing get_merchant_escrows function to EscrowManager contract.
The test at lines 1696-1740 calls get_merchant_escrows via the client, but this function is not implemented in the contract. The indexing infrastructure already exists (lines 341-349 store escrow IDs under (symbol_short!("seller"), seller)), but the corresponding public getter is missing. Without it, the test will fail to compile and any caller of the generated EscrowManagerClient cannot retrieve seller escrows. Implement the function to return the indexed vector of escrow IDs for a given merchant address.
🤖 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 1696 - 1740, The contract
is missing a public getter for merchant escrows: implement a method named
get_merchant_escrows on the EscrowManager contract that reads and returns the
indexed vector stored under the (symbol_short!("seller"), seller) index (the
same index populated at lines that call symbol_short!("seller") for storing
escrow IDs) so the generated EscrowManagerClient can call it; the function
should accept a seller Address and return the vector/collection type used for
escrow IDs in storage, e.g., the indexed Vec<...> type your contract uses, and
simply fetch and return that value (preserving the existing ID element type and
error handling conventions used by other getters).
|
You have issues with the CI |
|
CI,please |
3 participants
closes #212
Summary of Changes:
Smart Contract: Implemented core functions for the Escrow Manager, including fund_escrow, release_escrow (manual), add_oracle, and confirm_condition.
State Management: Added an Initialized state to support a secure "create-then-fund" lifecycle.
Consensus Engine: Integrated multi-oracle threshold validation via the OracleAdapter to trigger automatic fund release.
Configuration: Fixed server/tsconfig.json by correcting the @prisma-generated path alias, removing invalid trailing commas, and adjusting rootDir to prevent compilation errors.
Reason for Changes:
Fulfills the requirements of Issue #212 to build a robust, multi-signature/oracle-based escrow system for trade finance.
Stabilizes the server-side build environment by resolving blocking TypeScript errors in the configuration.
Summary by CodeRabbit
New Features
Tests