fix(vault): align event schema with EVENTS.md and expand deposit/withdraw tests#200
fix(vault): align event schema with EVENTS.md and expand deposit/withdraw tests#200kunal-drall wants to merge 1 commit intoSuncrest-Labs:mainfrom
Conversation
DepositEventData and WithdrawEventData were emitting a `total_assets` field, but EVENTS.md specifies the field must be named `total_deposits` for both DEPOSIT and WITHDRAW vault events. Rename the field in both structs and their emit_event call sites. Expand the test suite from 27 to 33 tests to satisfy the acceptance criteria around exchange-rate correctness: - test_exchange_rate_appreciates_after_yield: verifies share value increases proportionally after yield is reported - test_second_depositor_share_dilution: verifies a depositor joining at a 2:1 exchange rate receives half as many shares, preserving existing holders' claim on assets - test_partial_withdrawal_preserves_exchange_rate: verifies that a partial withdrawal does not alter the per-share value for remaining holders - test_full_cycle_leaves_vault_empty: verifies a complete deposit + withdrawal drains the vault contract balance to zero - test_deposit_at_exact_cap_succeeds_above_cap_fails: tightens the existing max-deposit cap test to cover the exact boundary
There was a problem hiding this comment.
Pull request overview
This PR aligns the vault’s emitted event payload field name with EVENTS.md and expands unit test coverage around ERC-4626-style share accounting and exchange-rate behavior.
Changes:
- Renames
total_assets→total_depositsinDepositEventData/WithdrawEventDataand updates emit call sites. - Adds new tests covering exchange-rate appreciation, share dilution, partial withdrawals, full-cycle emptying, and max-deposit boundary behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/contracts/contracts/vault/src/lib.rs |
Renames an event payload field and updates deposit/withdraw event emission. |
packages/contracts/contracts/vault/src/test.rs |
Adds exchange-rate/share-accounting and cap boundary tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| amount, | ||
| shares_minted: shares_to_mint, | ||
| new_balance: new_user_shares, | ||
| total_assets: total_assets + amount, | ||
| total_deposits: total_assets + amount, | ||
| }, |
There was a problem hiding this comment.
total_deposits in the DEPOSIT event is currently set to total_assets + amount (gross assets). The contract’s own get_total_deposits() view returns total_assets - accrued_fees, so when management fees have accrued this event field will be higher than the actual withdrawable deposits. Consider emitting total_assets_after - accrued_fees_after (i.e., the same value returned by get_total_deposits()) to keep the public event payload consistent.
| shares_burned: shares, | ||
| new_balance: new_user_shares, | ||
| total_assets: total_assets - assets_to_withdraw, | ||
| total_deposits: total_assets - assets_to_withdraw, |
There was a problem hiding this comment.
total_deposits in the WITHDRAW event is currently set to total_assets - assets_to_withdraw (gross assets after paying the user). Because accrued_fees increases by total_fee in the same function, this emitted value will include fees and will not match get_total_deposits() (which subtracts accrued fees). Consider emitting total_assets_after - accrued_fees_after so the event reflects net deposits available to shareholders.
| total_deposits: total_assets - assets_to_withdraw, | |
| // total_deposits should represent net deposits available to shareholders: | |
| // total_deposits_after = (total_assets - assets_to_withdraw) - (accrued_fees + total_fee) | |
| total_deposits: (total_assets - assets_to_withdraw) - (accrued_fees + total_fee), |
| pub new_balance: i128, | ||
| pub total_assets: i128, | ||
| pub total_deposits: i128, | ||
| pub fee_deducted: i128, |
There was a problem hiding this comment.
WithdrawEventData still includes fee_deducted, but packages/contracts/EVENTS.md specifies the WITHDRAW payload as { amount, shares_burned, new_balance, total_deposits } only. If the intent of this PR is to strictly align emitted event schemas with EVENTS.md, either update the spec to include fee_deducted or remove it from the emitted data to avoid breaking indexers that rely on the documented schema.
| pub fee_deducted: i128, |
| // Depositing when the vault has reached max capacity must be rejected. | ||
| #[test] | ||
| fn test_deposit_at_exact_cap_succeeds_above_cap_fails() { | ||
| let (env, admin, token_address, _contract_id, client) = setup(); | ||
| let user = Address::generate(&env); | ||
|
|
||
| client.set_max_deposit(&admin, &300); | ||
| mint_tokens(&env, &token_address, &user, 1_000); |
There was a problem hiding this comment.
The test name/comment says this is a “max capacity” check, but the contract’s MaxDeposit guard enforces a per-deposit amount (if amount > max_deposit) rather than a vault-wide TVL/cap check. Renaming the test/comment to reflect per-deposit limits (or adjusting the contract + test to enforce a cumulative cap) would avoid confusion about what behavior is guaranteed.
|
@kunal-drall Resolve conflicts please |
Closes #93
What changed
Event schema fix (
lib.rs)DepositEventDataandWithdrawEventDatawere emitting atotal_assetsfield. EVENTS.md defines the field astotal_depositsfor both theDEPOSITandWITHDRAWvault events. The field has been renamed in both structs and at everyemit_eventcall site. Internal storage helpers (get_total_assets/set_total_assets) are unaffected — the rename is scoped to the public event payload only.Exchange-rate test coverage (
test.rs)The existing suite covered happy-path deposit/withdraw and error paths but had no tests verifying the ERC-4626-style share accounting. Six tests were added:
test_exchange_rate_appreciates_after_yieldtest_second_depositor_share_dilutiontest_partial_withdrawal_preserves_exchange_ratetest_full_cycle_leaves_vault_emptytest_deposit_at_exact_cap_succeeds_above_cap_failsAcceptance criteria
total_depositsfield name fixed)