code/cl8y-dex-terraclassic

Fork 0

Gas: Pair registry lookup is O(n); exhausts CosmWasm gas at 1000+ pairs #122

New issue

Closed

opened 2026-05-03 12:00:46 +00:00 by PlasticDigits · 8 comments

PlasticDigits commented

2026-05-03 12:00:46 +00:00

(Migrated from gitlab.com)

Summary

assert_pair_in_registry in the factory contract performs a linear scan over all indexed pairs (PAIR_COUNT + PAIR_INDEX). At 1000+ pairs this exhausts CosmWasm gas limits, blocking governance flows that validate pair membership.

Location

File: smartcontracts/contracts/factory/src/contract.rs (approximately line 229)

Reported behavior

fn assert_pair_in_registry(deps: &DepsMut, pair_addr: &Addr) -> Result<(), ContractError> {
    let count = PAIR_COUNT.load(deps.storage)?;
    for idx in 0..count {
        if let Ok(info) = PAIR_INDEX.load(deps.storage, idx) {
            if info.contract_addr == *pair_addr {
                return Ok(());
            }
        }
    }
    Err(ContractError::PairNotInRegistry { ... })
}

Impact

Used by (among others): SetPairFee, SetPairHooks, SweepPair, SetPairPaused, and similar entry points. Linear cost per check makes these operations impractical at scale.

Suggested direction

Add a reverse index (e.g. Map<Addr, bool> or Map<Addr, u64>) maintained on pair register/update/remove so membership lookup is O(1) and gas stays bounded.

Acceptance criteria (draft)

Confirm gas profile / reproduce failure mode at large PAIR_COUNT (or document expected limits).
Design storage layout for reverse index (migration if needed for existing factories).
Implement O(1) lookup in assert_pair_in_registry (and any related paths).
Tests covering register, edge cases, and governance messages with many pairs (or mocked storage patterns).

Opened for investigation from an external report; details and line numbers should be verified against current main.

## Summary `assert_pair_in_registry` in the factory contract performs a linear scan over all indexed pairs (`PAIR_COUNT` + `PAIR_INDEX`). At 1000+ pairs this exhausts CosmWasm gas limits, blocking governance flows that validate pair membership. ## Location **File:** `smartcontracts/contracts/factory/src/contract.rs` (approximately line 229) ## Reported behavior ```rust fn assert_pair_in_registry(deps: &DepsMut, pair_addr: &Addr) -> Result<(), ContractError> { let count = PAIR_COUNT.load(deps.storage)?; for idx in 0..count { if let Ok(info) = PAIR_INDEX.load(deps.storage, idx) { if info.contract_addr == *pair_addr { return Ok(()); } } } Err(ContractError::PairNotInRegistry { ... }) } ``` ## Impact Used by (among others): **SetPairFee**, **SetPairHooks**, **SweepPair**, **SetPairPaused**, and similar entry points. Linear cost per check makes these operations impractical at scale. ## Suggested direction Add a **reverse index** (e.g. `Map<Addr, bool>` or `Map<Addr, u64>`) maintained on pair register/update/remove so membership lookup is **O(1)** and gas stays bounded. ## Acceptance criteria (draft) - [ ] Confirm gas profile / reproduce failure mode at large `PAIR_COUNT` (or document expected limits). - [ ] Design storage layout for reverse index (migration if needed for existing factories). - [ ] Implement O(1) lookup in `assert_pair_in_registry` (and any related paths). - [ ] Tests covering register, edge cases, and governance messages with many pairs (or mocked storage patterns). --- *Opened for investigation from an external report; details and line numbers should be verified against current `main`.*

PlasticDigits commented

2026-05-03 12:28:16 +00:00

(Migrated from gitlab.com)

mentioned in commit bf8e55a6a7

mentioned in commit bf8e55a6a76a588e256f806dc2edb43da226a2bf

PlasticDigits commented

2026-05-03 12:34:42 +00:00

(Migrated from gitlab.com)

mentioned in commit 7084a9776a

mentioned in commit 7084a9776a33fb65b3968ddb89f4fc1d96b91a15

PlasticDigits commented

2026-05-03 12:34:57 +00:00

(Migrated from gitlab.com)

