You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
open_settlement called with a positive amount on an asset that never received liquidity returns InsufficientLiquidity, while pool() returns PoolNotFound for that same asset. Both are correct, and the divergence is a consequence of storage::get_pool materializing Pool::empty(asset) for a missing entry while pool() checks entry existence directly. Nothing pinned that down, and the existing open_settlement error tests (test_open_settlement_rejects_insufficient_liquidity, _rejects_unregistered, _rejects_non_positive_amount) all probe a funded asset, so the never-funded state was uncovered.
This adds four tests plus a doc comment section.
Tests
test_open_settlement_never_funded_asset_returns_insufficient_liquidity — the main assertion, at amount = 1.
test_pool_getter_never_funded_asset_returns_pool_not_found — the companion the acceptance criteria asks for: the same asset, the other entrypoint, the other error. A refactor collapsing both onto one variant breaks one of these two.
test_open_settlement_never_funded_zero_amount_returns_invalid_amount — boundary above the liquidity check.
test_open_settlement_never_funded_unregistered_anchor_reports_registration — boundary below it.
Why amount = 1, and why four tests instead of one
open_settlement validates in this order: amount <= 0 → InvalidAmount, unregistered anchor → AnchorNotRegistered, amount above cap → AboveMaxSettlementAmount, and only then reads the pool. Two of those precede the liquidity check, so a probe that tripped either would report the wrong error and the test would be green for a reason unrelated to what it claims to cover. amount = 1 on a registered anchor is the minimum that actually reaches pool.total < amount.
Tests 3 and 4 bracket that branch from both sides: they assert the two preceding checks still fire first on the same never-funded asset. If a future refactor reordered validation, the main test would start passing for a different reason and one of these two would catch it.
Doc comment
Added an "Error surface vs pool" section to open_settlement explaining the divergence and noting that an InsufficientLiquidity result may mean never-funded rather than under-funded, so integrators shouldn't assume the two entrypoints agree. This covers the "distinction is documented in code comments" criterion. docs/ERRORS.md doesn't exist in this tree; per the issue's execution note, capturing the distinction there belongs to whichever issue tracks that document.
Scope: the doc comment is the only change to src/lib.rs — 25 added lines, all ///, no logic touched.
Note for maintainers: CI on main is currently red
Unrelated to this PR, but it will make this PR's checks fail, so flagging it. At 140d284, main fails four ways:
list_settlements_by_anchor_and_asset is 36 characters; Soroban caps exported contract function names at 32, so cargo build fails with contract function name is too long: 36, max is 32 at src/lib.rs:1061. The fix is a rename, which breaks the public interface, so that's a maintainer call — list_settlements_by_anchor_asset is exactly 32.
cargo fmt --all -- --check reports 11 hunks across src/lib.rs and src/test.rs that predate this branch.
test_clear_operator fails at src/test.rs:2435 — "pause reached contract logic instead of rejecting operator authorization".
I verified this PR by patching 1 and 2 locally, running, then reverting: the four new tests pass and the suite is 240 passed / 1 failed, the single failure being test_clear_operator above. I deliberately did not run cargo fmt --all, since it would have reformatted the 11 unrelated hunks and mixed other contributors' code into this diff. Happy to send fixes for any of these four as a separate PR if that's useful.
The failing check is pre-existing on main, not from this PR — see the "Note for maintainers" section in the description.
It fails after 26s, before the crate is even compiled: cargo fmt --all -- --check trips on 11 hunks in src/lib.rs and src/test.rs that predate this branch. Past that, cargo build fails on list_settlements_by_anchor_and_asset exceeding Soroban's 32-char limit for exported contract function names, and cargo test doesn't compile at all because Cargo.toml lost features = ["testutils"] from its dev-dependencies.
This branch adds 25 lines of doc comment to src/lib.rs and 97 lines of tests to src/test.rs — no logic changes. I verified it by patching those two blockers locally and reverting: the four new tests pass and the suite is 240 passed / 1 failed, the failure being the pre-existing test_clear_operator at src/test.rs:2435.
I can send a separate PR restoring a green build if that's useful. Two of the fixes are mechanical (the testutils feature is one line, cargo fmt --all handles the formatting), but the third is a rename of a public contract function, so that one's your call — list_settlements_by_anchor_asset fits in exactly 32 characters.
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
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.
Closes #152.
What
open_settlementcalled with a positive amount on an asset that never received liquidity returnsInsufficientLiquidity, whilepool()returnsPoolNotFoundfor that same asset. Both are correct, and the divergence is a consequence ofstorage::get_poolmaterializingPool::empty(asset)for a missing entry whilepool()checks entry existence directly. Nothing pinned that down, and the existingopen_settlementerror tests (test_open_settlement_rejects_insufficient_liquidity,_rejects_unregistered,_rejects_non_positive_amount) all probe a funded asset, so the never-funded state was uncovered.This adds four tests plus a doc comment section.
Tests
test_open_settlement_never_funded_asset_returns_insufficient_liquidity— the main assertion, atamount = 1.test_pool_getter_never_funded_asset_returns_pool_not_found— the companion the acceptance criteria asks for: the same asset, the other entrypoint, the other error. A refactor collapsing both onto one variant breaks one of these two.test_open_settlement_never_funded_zero_amount_returns_invalid_amount— boundary above the liquidity check.test_open_settlement_never_funded_unregistered_anchor_reports_registration— boundary below it.Why amount = 1, and why four tests instead of one
open_settlementvalidates in this order:amount <= 0→InvalidAmount, unregistered anchor →AnchorNotRegistered, amount above cap →AboveMaxSettlementAmount, and only then reads the pool. Two of those precede the liquidity check, so a probe that tripped either would report the wrong error and the test would be green for a reason unrelated to what it claims to cover.amount = 1on a registered anchor is the minimum that actually reachespool.total < amount.Tests 3 and 4 bracket that branch from both sides: they assert the two preceding checks still fire first on the same never-funded asset. If a future refactor reordered validation, the main test would start passing for a different reason and one of these two would catch it.
Doc comment
Added an "Error surface vs
pool" section toopen_settlementexplaining the divergence and noting that anInsufficientLiquidityresult may mean never-funded rather than under-funded, so integrators shouldn't assume the two entrypoints agree. This covers the "distinction is documented in code comments" criterion.docs/ERRORS.mddoesn't exist in this tree; per the issue's execution note, capturing the distinction there belongs to whichever issue tracks that document.Scope: the doc comment is the only change to
src/lib.rs— 25 added lines, all///, no logic touched.Note for maintainers: CI on
mainis currently redUnrelated to this PR, but it will make this PR's checks fail, so flagging it. At
140d284,mainfails four ways:Cargo.tomldev-dependencies lostfeatures = ["testutils"](in Add asset_onboarded event and emit it on first liquidity provision #202), socargo testdoesn't compile at all — 322 errors,mock_all_auths/Address::generate/register_contractall unresolved. One-line fix.list_settlements_by_anchor_and_assetis 36 characters; Soroban caps exported contract function names at 32, socargo buildfails withcontract function name is too long: 36, max is 32atsrc/lib.rs:1061. The fix is a rename, which breaks the public interface, so that's a maintainer call —list_settlements_by_anchor_assetis exactly 32.cargo fmt --all -- --checkreports 11 hunks acrosssrc/lib.rsandsrc/test.rsthat predate this branch.test_clear_operatorfails atsrc/test.rs:2435— "pause reached contract logic instead of rejecting operator authorization".I verified this PR by patching 1 and 2 locally, running, then reverting: the four new tests pass and the suite is 240 passed / 1 failed, the single failure being
test_clear_operatorabove. I deliberately did not runcargo fmt --all, since it would have reformatted the 11 unrelated hunks and mixed other contributors' code into this diff. Happy to send fixes for any of these four as a separate PR if that's useful.