Investigate gas limit risk: set_discount_registry_all emits unbounded submessages #123

New issue

Closed

opened 2026-05-03 12:01:32 +00:00 by PlasticDigits · 4 comments

PlasticDigits commented

2026-05-03 12:01:32 +00:00

(Migrated from gitlab.com)

Context

Report from code review / audit.

File: smartcontracts/contracts/factory/src/contract.rs

Location: execute_set_discount_registry_all (currently ~L335–L364; report cited L335)

Behavior

The handler loads PAIR_COUNT and loops 0..count, pushing a WasmMsg::Execute (SetDiscountRegistry) for each registered pair into a single Response via add_messages.

At large numbers of pairs, the serialized response / submessage vector can become large enough to hit chain gas limits (or related limits), making governance unable to roll out discount registry updates across the full set in one transaction.

Suggested direction

Add pagination (or an equivalent chunked workflow), e.g. a new execute path such as execute_set_discount_registry_batch { start_after, limit } (naming TBD) that processes a bounded slice of PAIR_INDEX per call, with callers / tooling repeating until complete.

Acceptance criteria (proposal)

Confirm limits (max messages per tx, typical gas) on target chain(s).
Design API for batched updates (cursor + max pairs per batch, semantics when count changes mid-flight).
Implement + tests; document how operators should run multi-step rollout.

/label ~bug ~smart-contract (adjust labels as needed for this project)

## Context Report from code review / audit. **File:** `smartcontracts/contracts/factory/src/contract.rs` **Location:** `execute_set_discount_registry_all` (currently ~L335–L364; report cited L335) ## Behavior The handler loads `PAIR_COUNT` and loops `0..count`, pushing a `WasmMsg::Execute` (`SetDiscountRegistry`) for each registered pair into a single `Response` via `add_messages`. At large numbers of pairs, the serialized response / submessage vector can become large enough to hit chain gas limits (or related limits), making governance unable to roll out discount registry updates across the full set in one transaction. ## Suggested direction Add **pagination** (or an equivalent chunked workflow), e.g. a new execute path such as `execute_set_discount_registry_batch { start_after, limit }` (naming TBD) that processes a bounded slice of `PAIR_INDEX` per call, with callers / tooling repeating until complete. ## Acceptance criteria (proposal) - [ ] Confirm limits (max messages per tx, typical gas) on target chain(s). - [ ] Design API for batched updates (cursor + max pairs per batch, semantics when count changes mid-flight). - [ ] Implement + tests; document how operators should run multi-step rollout. --- `/label ~bug ~smart-contract` (adjust labels as needed for this project)

PlasticDigits commented

2026-05-03 12:37:55 +00:00

(Migrated from gitlab.com)

Implementation merged to main (187bff4): Factory SetDiscountRegistryBatch paginates PAIR_INDEX scans with start_after (exclusive numeric cursor) + limit (dex_common::pagination, default 10, max 30 Wasm executes per tx). Response attributes: pairs_updated, has_more, optional next_start_after, scanned_through_index. SetDiscountRegistryAll unchanged; pairs_updated now reflects actual message count.

