Skip to content

feat: add CampaignApplicants index and campaign_applicants view function - #53

Open
EmeditWeb wants to merge 1 commit into
Ads-Bazaar:mainfrom
EmeditWeb:feat/issue-8-campaign-applicants-index
Open

feat: add CampaignApplicants index and campaign_applicants view function#53
EmeditWeb wants to merge 1 commit into
Ads-Bazaar:mainfrom
EmeditWeb:feat/issue-8-campaign-applicants-index

Conversation

@EmeditWeb

Copy link
Copy Markdown

Summary

Replaces the O(1) ApplicantCount counter with a CampaignApplicants Vec<Address> index per campaign, and adds a new campaign_applicants(campaign_id) view function so clients can list all applicants without relying on event logs alone.

Changes

storage.rs

  • Replace DataKey::ApplicantCount(CampaignId) with DataKey::CampaignApplicants(CampaignId) storing Vec<Address>
  • Update add_campaign_applicant to push the creator onto the ordered list
  • Update has_campaign_applicants to check for non-empty Vec
  • Add get_campaign_applicants with TTL bump on read

lib.rs

  • Add campaign_applicants(env, campaign_id) -> Vec<Address> view function
  • Import Vec from soroban_sdk

test.rs

  • Remove O(1) write-cost regression test (no longer applies with Vec-based storage)
  • Add campaign_applicants_returns_all_applicants_in_order: 3 creators apply → returned in order
  • Add campaign_applicants_empty_when_no_applicants: returns empty Vec
  • Add double_apply_via_index: second apply returns AlreadyApplied and index has only 1 entry
  • Add campaign_applicants_lock_metadata: brief locks once applicant exists

Testing

All 106 workspace tests pass (89 campaign-escrow + 17 dispute-resolution).

Close #8

Replace O(1) ApplicantCount counter with CampaignApplicants Vec<Address>
to support listing all applicants for a campaign.

- Add DataKey::CampaignApplicants(u64) storing Vec<Address>
- Update add_campaign_applicant to push_back creator to the ordered list
- Add campaign_applicants(campaign_id) view function returning Vec<Address>
- Add TTL bump on both read and write for CampaignApplicants key
- Guard against double-application via existing get_application check
- Update has_campaign_applicants to check non-empty Vec
- Add tests: 3 creators returned in order, empty list, double-apply guard

Close Ads-Bazaar#8

@JamesVictor-O JamesVictor-O left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up #8 — a campaign_applicants view function is a real gap, and the enumeration approach (mirroring SelectedCreators) is the right shape for it.

Requesting changes on the storage design, though: this PR replaces DataKey::ApplicantCount with DataKey::CampaignApplicants(Vec<Address>), which reverts #50 — merged three PRs ago specifically to fix #43 (add_campaign_applicant rewriting an ever-growing Vec<Address> on every single apply_to_campaign call). That issue explicitly laid out two acceptable resolutions:

  1. Replace the Vec with a counter (what #50 did)
  2. Or, if the ordered list is intentionally useful for something else (e.g. #8) — keep the Vec, but add a hard cap on total applications per campaign so growth is at least bounded

This PR takes the Vec back but doesn't add the cap from option 2, so it silently reintroduces #43's exact problem: every apply_to_campaign on a popular campaign gets progressively more expensive to write, unbounded. has_campaign_applicants also goes back to deserializing the whole list just to check non-emptiness, on every update_campaign_metadata lock check.

Could you add a cap — something like MAX_APPLICANTS_PER_CAMPAIGN, checked in add_campaign_applicant/apply_to_campaign, returning a dedicated error once exceeded (mirroring the existing max_creators / MaxCreatorsReached pattern for approved creators)? That satisfies both #8 and #43 as issue #43 itself anticipated, with a regression test for the cap alongside the enumeration tests you've already written.

Everything else here — the campaign_applicants shape, TTL-bump-on-read, and the new test coverage for ordering/empty-list — looks solid. This is the one thing blocking merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add application index per campaign to support listing all applicants for a campaign

2 participants