fix(factory): uncap stream_addresses to match paginated index page size - #483
Open
maybay-dev wants to merge 1 commit into
Open
fix(factory): uncap stream_addresses to match paginated index page size#483maybay-dev wants to merge 1 commit into
stream_addresses to match paginated index page size#483maybay-dev wants to merge 1 commit into
Conversation
…size
`stream_addresses` (the batch ID→address resolver) was capped at
`MAX_BATCH_SIZE` (10), the same limit used for write-heavy operations
like `create_batch_streams` and `cancel_batch_streams`. Meanwhile,
`streams_by_sender` / `streams_by_recipient` return up to 100 IDs per
page (`MAX_PAGE_SIZE`). Resolving one full page of 100 stream IDs
required 10 separate `stream_addresses` round-trips — contradicting the
API's own docstring ("a page of IDs from either can be resolved to
addresses in one call").
The root cause is that `stream_addresses` inherited the write-path cap
(`MAX_BATCH_SIZE`) even though it only performs `persistent().get()`
lookups per ID — no contract deployment, no token transfers, no
governor cross-contract calls. The cost profile is purely read-bound.
Introduce `MAX_RESOLVE_SIZE = 100` in `query.rs` as a dedicated read-path
cap sized to match `MAX_PAGE_SIZE`, so a full page from either paginated
index can be resolved in a single call. Add a new `ResolveTooLarge` error
discriminant (30) so callers can distinguish a resolve-path rejection
from a write-path `BatchTooLarge`.
Alongside the core fix, this commit resolves several pre-existing build
failures left by recent merges that prevented CI from passing:
- `drip-stream/errors.rs`: Added missing `InvalidRecipient` (19),
`BackdatedStream` (20), and `StreamUnderfunded` (21) variants that
`drip-stream/src/lib.rs` references but `errors.rs` never defined.
- `drip-factory/index.rs`: Fixed `streams_by_sender` return type from
`Vec<u64>` to `StreamPage` and captured the `read_index` result as
`ids` (both `streams_by_sender` and `streams_by_recipient` had the
same bug where the return value was discarded).
- `drip-factory/tests.rs`: Updated test assertions to access
`page.ids.len()` / `page.ids.get()` instead of calling methods
directly on `StreamPage`.
Closes conduit-protocol#418
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@maybay-dev 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! 🚀 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #418
What this fixes
DripFactory::stream_addresses— the batch ID→address resolver meant to pair withstreams_by_sender/streams_by_recipient— was capped atMAX_BATCH_SIZE(10), while the paginated index queries return up to 100 IDs per page (MAX_PAGE_SIZE). Resolving a single full page of 100 stream IDs required 10 separatestream_addressescalls instead of 1, contradicting the API's own docstring which implies one call per page.This commit also fixes several pre-existing build failures left by recent merges that prevented CI from passing (missing error variants in
drip-stream, wrong return types indrip-factory/index.rs, and stale test assertions).Root cause
stream_addressesinherited the write-path cap (MAX_BATCH_SIZE = 10) even though it only performspersistent().get()lookups per ID — no contract deployment, no token transfers, no governor cross-contract calls. The two caps were set from different design constraints:MAX_BATCH_SIZEbounds the write batch cost: eachcreate_streamin the batch performs a governor cross-contract call, twotoken::transfers, a contract deploy +initializeinvoke, and three persistent writes (~2.5M CPU instructions per stream).MAX_PAGE_SIZEbounds a read vector in pagination.stream_addressesis purely read-bound (persistent().get()per ID) but incorrectly inherited the write cap.The fix and why
Core change: Introduce
MAX_RESOLVE_SIZE = 100inquery.rsas a dedicated read-path cap, sized to matchMAX_PAGE_SIZEso a full page from either paginated index can be resolved in a single call.contracts/factory/src/query.rs: Addedpub const MAX_RESOLVE_SIZE: u32 = 100with documentation explaining its relationship toMAX_PAGE_SIZEand why the write-pathMAX_BATCH_SIZEdoes not apply.contracts/factory/src/lib.rs: Updatedstream_addressesto checkids.len() > query::MAX_RESOLVE_SIZEinstead ofMAX_BATCH_SIZE. Updated docstrings to reference the new constant.contracts/factory/src/errors.rs: AddedResolveTooLarge = 30error discriminant so callers can distinguish a resolve-path rejection from a write-pathBatchTooLarge.contracts/factory/src/lib.rs: UpdatedMAX_BATCH_SIZEdoc comment to no longer referencestream_addresses.Pre-existing build fixes (required for CI to pass):
contracts/stream/src/errors.rs: Added missingInvalidRecipient(19),BackdatedStream(20), andStreamUnderfunded(21) variants thatdrip-stream/src/lib.rsreferences buterrors.rsnever defined (broken by recent merges).contracts/factory/src/index.rs: Fixedstreams_by_senderreturn type fromVec<u64>toStreamPageand captured theread_indexresult asids(bothstreams_by_senderandstreams_by_recipientdiscarded the return value and had an unboundidsvariable).contracts/factory/src/tests.rs: Updated test assertions to accesspage.ids.len()/page.ids.get()instead of calling methods directly onStreamPage.How it was tested
cargo clippy -p drip-factory -- -D warnings— passes with zero warnings.cargo test -p drip-factory— all 35 unit tests pass (including legacy migration, TTL refresh, cancel batch, protocol fee, upgrade, and pause tests).cargo test -p drip-stream— all 93 stream unit tests pass (verifying the error variant additions don't break existing behavior).factory_deploy(36),factory_batch_create(7 + 1 ignored),factory_pause(11),factory_ttl(6),governor_config(42),governor_rbac(22),stream_lifecycle(3),stream_pause_resume(9),stream_clawback(8),batch_transfer_processor(14),oracle_stress_test(4),reentrancy_stress_test(22),audit_round_2_regression(9),yield_rebate(15).Follow-up worth filing separately
stream_addresseswith >10 IDs — The current test suite has no integration test exercisingstream_addressesdirectly. A test that seeds 100+ stream IDs and resolves them in a single call would validate the new cap and serve as a regression test.token-vaultbuild failures — Thetoken-vaultcrate has 6 unresolved import errors (set_pending_owner,get_pending_owner, etc.) that are unrelated to this issue but also block full-workspace CI.