🚨 EVERY CI CHECK MUST PASS. NO EXCEPTIONS. 🚨
Your PR will not be reviewed or merged unless every single job on the CI workflow is green: Formatting, Clippy, Build Optimized WASM, Tests, Dependency Audit, Coverage, and SDK Error Code Parity. No skipped jobs, no "unrelated failure", no #[ignore] to make a test go away. If a check is red, the work is not finished.
Summary
release_partial is a public entry point on a deployed escrow contract that takes an escrow ID and an amount, ignores both, moves no funds, and returns Ok(()).
// contracts/marketx/src/lib.rs:1522
pub fn release_partial(env: Env, _escrow_id: u64, _amount: i128) -> Result<(), ContractError> {
Self::assert_not_paused(&env)?;
Self::assert_partial_releases_enabled(&env)?;
Ok(())
}That is the whole function. The leading underscores on _escrow_id and _amount are the tell — they exist to silence the unused-parameter warning, which is the only reason Clippy with -D warnings stays green on it.
There is no require_auth(). No escrow lookup. No state check. No amount validation. No token transfer. No event. No counter update. Anyone can call it, with any arguments, and receive a successful transaction.
Why this is worse than an unimplemented function
An unimplemented function that panics or returns an error is safe: the caller learns nothing happened. This one reports success.
A transaction calling release_partial lands on-chain, succeeds, and appears in the caller's transaction history as a completed contract invocation. An SDK, a frontend, an integrator's backend, or a retry-on-failure job all read that as the partial release went through. Nothing did. And because no event is emitted, there is not even a negative signal an indexer could use to detect the discrepancy — the escrow simply still holds the full amount while the caller believes part of it was released.
I confirmed the behaviour by wiring a probe test into the existing suite:
client.initialize(&admin, &collector, &250, &0, &0);
client.set_partial_releases_enabled(&true);
// Escrow 999_999 does not exist. The amount is negative.
let r = client.try_release_partial(&999_999u64, &-5i128);
assert_eq!(r, Ok(Ok(())), "release_partial reported success");
assert_eq!(client.get_total_released_amount(), 0);
assert_eq!(client.get_total_released_count(), 0);
assert_eq!(client.get_total_escrows(), 0);
test test::temp_probe_release_partial_is_a_silent_noop ... ok
A non-existent escrow and a negative amount both return success. Every one of those three conditions should be fatal:
| Condition |
Correct behaviour |
Actual |
escrow 999_999 does not exist |
EscrowNotFound (10) |
Ok(()) |
amount is -5 |
InvalidEscrowAmount (13) |
Ok(()) |
| caller is not the buyer |
Unauthorized (2) |
Ok(()) |
Note also that release_partial never calls require_auth() at all, so it does not even establish who is asking. Compare release_item immediately below it, which is the properly-implemented partial-release path, and refund_escrow, which does initiator.require_auth() and then checks initiator != escrow.buyer.
The probe was reverted; main is unmodified and still reports 128 passing tests.
The existing test actively hides this
release_partial is referenced by the test suite — exactly once, in test.rs:270:
#[test]
fn disabled_feature_flags_block_paths() {
// ...
client.set_partial_releases_enabled(&false);
let partial_release = client.try_release_partial(&1u64, &1i128);
assert_eq!(partial_release, Err(Ok(ContractError::FeatureDisabled)));The one test that touches this function exercises it with the feature flag turned off, and asserts only that the flag check fires. The enabled path — the no-op — has zero coverage.
This is the most dangerous shape a test can have. A reviewer grepping for release_partial finds a passing test and moves on. Coverage tooling counts the function as exercised. The function is "tested" in every mechanical sense, and the actual behaviour under the only configuration that matters was never asserted.
The one warning that exists is buried and stale
README.md:369 is the sole place a reader is told not to trust this function:
release_partial, refund_escrow, and broader pending-state transitions are still placeholders and should not yet be treated as production-ready flows.
Two problems.
First, it is in the wrong place. It sits in a prose section called "Current Implementation Notes" near the bottom of the README, nowhere near the API documentation, and — critically — nowhere near the contract. A caller integrating against the deployed WASM has the contract spec, not the README. On-chain, the disclaimer does not exist.
Second, it is factually wrong about refund_escrow. That function is fully implemented (lib.rs:1711):
Self::assert_not_paused(&env)?;
Self::assert_disputes_enabled(&env)?;
initiator.require_auth();
Self::validate_bytes_size(&evidence_hash, MAX_EVIDENCE_HASH_SIZE)?;
let mut escrow: Escrow = ... .ok_or(ContractError::EscrowNotFound)?;
if initiator != escrow.buyer { return Err(ContractError::Unauthorized); }
Self::assert_escrow_funded(&escrow)?;
if amount <= 0 || amount > escrow.amount { return Err(ContractError::InvalidEscrowAmount); }
let request_id = Self::next_refund_id(&env)?;
// ... constructs and persists a RefundRequestAuth, ownership, state, and amount validation, then real persisted state. Whatever this note described, it has not been true of refund_escrow for some time. So the single document a contributor might rely on is wrong in one direction (understating refund_escrow) while being the only thing standing between an integrator and a silent no-op in the other. It cannot be trusted either way.
Decide the contract's actual intent
This needs a deliberate decision, not a patch. Pick one and say which in the PR:
Option A — implement it. release_partial(escrow_id, amount) releases an arbitrary amount rather than a discrete item. It must:
require_auth() the buyer, and reject any other caller with Unauthorized- load the escrow, returning
EscrowNotFound when absent
- reject
amount <= 0 and amount > remaining with InvalidEscrowAmount
- apply the fee consistently with
release_escrow and release_item — reuse the existing fee helper, do not reimplement the arithmetic
- transfer, persist the reduced remaining balance, and transition to
Released once nothing remains
- emit
FundsReleasedEvent and update TotalReleasedAmount / TotalReleasedCount
- interact correctly with
release_item on an itemised escrow — an escrow must not be drainable through both paths. State explicitly in the PR what happens when both are used on the same escrow, and test it.
Option B — make it fail loudly. If item-based release via release_item is the intended design and release_partial is redundant, it must stop returning success. Either remove the entry point outright, or have it return a clear error. A new NotImplemented variant in errors.rs is acceptable, but note it must be mirrored in sdk/error-codes.ts or the SDK Error Code Parity job will fail — that is the job doing its work, not an obstacle to route around.
Option A is preferred if a free-amount partial release is genuinely wanted, since release_item only covers itemised escrows. Either way, the current state — a function that succeeds without acting — is not an option.
Also in scope
- Fix the README. Correct the stale
refund_escrow claim, and move any remaining caveat next to the function's own doc comment where a reader of the code will see it. release_partial currently has no doc comment at all, while release_item directly beneath it has a full one with # Arguments and # Errors sections. Match that standard.
- Audit for siblings.
release_partial was found by noticing underscore-prefixed parameters on a public function. Check every other entry point for the same pattern and report the findings in the PR, even if it turns out to be the only one:
grep -n "pub fn.*(_\|, _[a-z]" contracts/marketx/src/lib.rs
pub fn in this file. One of them was a silent no-op. Establishing that the other 114 are not is part of this issue.
Acceptance criteria
Reproducing what I found
sed -n '1522,1527p' contracts/marketx/src/lib.rs # the whole function
sed -n '259,276p' contracts/marketx/src/test.rs # the only test, flag disabled
sed -n '1711,1740p' contracts/marketx/src/lib.rs # refund_escrow, README says placeholder
grep -n "release_partial" README.md
Then add the probe test above to test.rs and run cargo test --lib temp_probe_release_partial. It passes, which is the bug.
Please leave a comment before starting so the work is not duplicated.
🚨 EVERY CI CHECK MUST PASS. NO EXCEPTIONS. 🚨
Your PR will not be reviewed or merged unless every single job on the CI workflow is green:
Formatting,Clippy,Build Optimized WASM,Tests,Dependency Audit,Coverage, andSDK Error Code Parity. No skipped jobs, no "unrelated failure", no#[ignore]to make a test go away. If a check is red, the work is not finished.Summary
release_partialis a public entry point on a deployed escrow contract that takes an escrow ID and an amount, ignores both, moves no funds, and returnsOk(()).That is the whole function. The leading underscores on
_escrow_idand_amountare the tell — they exist to silence the unused-parameter warning, which is the only reasonClippywith-D warningsstays green on it.There is no
require_auth(). No escrow lookup. No state check. No amount validation. No token transfer. No event. No counter update. Anyone can call it, with any arguments, and receive a successful transaction.Why this is worse than an unimplemented function
An unimplemented function that panics or returns an error is safe: the caller learns nothing happened. This one reports success.
A transaction calling
release_partiallands on-chain, succeeds, and appears in the caller's transaction history as a completed contract invocation. An SDK, a frontend, an integrator's backend, or a retry-on-failure job all read that as the partial release went through. Nothing did. And because no event is emitted, there is not even a negative signal an indexer could use to detect the discrepancy — the escrow simply still holds the full amount while the caller believes part of it was released.I confirmed the behaviour by wiring a probe test into the existing suite:
A non-existent escrow and a negative amount both return success. Every one of those three conditions should be fatal:
999_999does not existEscrowNotFound(10)Ok(())-5InvalidEscrowAmount(13)Ok(())Unauthorized(2)Ok(())Note also that
release_partialnever callsrequire_auth()at all, so it does not even establish who is asking. Comparerelease_itemimmediately below it, which is the properly-implemented partial-release path, andrefund_escrow, which doesinitiator.require_auth()and then checksinitiator != escrow.buyer.The probe was reverted;
mainis unmodified and still reports 128 passing tests.The existing test actively hides this
release_partialis referenced by the test suite — exactly once, intest.rs:270:The one test that touches this function exercises it with the feature flag turned off, and asserts only that the flag check fires. The enabled path — the no-op — has zero coverage.
This is the most dangerous shape a test can have. A reviewer grepping for
release_partialfinds a passing test and moves on. Coverage tooling counts the function as exercised. The function is "tested" in every mechanical sense, and the actual behaviour under the only configuration that matters was never asserted.The one warning that exists is buried and stale
README.md:369is the sole place a reader is told not to trust this function:Two problems.
First, it is in the wrong place. It sits in a prose section called "Current Implementation Notes" near the bottom of the README, nowhere near the API documentation, and — critically — nowhere near the contract. A caller integrating against the deployed WASM has the contract spec, not the README. On-chain, the disclaimer does not exist.
Second, it is factually wrong about
refund_escrow. That function is fully implemented (lib.rs:1711):Auth, ownership, state, and amount validation, then real persisted state. Whatever this note described, it has not been true of
refund_escrowfor some time. So the single document a contributor might rely on is wrong in one direction (understatingrefund_escrow) while being the only thing standing between an integrator and a silent no-op in the other. It cannot be trusted either way.Decide the contract's actual intent
This needs a deliberate decision, not a patch. Pick one and say which in the PR:
Option A — implement it.
release_partial(escrow_id, amount)releases an arbitrary amount rather than a discrete item. It must:require_auth()the buyer, and reject any other caller withUnauthorizedEscrowNotFoundwhen absentamount <= 0andamount > remainingwithInvalidEscrowAmountrelease_escrowandrelease_item— reuse the existing fee helper, do not reimplement the arithmeticReleasedonce nothing remainsFundsReleasedEventand updateTotalReleasedAmount/TotalReleasedCountrelease_itemon an itemised escrow — an escrow must not be drainable through both paths. State explicitly in the PR what happens when both are used on the same escrow, and test it.Option B — make it fail loudly. If item-based release via
release_itemis the intended design andrelease_partialis redundant, it must stop returning success. Either remove the entry point outright, or have it return a clear error. A newNotImplementedvariant inerrors.rsis acceptable, but note it must be mirrored insdk/error-codes.tsor theSDK Error Code Parityjob will fail — that is the job doing its work, not an obstacle to route around.Option A is preferred if a free-amount partial release is genuinely wanted, since
release_itemonly covers itemised escrows. Either way, the current state — a function that succeeds without acting — is not an option.Also in scope
refund_escrowclaim, and move any remaining caveat next to the function's own doc comment where a reader of the code will see it.release_partialcurrently has no doc comment at all, whilerelease_itemdirectly beneath it has a full one with# Argumentsand# Errorssections. Match that standard.release_partialwas found by noticing underscore-prefixed parameters on a public function. Check every other entry point for the same pattern and report the findings in the PR, even if it turns out to be the only one:grep -n "pub fn.*(_\|, _[a-z]" contracts/marketx/src/lib.rspub fnin this file. One of them was a silent no-op. Establishing that the other 114 are not is part of this issue.Acceptance criteria
release_partialcan no longer returnOk(())without having released funds — either it works, or it errorsEscrowNotFound;amount <= 0→InvalidEscrowAmount;amountgreater than remaining →InvalidEscrowAmount; a non-buyer caller →Unauthorizeddisabled_feature_flags_block_pathstest still passes unchanged — the flag check must be preserved, not replacedrelease_escrowandrelease_itemand reuses the same helper; a test asserts the fee is identical across all three paths for the same amountrelease_itemon the same escrow is defined and tested, and an escrow cannot be over-released through the two paths combinedFundsReleasedEventand the release counters behave the same as the other release pathsrelease_partialhas a doc comment matchingrelease_item's standard, including# Errorsrefund_escrowplaceholder claim is correctedpub fnare audited for unused parameters, with the result reported in the PRerrors.rsandsdk/error-codes.tsReproducing what I found
Then add the probe test above to
test.rsand runcargo test --lib temp_probe_release_partial. It passes, which is the bug.Please leave a comment before starting so the work is not duplicated.