test(vault): cover admin-setter success paths and getter coverage gaps (#428-431) - #467
Merged
abayomicornelius merged 2 commits intoAug 24, 2026
Conversation
Fix pre-existing merge-corruption bugs in investment_vault that made the crate fail to compile from a clean checkout: - lib.rs: check_deposit_lock() had a stray, undefined-variable (`last_seq`) sequence-number check spliced into the middle of its body with mismatched braces; restore it to the single timestamp-based MIN_LOCK_PERIOD check its doc comment (Heliobond#33) describes. - lib.rs: get_withdrawal_window() and get_volume_fee_tier() were each missing their closing brace, merging their bodies into the following function. - events.rs: withdrawal_window_set() and funding_round_ended() were each missing their closing brace for the same reason. - types.rs: VaultError::EmptyBatchDeposit, FundingRoundActive, and InvestmentCapExceeded all shared discriminant 41 (E0081); reassign FundingRoundActive = 42 and InvestmentCapExceeded = 43 since no test or caller depends on their specific numeric values. None of this is reachable without a full crate compile, so the existing test-coverage issues (Heliobond#428-431) targeted in the next commit could not otherwise be verified.
set_max_transaction_amount/get_deposit_lock_expiry/is_funding_round_active Closes Heliobond#428 Closes Heliobond#429 Closes Heliobond#430 Closes Heliobond#431 Each of these entry points previously had only its #[only_owner] admin-rejection path exercised (in test_all_only_owner_functions_reject_non_admin_caller) or, for the two getters, no coverage at all: - test_set_bridge_persists_emits_event_and_is_idempotent / test_set_wormhole_core_persists (Heliobond#428): call set_bridge/ set_wormhole_core as owner, confirm the address round-trips through contract storage, confirm set_bridge's BridgeSet event fires on first call, and confirm the no-op-if-unchanged early return skips emitting a second event on a repeat call with the same address. - test_set_carbon_oracle_persists_emits_event_and_is_idempotent / test_set_max_transaction_amount_persists_emits_event_and_is_idempotent (Heliobond#429): same persist + event + no-op-early-return shape for set_carbon_oracle and set_max_transaction_amount. - test_get_deposit_lock_expiry (Heliobond#430): asserts 0 for a fresh account and deposited_at + MIN_LOCK_PERIOD immediately after a deposit. - test_is_funding_round_active_reflects_start_and_end (Heliobond#431): asserts the getter is false initially, true after start_funding_round, and false again after end_funding_round. Fixing check_deposit_lock() in the previous commit means the deposit lock is now genuinely enforced in tests (it previously could not run at all), which surfaced three more compile/runtime issues blocking any test from running: - test_get_project_investments_batch_returns_correct_amounts, test_get_all_project_investments_returns_all (reconstructed — its body had been entirely lost, with unrelated tests spliced into the gap) and test_claim_queued_is_idempotent_against_double_claim were calling registry_contract::create_project() with a stale 6-argument signature; updated to the current 4-argument (creator, uri, maturity_date, metadata_hash) signature. - test_claim_settles_queued_withdrawal_then_second_claim_is_noop (recovered from the same orphaned block as test_get_all_project_investments_returns_all, which had swallowed this test's real body) needed the ledger timestamp advanced past MIN_LOCK_PERIOD before withdrawing, now that the lock is enforced. - test_get_deposit_lock_expiry needed a non-zero ledger timestamp before depositing, since the getter's own 0-means-never-deposited sentinel is otherwise indistinguishable from a real deposit made at the test env's default timestamp of 0. `cargo test -p investment-vault --lib` passes for all tests touched by this change.
|
@chonilius 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.
Summary
Adds the missing success-path test coverage flagged in #428, #429, #430, and #431 — four
investment_vaultentry points that either had only their#[only_owner]admin-rejection path exercised, or (for the two getters) no coverage at all.set_bridge()/set_wormhole_core()(investment_vault: set_bridge() and set_wormhole_core() are never called successfully in any test — only their admin-rejection path is covered #428): new tests call each as owner, confirm the address round-trips through contract storage, confirmset_bridge'sBridgeSetevent fires on the first call, and confirm the no-op-if-unchanged early return (lib.rs:1247-1250) skips emitting a second event on a repeat call with the same address.set_carbon_oracle()/set_max_transaction_amount()(investment_vault: set_carbon_oracle() and set_max_transaction_amount() are never called successfully in any test — only their admin-rejection path is covered #429): same persist + event + no-op-early-return shape.get_deposit_lock_expiry()(investment_vault: get_deposit_lock_expiry() has zero test coverage #430): asserts it returns0for a fresh account anddeposited_at + MIN_LOCK_PERIODimmediately after a deposit.is_funding_round_active()(investment_vault: is_funding_round_active() getter has zero direct test coverage #431): assertsfalseinitially,trueafterstart_funding_round,falseagain afterend_funding_round.Why this touches more than test.rs
investment_vaultdid not compile from a clean checkout ofmain— the crate has several pre-existing merge-corruption bugs unrelated to these issues that had to be fixed before any test (old or new) could even run:check_deposit_lock()had a stray, undefined-variable (last_seq) sequence-number check spliced into the middle of its body with mismatched braces. Restored it to the single timestamp-basedMIN_LOCK_PERIODcheck its own doc comment ([Logic] Implement share lock-up period after deposit #33) describes.get_withdrawal_window()andget_volume_fee_tier()in lib.rs, andwithdrawal_window_set()/funding_round_ended()in events.rs, were each missing their closing brace, silently merging their bodies into the next function.VaultError::EmptyBatchDeposit,FundingRoundActive, andInvestmentCapExceededall shared discriminant41(E0081). ReassignedFundingRoundActive = 42andInvestmentCapExceeded = 43— nothing depends on their specific numeric values.Fixing
check_deposit_lock()means the deposit lock is now genuinely enforced during tests (previously it could not run at all), which surfaced a few more pre-existing issues blocking the suite from running:test_get_project_investments_batch_returns_correct_amountsandtest_claim_queued_is_idempotent_against_double_claimwere callingregistry_contract::create_project()with a stale 6-argument signature; updated to the current 4-argument(creator, uri, maturity_date, metadata_hash)signature.test_get_all_project_investments_returns_allhad entirely lost its body (with unrelated, otherwise-complete tests spliced into the gap) — reconstructed a minimal body matching its name.test_claim_settles_queued_withdrawal_then_second_claim_is_noopand advanced its ledger timestamp pastMIN_LOCK_PERIODbefore withdrawing, now that the lock is enforced.test_get_deposit_lock_expiry(this PR's own new test) needed a non-zero ledger timestamp before depositing, since the getter's own "0 means never deposited" sentinel is otherwise indistinguishable from a real deposit made at the test env's default timestamp of0.Test plan
cargo test -p investment-vault --lib— full suite compiles and runs (previously failed to compile at all)test_set_bridge_persists_emits_event_and_is_idempotent,test_set_wormhole_core_persists,test_set_carbon_oracle_persists_emits_event_and_is_idempotent,test_set_max_transaction_amount_persists_emits_event_and_is_idempotent,test_get_deposit_lock_expiry,test_is_funding_round_active_reflects_start_and_endtest_get_all_project_investments_returns_all,test_get_project_investments_batch_returns_correct_amounts,test_claim_queued_is_idempotent_against_double_claim,test_claim_settles_queued_withdrawal_then_second_claim_is_noop,test_get_set_withdrawal_window,test_volume_fee_tier_is_admin_onlyCloses #428
Closes #429
Closes #430
Closes #431