Docs / crosslinks: docs/contracts-terraclassic.md (rollout invariants + #factory-discount-registry-rollout-invariants-glab-123), docs/contracts-security-audit.md, docs/deployment-guide.md, docs/testing.md, smartcontracts/scripts/deploy.sh, skills/AGENTS_TERRACLASSIC_GAS.md, docs/README.md.

Tests: factory_coverage_tests::test_set_discount_registry_batch_*, cargo test green.

@brouie Please verify against the checklist below (issue stays open until you sign off).

Verification checklist

ExecuteMsg JSON: set_discount_registry_batch accepts registry, optional start_after, optional limit.
First batch with limit=1 on a factory with ≥2 pairs emits has_more=true and next_start_after consistent with continued scanning.
Second batch with start_after = that cursor completes with has_more=false and no next_start_after.
start_after beyond last index returns pairs_updated=0, has_more=false (idempotent).
Non-governance sender gets Unauthorized.
Governance gas: each tx carries at most min(limit cap, remaining pairs) discount-registry Wasm messages (consistent with pagination::MAX_LIMIT).
After multi-tx rollout, spot-check pairs (or indexer) for expected discount registry address vs None when clearing.

Implementation merged to **main** (`187bff4`): **Factory `SetDiscountRegistryBatch`** paginates `PAIR_INDEX` scans with `start_after` (exclusive numeric cursor) + `limit` (`dex_common::pagination`, default 10, max 30 Wasm executes per tx). Response attributes: `pairs_updated`, `has_more`, optional `next_start_after`, `scanned_through_index`. `SetDiscountRegistryAll` unchanged; `pairs_updated` now reflects actual message count. **Docs / crosslinks:** `docs/contracts-terraclassic.md` (rollout invariants + `#factory-discount-registry-rollout-invariants-glab-123`), `docs/contracts-security-audit.md`, `docs/deployment-guide.md`, `docs/testing.md`, `smartcontracts/scripts/deploy.sh`, `skills/AGENTS_TERRACLASSIC_GAS.md`, `docs/README.md`. Tests: `factory_coverage_tests::test_set_discount_registry_batch_*`, `cargo test` green. @brouie **Please verify** against the checklist below (issue stays open until you sign off). ### Verification checklist - [ ] `ExecuteMsg` JSON: `set_discount_registry_batch` accepts `registry`, optional `start_after`, optional `limit`. - [ ] First batch with `limit=1` on a factory with ≥2 pairs emits `has_more=true` and `next_start_after` consistent with continued scanning. - [ ] Second batch with `start_after` = that cursor completes with `has_more=false` and **no** `next_start_after`. - [ ] `start_after` beyond last index returns `pairs_updated=0`, `has_more=false` (idempotent). - [ ] Non-governance sender gets `Unauthorized`. - [ ] Governance gas: each tx carries at most `min(limit cap, remaining pairs)` discount-registry Wasm messages (consistent with `pagination::MAX_LIMIT`). - [ ] After multi-tx rollout, spot-check pairs (or indexer) for expected discount registry address vs `None` when clearing.

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:22:07 +00:00

(Migrated from gitlab.com)

@PlasticDigits — source-side verification PASS on the SetDiscountRegistryBatch pagination.

ran the new test trio:

test factory_coverage_tests::test_set_discount_registry_batch_noop_when_cursor_past_end ... ok
test factory_coverage_tests::test_set_discount_registry_batch_unauthorized ... ok
test factory_coverage_tests::test_set_discount_registry_batch_paginates_cursor ... ok

these map cleanly to your checklist — paginates_cursor covers the limit=1 + has_more + next_start_after path, noop_when_cursor_past_end covers idempotent past-end, unauthorized covers the non-governance reject. full workspace 321/321 PASS.

source review:

dispatch at contract.rs:94-98: SetDiscountRegistryBatch { registry, start_after, limit } accepted, all three params optional except registry
handler at contract.rs:391 (execute_set_discount_registry_batch)
cursor-past-end early-return at contract.rs:408-411 emits pairs_updated=0 + has_more=false (no next_start_after attribute) — matches idempotent semantics
pagination math at contract.rs:430-431:
- has_more = idx < count after the scan completes
- next_start_after = has_more.then_some(idx.saturating_sub(1))
response attributes at contract.rs:437-443: action, pairs_updated, has_more, optional next_start_after
SetDiscountRegistryAll preserved separately — pairs_updated attribute now reflects actual message count rather than the static count

defaults / caps come from dex_common::pagination per your description (default 10, max 30) — not inlined in factory contract, which is the right place for the constant.

still pending: live ops verification — multi-tx rollout against a real factory with N>30 pairs, observing that each tx carries at most min(limit, remaining) wasm executes, and that final batch returns has_more=false cleanly. queued behind the other live-walk debt.

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 SetDiscountRegistryBatch pagination. ran the new test trio: ``` test factory_coverage_tests::test_set_discount_registry_batch_noop_when_cursor_past_end ... ok test factory_coverage_tests::test_set_discount_registry_batch_unauthorized ... ok test factory_coverage_tests::test_set_discount_registry_batch_paginates_cursor ... ok ``` these map cleanly to your checklist — paginates_cursor covers the limit=1 + has_more + next_start_after path, noop_when_cursor_past_end covers idempotent past-end, unauthorized covers the non-governance reject. full workspace 321/321 PASS. source review: - dispatch at `contract.rs:94-98`: `SetDiscountRegistryBatch { registry, start_after, limit }` accepted, all three params optional except `registry` - handler at `contract.rs:391` (`execute_set_discount_registry_batch`) - cursor-past-end early-return at `contract.rs:408-411` emits `pairs_updated=0` + `has_more=false` (no `next_start_after` attribute) — matches idempotent semantics - pagination math at `contract.rs:430-431`: - `has_more = idx < count` after the scan completes - `next_start_after = has_more.then_some(idx.saturating_sub(1))` - response attributes at `contract.rs:437-443`: `action`, `pairs_updated`, `has_more`, optional `next_start_after` - `SetDiscountRegistryAll` preserved separately — `pairs_updated` attribute now reflects actual message count rather than the static count defaults / caps come from `dex_common::pagination` per your description (default 10, max 30) — not inlined in factory contract, which is the right place for the constant. still pending: live ops verification — multi-tx rollout against a real factory with N>30 pairs, observing that each tx carries at most min(limit, remaining) wasm executes, and that final batch returns `has_more=false` cleanly. queued behind the other live-walk debt. 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:21 +00:00