Merge PR #14 (Test Infrastructure) into refactored pool creation branch#19
Merge PR #14 (Test Infrastructure) into refactored pool creation branch#19kgrgpg merged 64 commits intotracer-bulletfrom
Conversation
- TidalProtocol: 100% restoration of Dieter's AlpenFlow functionality - Oracle-based pricing and health calculations - Deposit rate limiting and position queues - Advanced health management functions - Async position updates - MOET: Mock stablecoin for multi-token testing - TidalPoolGovernance: Role-based governance system - AlpenFlow_dete_original: Reference implementation Note: Old tests removed as they're incompatible with new contracts. Updated tests coming in follow-up PR. No breaking changes. Foundation for multi-token lending protocol.
…pulling from topUpSource
…lization rebalancing
…st practices guide
Kan/happy path test improvement
- Added expected debt calculations and assertions in both overcollateralized and undercollateralized rebalance tests - For overcollateralized: Assert that after price increase and rebalance, debt = effective_collateral / target_health (≈738.46 MOET) - For undercollateralized: Assert that after price drop and rebalance, debt is reduced by the exact amount needed to restore target health (≈516.92 MOET) - Added debug logging to verify calculations match actual values - All tests now pass with precise value assertions as requested in PR review
…flow/TidalProtocol into cadence-testing-patterns-fork # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
- Fixes fundsAvailableAboveTargetHealthAfterDepositing to handle zero debt case properly - When there's no debt, use entire collateral amount instead of health ratio calculation - Fixes repay_and_close_position.cdc to use availableBalance() instead of hardcoded 1000.0 - This resolves the issue where withdrawing all collateral caused health to drop to 1.0 (below minimum 1.1)
- Tests that when a position has no debt, availableBalance() returns full collateral amount - Regression test for the bug where zero debt positions couldn't withdraw (availableBalance returned 0) - Verifies the conditional logic: when effectiveDebtAfterDeposit == 0.0, use effectiveCollateralAfterDeposit
…AfterDeposit == 0.0
* update PositionDetails and PositionBalance comments and fields * consolidate public methods to the bottom of the contract * remove comment references to restored functionality * update contract comments * update contract comments and reorganize for clarity * update contract and construct internal methods * update contract comments * revert changes to PositionDetails balances field * update contract comments * Update cadence/contracts/TidalProtocol.cdc Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org> * update interest index & balance type comments --------- Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org>
Resolved conflicts by accepting structural changes from gio/refactor-pool-creation-updated branch, which already includes all bug fixes from PR #14: - Zero debt fix for fundsAvailableAboveTargetHealthAfterDepositing - Division by zero fix in healthComputation - Improved internal function naming with _ prefix - Event emissions (Opened, Deposited, Withdrawn, Rebalanced) - Better code organization and error messages Preserved test infrastructure and documentation from PR #14
…references Fixed all test files and scripts to use the correct field name 'vaultType' instead of 'type' when accessing PositionBalance struct fields. This aligns with the struct definition in TidalProtocol.cdc and resolves test failures. Updated files: - cadence/scripts/tidal-protocol/get_position_details.cdc - cadence/transactions/tidal-protocol/pool-management/repay_and_close_position.cdc - cadence/tests/auto_borrow_behavior_test.cdc - cadence/tests/rebalance_overcollateralised_test.cdc - cadence/tests/rebalance_undercollateralised_test.cdc All tests now pass successfully.
Critical fixes: - Fixed DFBUtils naming conflict by using real DFBUtils from DeFiBlocks - Removed incompatible emulator addresses from flow.json - Made testing addresses consistent with 0x prefix Best practice improvements: - Moved repay_and_close_position.cdc to tests directory - Refactored transaction to use prepare/execute blocks - Applied principle of least privilege (BorrowValue authorization) - Removed test logging from transaction - Added specific MOET balance assertion in position_lifecycle_happy_test - Removed unused get_position_details.cdc script All tests passing successfully
✅ Addressed all review comments from PR #14I've implemented all the feedback from @sisyphusSmiling on PR #14: Critical Issues Fixed:
Best Practice Improvements:
Test Results:All 9 test suites pass ✅ This PR is now ready for review with all the improvements from PR #14's feedback incorporated. |
| // Verify the amount is approximately what we calculated (within 0.01 tolerance) | ||
| Test.assert(moetBalance >= expectedDebt - 0.01 && moetBalance <= expectedDebt + 0.01, | ||
| message: "Expected MOET debt to be approximately \(expectedDebt), but got \(moetBalance)") |
There was a problem hiding this comment.
Could also check the user's account state to ensure the funds actually made their way to the user's MOET Vault with the helper getBalance(address: user.address, vaultPublicPath: MOET.VaultPublicPath). That would ensure the minted funds are actually deposited to the drawDownSink and not just adjusted within the protocol balances.
| Test.assert(!hasMoetBalance, | ||
| message: "Should not have MOET balance when no auto-borrowing occurs") |
There was a problem hiding this comment.
Same note about checking user account state to ensure no funds were deposited without incrementing locally. Another check on this might be validating against the MOET totalSupply to ensure that no tokens were minted.
| let flowBalanceBefore = getBalance(address: user.address, vaultPublicPath: /public/flowTokenReceiver)! | ||
| log("Flow balance BEFORE repay: ".concat(flowBalanceBefore.toString())) | ||
|
|
||
| /* --- NEW: repay MOET and close position --- */ |
There was a problem hiding this comment.
| /* --- NEW: repay MOET and close position --- */ | |
| // repay MOET and close position |
| Test.assert(healthAfterPriceChange > healthBefore) // got healthier due to price increase | ||
| Test.assert(healthAfterRebalance < healthAfterPriceChange) // health decreased after drawing down excess collateral |
There was a problem hiding this comment.
These directional checks make sense. Are we able to assert either the exact or minimum expected healthAfterPriceChange? I would think it should at least be greater than the minimum health if not the target health.
| } | ||
|
|
||
| let tolerance: UFix64 = 0.01 | ||
| Test.assert((actualDebt >= expectedDebt - tolerance) && (actualDebt <= expectedDebt + tolerance)) |
There was a problem hiding this comment.
Noting again regarding checking user account state. I'm assuming we'd also want to ensure the MOET was actually pushed to the drawDownSink which in the position setup is configured to direct borrowed funds to their MOET Vault.
| // drop price | ||
| setMockOraclePrice(signer: protocolAccount, forTokenIdentifier: flowTokenIdentifier, price: initialPrice * (1.0 - priceDropPct)) | ||
|
|
||
| let availableAfterPriceChange = getAvailableBalance(pid: 0, vaultIdentifier: defaultTokenIdentifier, pullFromTopUpSource: true, beFailed: false) |
There was a problem hiding this comment.
I don't see this value used anywhere. It'd be good to include at least a directional if not exact assertion on the available balance after the price adjustment.
| let healthAfterPriceChangeVal: UFix64 = healthAfterPriceChange | ||
| let target: UFix64 = 1.3 | ||
|
|
||
| let requiredPaydown: UFix64 = (target - healthAfterPriceChangeVal) * effectiveCollateralAfterDrop / (target * target) |
There was a problem hiding this comment.
A comment on this calculation would be helpful
| log("Health after price change: ".concat(healthAfterPriceChange.toString())) | ||
| log("Required paydown: ".concat(requiredPaydown.toString())) | ||
| log("Expected debt: ".concat(expectedDebt.toString())) | ||
| log("Actual debt: ".concat(actualDebt.toString())) |
There was a problem hiding this comment.
We'll want to also ensure that the debt was actually repaid and not just altered in the ledger. Checking user's MOET balance as well as the available balance via the position IMO are important for this test case as well.
- auto_borrow_behavior_test: verify user MOET vault receives borrowed funds - auto_borrow_behavior_test: ensure no MOET minted when pushToDrawDownSink=false - position_lifecycle_happy_test: update comment style from /* */ to // - rebalance_overcollateralised_test: add minimum health assertion and user balance check - rebalance_undercollateralised_test: add explanatory comment for paydown calculation All tests passing successfully.
✅ All review comments have been addressedThank you for the thorough review @sisyphusSmiling! I've implemented all the suggested improvements to enhance test coverage. Here's what was done:
|
sisyphusSmiling
left a comment
sisyphusSmiling
left a comment
There was a problem hiding this comment.
Just a couple smaller suggestions and callouts
| Test.assert(health >= 1.29 && health <= 1.31, | ||
| message: "Expected health to be at target (1.3), but got \(health)") | ||
|
|
||
| // NEW: Verify the user actually received the borrowed MOET in their Vault (draw-down sink) |
There was a problem hiding this comment.
| // NEW: Verify the user actually received the borrowed MOET in their Vault (draw-down sink) | |
| // Verify the user actually received the borrowed MOET in their Vault (draw-down sink) |
| Test.assert(!hasMoetBalance, | ||
| message: "Should not have MOET balance when no auto-borrowing occurs") | ||
|
|
||
| // NEW: Ensure user's MOET balance remains unchanged (i.e. no tokens minted) |
There was a problem hiding this comment.
| // NEW: Ensure user's MOET balance remains unchanged (i.e. no tokens minted) | |
| // Ensure user's MOET balance remains unchanged (i.e. no tokens minted) |
|
|
||
| let healthAfterPriceChange = getPositionHealth(pid: 0, beFailed: false) | ||
|
|
||
| // NEW: After a 20% price increase, health should be at least 1.5 (=960/615.38) |
There was a problem hiding this comment.
| // NEW: After a 20% price increase, health should be at least 1.5 (=960/615.38) | |
| // After a 20% price increase, health should be at least 1.5 (=960/615.38) |
There was a problem hiding this comment.
I think we should also add cases including coverage for more extreme price changes such as 100% and 1,000%. The 10x increase on the demo call earlier didn't seem to rebalance to the target health and was still undercollateralized, but needs test case validation and coverage.
There was a problem hiding this comment.
This is indeed important. But similar tests where parameter values need to be changed fall more under fuzzy tests category. Will target these in future tests/PRs.
There was a problem hiding this comment.
Same callout as the undercollateralized test file - it'd be helpful to cover more extreme price changes up to an order of magnitude.
There was a problem hiding this comment.
This is indeed important. But similar tests where parameter values need to be changed fall more under fuzzy tests category. Will target these in future tests/PRs.
There was a problem hiding this comment.
Calling out that this is a framing and doesn't actually test anything.
There was a problem hiding this comment.
Calling out that this is a a TODO and isn't actually executing anything meaningful.
There was a problem hiding this comment.
This is indeed as you mentioned.
Here’s the current situation for the reserve-withdrawal flow:
-
cadence/tests/reserve_withdrawal_test.cdc
• The file only sets up a pool and then calls the reserve-withdrawal transaction, but it has no assertions beyond aTest.expectthat the transaction “succeeds”.
• We created the scaffolding early to verify that the transaction script itself type-checks and can be signed/executed by the governance signer, but we haven’t added behavioural checks (e.g. before/after reserve balance, events, or account state).
• Essentially it’s a placeholder so the repo builds green while governance-withdraw logic is still in flux. -
cadence/transactions/tidal-protocol/pool-governance/withdraw_reserve.cdc
• The transaction currently just imports the protocol and callsTidalProtocol.withdrawReserve(...)with hard-coded/placeholder arguments.
• Internally that call is still a TODO—the governance module will eventually restrict who may withdraw, emitReserveWithdrawnevents, and move tokens to the treasury multisig.
• Until that implementation lands, the transaction is merely a stub to keep the on-chain interface stable for front-end work.
Why we left it this way
- Governance module: we’re finalising the access-control and timelock design. Implementing the full withdrawal path now would require re-work once those decisions settle.
- Continuous integration: the empty test guarantees that the stub transaction compiles after every change to contracts or imports. If the protocol, token identifiers, or helper functions change, CI will catch it because this test runs.
Next steps (tracked in #42)
- Implement
PoolGovernance.withdrawReservewith:
• role-based access (GovernanceAdmin)
• event emission (ReserveWithdrawn)
• balance checks against the pool’s accounting - Flesh out
withdraw_reserve.cdcto pass amount/receiver params. - Expand
reserve_withdrawal_test.cdcto:
• Assert pre-/post-reserve balances
• Verify event fields
• Ensure only authorised signer can call the tx.
Until those pieces are complete the test remains a “framing” check rather than a behavioural test.
|
|
||
| rebalancePosition(signer: protocolAccount, pid: 0, force: true, beFailed: false) | ||
|
|
||
| let healthAfterRebalance = getPositionHealth(pid: 0, beFailed: false) |
There was a problem hiding this comment.
I realized after approving that we don't assert that the health post-rebalance is above the minimum health. We should add that here and add a more extreme price drop rebalance scenario to ensure the protocol is rebalancing to at least the minimum health @kgrgpg.
There was a problem hiding this comment.
Added assertion. Extreme price drop will be implemented with fuzzy tests c6c8350
Clean up comment style by removing NEW: prefixes while maintaining the same functionality and test coverage.
Per PR review, add assertion that health after rebalance is at least 1.1 (minimum threshold) to ensure protocol rebalances positions to a safe state.
|
Closing since #19 (that was created with transfer of work) is ready to be merged |
3 participants
This PR merges the test infrastructure from PR #14 into the refactored pool creation branch.
Changes Included
type→vaultType) in PositionBalance referencesImportant Notes
Conflicts Resolved
test_helpers.cdc- Kept the defaultTokenIdentifier constantTidalProtocol.cdc- Used the refactored version (no functional changes needed)Test Results
All 9 test suites pass successfully:
Supersedes PR #14