Implemented (GitLab #122)

Summary: Factory assert_pair_in_registry now uses storage map pair_addr_reg (PAIR_ADDR_REGISTERED) for O(1) membership checks instead of scanning PAIR_INDEX.

Code: smartcontracts/contracts/factory/src/state.rs, contract.rs (cw2 version 1.1.0, migrate backfills reverse map from PAIR_INDEX). Reply handler registers each new pair addr; migrate path for legacy 1.0.0 factories.

Docs: docs/contracts-terraclassic.md § Factory storage & upgrades, skills/AGENTS_TERRACLASSIC_GAS.md, docs/runbooks/wasm-admin-migration.md.

Tests: Unit migrate backfill; integration test_factory_many_pairs_governance_fee_update_uses_registry_lookup (35 pairs + SetPairFee on last); full cargo test -p cl8y-dex-tests green on main (follow-up commit fixes per-block CreatePair in that test).

@brouie please verify on your side:

Checklist

Chain / ops: Existing factory wasm 1.0.0 → store 1.1.0 wasm → migrate once; confirm governance ops (SetPairFee, SweepPair, etc.) succeed on a pair deep in the registry without gas exhaustion.
Invariant: After migrate + any new CreatePair, every pair_index[i].contract_addr has pair_addr_reg set (spot-check raw keys or behaviour).
Regression: Random non-registry pair address still rejected with PairNotInRegistry.
Pagination: Pairs query pagination / indexer discovery still behaves as before (linear scan there is intentional).
Broadcast-all: SetDiscountRegistryAll / governance fan-out still intentionally iterate all pairs (gas scales with pair count).

Leaving issue open per request.

## Implemented (GitLab #122) **Summary:** Factory `assert_pair_in_registry` now uses storage map `pair_addr_reg` (`PAIR_ADDR_REGISTERED`) for **O(1)** membership checks instead of scanning `PAIR_INDEX`. **Code:** `smartcontracts/contracts/factory/src/state.rs`, `contract.rs` (cw2 version **1.1.0**, migrate backfills reverse map from `PAIR_INDEX`). Reply handler registers each new pair addr; migrate path for legacy **1.0.0** factories. **Docs:** [`docs/contracts-terraclassic.md` § Factory storage & upgrades](https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/blob/main/docs/contracts-terraclassic.md), [`skills/AGENTS_TERRACLASSIC_GAS.md`](https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/blob/main/skills/AGENTS_TERRACLASSIC_GAS.md), [`docs/runbooks/wasm-admin-migration.md`](https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/blob/main/docs/runbooks/wasm-admin-migration.md). **Tests:** Unit migrate backfill; integration `test_factory_many_pairs_governance_fee_update_uses_registry_lookup` (35 pairs + `SetPairFee` on last); full `cargo test -p cl8y-dex-tests` green on `main` (follow-up commit fixes per-block `CreatePair` in that test). --- @brouie please verify on your side: **Checklist** - [ ] **Chain / ops:** Existing factory wasm **1.0.0** → store **1.1.0** wasm → `migrate` once; confirm governance ops (`SetPairFee`, `SweepPair`, etc.) succeed on a pair deep in the registry without gas exhaustion. - [ ] **Invariant:** After migrate + any new `CreatePair`, every `pair_index[i].contract_addr` has `pair_addr_reg` set (spot-check raw keys or behaviour). - [ ] **Regression:** Random non-registry pair address still rejected with `PairNotInRegistry`. - [ ] **Pagination:** `Pairs` query pagination / indexer discovery still behaves as before (linear scan there is intentional). - [ ] **Broadcast-all:** `SetDiscountRegistryAll` / governance fan-out still intentionally iterate all pairs (gas scales with pair count). Leaving issue **open** per request.

PlasticDigits commented

2026-05-03 12:39:42 +00:00

(Migrated from gitlab.com)

mentioned in commit d283a695c2

mentioned in commit d283a695c27d7acc7579ab34af9cce7c3eccf7a4

Brouie commented

2026-05-05 23:55:34 +00:00

(Migrated from gitlab.com)

mentioned in issue #133

Brouie commented

2026-05-06 04:20:22 +00:00

(Migrated from gitlab.com)

mentioned in issue #121

Brouie commented

2026-05-06 04:21:26 +00:00

(Migrated from gitlab.com)

@PlasticDigits — source-side verification PASS on the O(1) registry lookup.

ran your named test:

test factory_tests::test_factory_many_pairs_governance_fee_update_uses_registry_lookup ... ok

full workspace 321/321 PASS, 0 failures.

source review:

PAIR_ADDR_REGISTERED: Map<Addr, bool> declared in state.rs:37 (key pair_addr_reg matches your description)
assert_pair_in_registry rewritten to PAIR_ADDR_REGISTERED.has(deps.storage, pair_addr.clone()) at contract.rs:258 — pure O(1) map lookup, no PAIR_INDEX scan
reply handler saves to the reverse map on every new CreatePair at contract.rs:702
migrate path backfills the reverse map from PAIR_INDEX at contract.rs:765 for legacy 1.0.0 factories
CONTRACT_VERSION = "1.1.0" at contract.rs:22, migrate uses cw2::ensure_from_older_version at contract.rs:759 — proper cw2 versioning
migrate-path unit test at contract.rs:777-814 asserts pairs are present in PAIR_ADDR_REGISTERED after backfill

cross-checked all five governance entry points use the same assert_pair_in_registry:

contract.rs:281 — assert_pair_in_registry(&deps, &pair_addr)?;
contract.rs:307 — assert_pair_in_registry(&deps, &pair_addr)?;
contract.rs:333 — assert_pair_in_registry(&deps, &pair_addr)?;
contract.rs:460 — assert_pair_in_registry(&deps, &pair_addr)?;
contract.rs:487 — assert_pair_in_registry(&deps, &pair_addr)?;

so SetPairFee, SetPairHooks, SweepPair, SetPairPaused etc. all benefit from the O(1) lookup. matches the audit fix scope.

still pending: live ops verification — store 1.1.0 wasm → migrate once on a 1.0.0 factory → confirm governance ops on a deep-index pair succeed. plus invariant spot-check that every pair_index[i].contract_addr has pair_addr_reg set after a fresh CreatePair post-migrate. queued behind the other live-walk debt.

regression noted: random non-registry address rejected with PairNotInRegistry is preserved by the new path (the .has() returns false, falls through to the existing error).

Pairs query pagination + SetDiscountRegistryAll broadcast-all behavior unchanged — that intentional iteration still scales with pair count, which is the contract for those entry points.

ready for close on your side once you are happy with source PASS, or hold for chain-ops walk.

@PlasticDigits — source-side verification PASS on the O(1) registry lookup. ran your named test: ``` test factory_tests::test_factory_many_pairs_governance_fee_update_uses_registry_lookup ... ok ``` full workspace 321/321 PASS, 0 failures. source review: - `PAIR_ADDR_REGISTERED: Map<Addr, bool>` declared in `state.rs:37` (key `pair_addr_reg` matches your description) - `assert_pair_in_registry` rewritten to `PAIR_ADDR_REGISTERED.has(deps.storage, pair_addr.clone())` at `contract.rs:258` — pure O(1) map lookup, no `PAIR_INDEX` scan - reply handler saves to the reverse map on every new `CreatePair` at `contract.rs:702` - migrate path backfills the reverse map from `PAIR_INDEX` at `contract.rs:765` for legacy 1.0.0 factories - `CONTRACT_VERSION = "1.1.0"` at `contract.rs:22`, migrate uses `cw2::ensure_from_older_version` at `contract.rs:759` — proper cw2 versioning - migrate-path unit test at `contract.rs:777-814` asserts pairs are present in `PAIR_ADDR_REGISTERED` after backfill cross-checked all five governance entry points use the same `assert_pair_in_registry`: ``` contract.rs:281 — assert_pair_in_registry(&deps, &pair_addr)?; contract.rs:307 — assert_pair_in_registry(&deps, &pair_addr)?; contract.rs:333 — assert_pair_in_registry(&deps, &pair_addr)?; contract.rs:460 — assert_pair_in_registry(&deps, &pair_addr)?; contract.rs:487 — assert_pair_in_registry(&deps, &pair_addr)?; ``` so SetPairFee, SetPairHooks, SweepPair, SetPairPaused etc. all benefit from the O(1) lookup. matches the audit fix scope. still pending: live ops verification — store 1.1.0 wasm → migrate once on a 1.0.0 factory → confirm governance ops on a deep-index pair succeed. plus invariant spot-check that every `pair_index[i].contract_addr` has `pair_addr_reg` set after a fresh `CreatePair` post-migrate. queued behind the other live-walk debt. regression noted: random non-registry address rejected with `PairNotInRegistry` is preserved by the new path (the `.has()` returns false, falls through to the existing error). `Pairs` query pagination + `SetDiscountRegistryAll` broadcast-all behavior unchanged — that intentional iteration still scales with pair count, which is the contract for those entry points. ready for close on your side once you are happy with source PASS, or hold for chain-ops walk.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-05-06 08:19:35 +00:00

PlasticDigits commented

2026-05-31 13:45:56 +00:00

(Migrated from gitlab.com)

mentioned in issue #258