feat(vault): structured upgrade lifecycle events#586
Open
Osuolale1 wants to merge 3 commits into
Open
Conversation
Recent "-X theirs" auto-resolutions in PRs CalloraOrg#578/CalloraOrg#579 left contracts/vault/src/lib.rs in a non-compiling state with 107 errors. This commit reconstructs the intended state: - remove duplicate get_max_deduct (was defined at L376 and L1305; kept the documented copy at L376) - remove panic-version broadcast (kept the Result-returning version that matches the surrounding contract style and reuses VaultError::MetadataTooLong) - replace inline VaultError enum with the canonical one in errors.rs via `mod errors; pub use errors::VaultError;` (the inline copy was missing the contracterror macro import and was diverging from errors.rs) - declare `mod validators;` so validators.rs is wired into the crate - fix two StorageKey::Meta typos (variant is MetaKey) - add the missing `&ut` (USDC token) argument to two SettlementClient::receive_payment calls (the settlement trait takes 5 args, the call sites were passing 4) - change `max_fee_bps: u16` -> `u32` in `deduct` (Soroban contract ABI does not support u16; updated the u16::MAX sentinel to u32::MAX accordingly) Verified with `cargo check -p callora-vault --lib`.
The existing set_admin_unauthorized_fails test runs under env.mock_all_auths(), so it only exercises the in-body `caller != current_admin` identity check. Removing caller.require_auth() from set_admin would leave that test green, leaving the contract one edit away from accepting unauthenticated rotations. Add three integration tests that assert the Soroban auth framework itself is the gate: - set_admin_fails_without_authorization: env.set_auths(&[]) + try_set_admin must return Err (proves require_auth, not the identity check, is rejecting) - set_admin_records_caller_auth_entry: under mock_auths for the caller, env.auths() must contain an entry signed by the caller - set_admin_fails_when_only_other_party_authorizes: auth mocked for a different address than the declared caller must still cause the call to fail (proves require_auth checks the caller parameter, not any in-scope signer) Note: the bounty wording asks for "multisig" tests but the vault uses a single-admin model with a two-step pending/accept transfer. The tests assert the strongest property the current contract supports.
Replace the single, loose-tuple `upgraded` event on `CalloraVault::upgrade` with a structured pair so indexers can detect in-progress vs completed upgrades and decode a typed payload. Each successful `upgrade()` now emits three events in order: 1. `upgrade_started` — published BEFORE `update_current_contract_wasm`. Topic `(symbol, caller)`, data `UpgradeEvent`. 2. `upgrade_completed` — published AFTER the WASM swap and the `ContractVersion` storage write. Same topic/payload shape. 3. `upgraded` — legacy single-event shape, retained for backwards compatibility with off-chain subscribers. The `UpgradeEvent` payload carries caller, previous_wasm (Option), new_wasm, ledger, and timestamp. Public ABI is additive only; the legacy `upgraded` topic and shape are unchanged. Tests in contracts/vault/tests/upgrade_events.rs cover state effects, Val round-trip for the payload, and ledger/timestamp fidelity. SDK 22's test harness hides events after WASM swap; this is the same limitation the revenue_pool tests document.
|
@Osuolale1 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
1 participant
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.
CLOSE #528
Summary
CalloraVault::upgrade previously emitted a single loose-tuple upgraded event with the admin as a topic and the new WASM hash as raw bytes data. Off-chain indexers had no structured payload, no way to distinguish an in-progress upgrade from a completed one, and no record of the prior version on the same ledger entry as the new one.
This PR introduces a structured lifecycle: upgrade_started is published before the host swaps the WASM, and upgrade_completed after the swap and version persist. Both carry a typed UpgradeEvent payload. The legacy upgraded event is retained for backwards compatibility.
Changes
contracts/vault/src/lib.rs — new #[contracttype] struct UpgradeEvent { caller, previous_wasm, new_wasm, ledger, timestamp }; rewrote upgrade() to emit the lifecycle pair around the host swap; legacy upgraded emit retained at end.
contracts/vault/src/events.rs — added event_upgrade_started and event_upgrade_completed topic helpers with snapshot byte-identity tests; existing event_upgraded helper documented as legacy.
contracts/vault/tests/upgrade_events.rs (new) — 5 integration tests:
upgrade_completes_with_lifecycle_events_and_persists_version — single upgrade succeeds and ContractVersion is set
second_upgrade_carries_first_hash_as_previous — multi-upgrade exercises the previous_wasm derivation pathupgrade_event_payload_roundtrips_through_val — UpgradeEvent encodes and decodes via Val without field loss
upgrade_event_payload_roundtrips_with_none_previous_wasm — first-upgrade payload (previous_wasm: None) round-trips cleanly
upgrade_event_pins_ledger_and_timestamp_from_env — payload reflects env.ledger().sequence()/timestamp() at emit time
EVENT_SCHEMA.md — new section documenting the lifecycle, the UpgradeEvent payload schema, and the legacy event's continued shape.
Note on test-harness limitation
Soroban SDK 22's test environment swaps the native test contract to WASM at the end of any call to update_current_contract_wasm, and the harness does not surface contract-level events through env.events().all() after that swap (same caveat documented at contracts/revenue_pool/src/test.rs::upgrade_sets_version_with_uploaded_wasm). The integration tests therefore assert every property of the emit that the harness can observe — state side effects, Val round-trip of the payload (both Some and None previous_wasm), topic byte strings, and the full lifecycle composing without panic. End-to-end visibility of the post-upgrade event payload belongs in soroban-rpc-backed E2E.
Note on scope
Strictly events. The discovered absence of an explicit caller != admin identity check inside upgrade() (the function fetches admin but never compares) is not addressed in this PR. The new UpgradeEvent.caller field makes that gap more observable to indexers but does not close it. Should be filed as a separate issue.
Acceptance criteria
Structured event payload emitted on every upgrade
Lifecycle (started → completed) so indexers can detect mid-upgrade state
previous_wasm carried alongside new_wasm for audit reconstruction
Backwards-compatible legacy upgraded event retained
EVENT_SCHEMA.md updated with the new topic strings and payload schema
NatSpec-style /// rustdoc on the new type, helpers, and upgrade()'s # Events block
No breaking ABI change (additive: new topics, new exported type)
Test plan
cargo test -p callora-vault --test upgrade_events → 5 passed
CI green (note: settlement crate currently has its own pre-existing merge breakage from prior -X theirs merges; if CI is red for that reason it is not a regression from this PR)