test(vault): explicit require_auth assertions for set_admin - #585
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.
|
@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! 🚀 |
|
CI is failing on a build error in the The settlement crate currently has ~77 compile errors (duplicate Locally, with that vault repair in place: ``` $ cargo test -p callora-vault --test set_admin_auth (Test was verified by temporarily commenting out the `callora-settlement` dev-dep in `contracts/vault/Cargo.toml`; that change is not in this PR — the dev-dep is restored to its original state.) The settlement crate has a real product question underneath: it looks like a per-token storage migration (`StorageKey::DeveloperBalance(addr, token)`, `StorageVersion`, the `migrate` module's V1→V2 path) was staged but never finished. Reconciling it requires deciding whether that migration is meant to land or to be rolled back — I don't want to guess on that. Could a maintainer confirm the intended direction and either fix settlement here or flag it as a separate issue? |
2 participants
CLOSE #534
Summary
CalloraVault::set_adminis gated by bothcaller.require_auth()and an in-bodycaller != current_adminidentity check. The existingset_admin_unauthorized_failstest runs underenv.mock_all_auths(), which silently approves any signer — so it only exercises the identity check. Ifcaller.require_auth()were ever removed (intentionally or via a bad merge), the existing test would still pass, leaving the contract one line away from accepting unauthenticated admin rotations.This PR adds an integration-test file that asserts the Soroban auth framework itself is the gate.
Changes
contracts/vault/tests/set_admin_auth.rs(new) — three integration tests:set_admin_fails_without_authorization—env.set_auths(&[])+try_set_adminmust errorset_admin_records_caller_auth_entry—env.auths()must contain an entry for the caller after a successful callset_admin_fails_when_only_other_party_authorizes— auth mocked for a different address than the declaredcallermust still failcontracts/vault/src/lib.rs— separate commit; reconstructs merge corruption left by PRs #578/#579 "-X theirs" auto-resolutions so the workspace compiles. See commit message for the line-by-line list.Note on "multisig" wording
The bounty asks for tests that
set_admin"requires multisig." This contract uses a single-admin model with a two-step pending/accept transfer (lib.rs:574-619), not multisig. The tests assert the strongest property the current contract supports: that Soroban'srequire_authframework gatesset_admin.Acceptance criteria
require_authis invoked (not just the identity check)cargo test -p callora-vault --test set_admin_auth///rustdoc on every test plus//!module preambleset_adminTest plan
cargo test -p callora-vault --test set_admin_auth→ 3 passedKnown pre-existing breakage NOT addressed here
The
callora-settlementcrate has ~77 errors from the same kind of "-X theirs" merge corruption (missing module declarations, duplicateget_storage_ttl, driftedDeveloperBalance.tokenfield, missingStorageKey::DeveloperBalanceV1/StorageVersionvariants). Fixing it requires judgment on whether the per-token storage migration was meant to land — out of scope for this PR.