Skip to content

fix: emit CampaignExpired event in expire_campaign - #66

Closed
kragent66-glitch wants to merge 2 commits into
Ads-Bazaar:mainfrom
kragent66-glitch:fix/emit-campaign-expired-event
Closed

fix: emit CampaignExpired event in expire_campaign#66
kragent66-glitch wants to merge 2 commits into
Ads-Bazaar:mainfrom
kragent66-glitch:fix/emit-campaign-expired-event

Conversation

@kragent66-glitch

Copy link
Copy Markdown

Problem

expire_campaign emitted CampaignCancelled, misreporting an expired campaign as cancelled.

Approach

  • Add CampaignExpired event struct in events.rs
  • Emit it from expire_campaign (1-line change in lib.rs)
  • Add test_expire_event regression test

Verification

Minimal scoped change following existing event patterns.

Closes #64

Closes Ads-Bazaar#64: expire_campaign emitted CampaignCancelled which misreports
the campaign state. Add CampaignExpired event struct and emit it from
expire_campaign, with a regression test (test_expire_event).

@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.

The core fix is correct — expire_campaign was emitting CampaignCancelled instead of CampaignExpired, and swapping the event struct at the call site is the right change.

However, the added test in test.rs (test_expire_event::test_campaign_expired_event) does not compile:

error[E0433]: cannot find module or crate `hex` in this scope
   --> contracts/campaign-escrow/src/test.rs:1967:35
error[E0599]: no method named `abi_encode` found for struct `soroban_sdk::events::Events`
error[E0599]: no method named `iter` found for struct `ContractEvents`
error[E0599]: no function or associated item named `generate` found for struct `soroban_sdk::Address`

Specifically:

  • hex::encode(...) — the hex crate isn't a dependency of this crate, and isn't needed here.
  • env.events().abi_encode(...) — no such method exists on soroban_sdk::events::Events.
  • env.events().all().iter()ContractEvents (the type returned by .all()) needs a different access pattern; check how existing tests in this file assert on emitted events (e.g. search for other env.events() usage) and follow that pattern instead.
  • Address::generate(&env) needs use soroban_sdk::testutils::Address as _; in scope (see how other test modules in this file import it).

Simplest fix: look at an existing test elsewhere in test.rs that already asserts an event was emitted (e.g. around SurplusReclaimed or DisputeFrozen) and mirror that pattern rather than hand-rolling XDR/hex encoding — the existing pattern is much simpler and will compile.

Please push a fix and I'll re-review. Ran cargo test --workspace locally to confirm the compile failure.

@JamesVictor-O

Copy link
Copy Markdown
Contributor

Correction to my review above: I said to mirror an existing event-assertion test elsewhere in test.rs, but on checking, no existing test in this file actually asserts on emitted events — so there's no example to copy. Concrete guidance instead:

env.events().all() returns a soroban_sdk::testutils::ContractEvents (not a plain iterable), which exposes:

  • .events()&[xdr::ContractEvent] (raw XDR), or
  • PartialEq<Vec<(Address, Vec<Val>, Val)>> — so you can assert equality directly against a vector of (contract_address, topics_vec, data_val) tuples without any hex/XDR encoding at all.

The simplest fix is likely:

assert_eq!(
    env.events().all(),
    vec![
        &env,
        (
            contract_id.clone(),
            (symbol_short!("...") /* whatever topic Campaign
Expired's #[topic] derives */,).into_val(&env),
            (id, refund).into_val(&env), // or however the event's data fields serialize
        ),
    ]
);

(check the #[contractevent] macro output / another Soroban project's tests for the exact topic symbol and data shape CampaignExpired derives — I don't have that mapping handy, but the ContractEvents: PartialEq<Vec<(Address, Vec<Val>, Val)>> impl is the supported comparison path in soroban-sdk 27, not manual XDR/hex encoding).

No hex crate, no abi_encode needed either way.

The previous commit swapped the wrong call site: cancel_campaign was
changed to emit CampaignExpired while expire_campaign (the subject of
Ads-Bazaar#64) kept emitting CampaignCancelled. Restore CampaignCancelled in
cancel_campaign and emit CampaignExpired in expire_campaign.

Also fix the regression test, which did not compile (hex crate not a
dependency, no abi_encode/iter on the events API) and never actually
called expire_campaign. Use Event::to_xdr + filter_by_contract, the
supported soroban-sdk 27 comparison path, and assert the emitted
refunded_amount matches the unallocated balance.
@kragent66-glitch

Copy link
Copy Markdown
Author

Pushed a fix addressing the review — and found a deeper bug in the process.

What was wrong:

  1. The previous commit changed the wrong call sitecancel_campaign was emitting CampaignExpired while expire_campaign (the subject of bug: expire_campaign reuses the CampaignCancelled event instead of a distinct CampaignExpired event #64) kept emitting CampaignCancelled. Both are now correct: cancel_campaignCampaignCancelled, expire_campaignCampaignExpired.
  2. The regression test did not compile (no hex dep, no abi_encode/.iter() on the events API) and never actually called expire_campaign, so it could not have caught the swapped events even if it had compiled.

The rewritten test follows the supported soroban-sdk 27 path you outlined:

  • env.events().all().filter_by_contract(&contract_id) to scope to this contract
  • Event::to_xdr (the documented test-comparison helper) instead of hex/abi_encode
  • Asserts refunded_amount equals the unallocated balance (budget − 2 × 1M committed), not 0

Verified locally: cargo fmt --check ✓, cargo clippy --workspace --all-targets -- -D warnings ✓, cargo test --workspace → 124 passed, 0 failed (88 escrow + 15 dispute + 21 pause). The new test fails against the pre-fix code (it caught campaign_cancelled vs campaign_expired), so it's a genuine regression guard.

@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.

Automated review: confirmed the change is exactly what it claims — expire_campaign now emits CampaignExpired instead of CampaignCancelled, with cancel_campaign's own emit untouched (no duplicate/leftover). New event struct mirrors CampaignCancelled's shape and the file's existing event conventions. Regression test asserts the exact XDR-encoded event (topic + data), not just "doesn't panic". No fund-movement or state-transition logic touched. CI green. Approving.

@JamesVictor-O

Copy link
Copy Markdown
Contributor

⚠️ Merge conflict — this PR was approved, but #72 (checks-effects-interactions reorder) merged first and now conflicts with this branch in exactly one spot.

contracts/campaign-escrow/src/lib.rs, in expire_campaign: #72 moved the token transfer to after storage::set_campaign while keeping the events::CampaignCancelled { ... } call; this PR renames that same call to events::CampaignExpired { ... } at its old position. events.rs and test.rs merge cleanly — only this one hunk conflicts.

Resolution: rebase on latest main, keep #72's write-then-transfer ordering, and just rename the event call within it to events::CampaignExpired { ... }. Since maintainerCanModify is off for this PR, I can't push the rebase myself — could you (or whoever picks this up) rebase and push? Happy to re-review and merge right after.

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.

bug: expire_campaign reuses the CampaignCancelled event instead of a distinct CampaignExpired event

2 participants