refactor: implement phase 0 - extract pure math functions from positionHealth by kgrgpg · Pull Request #35 · onflow/FlowALP

kgrgpg · 2025-08-04T13:59:12Z

Overview

This PR implements Phase 0 of the refactoring plan, extracting pure mathematical functions from the positionHealth method.

Implementation Status

Phase 0 - All tests passing

Changes

Documentation

REFACTORING_PLAN.md: Comprehensive plan outlining the functional programming approach
REFACTOR_EVALUATION.md: Evaluation criteria for the refactoring

Implementation (Phase 0)

Created pure value types (RiskParams, TokenSnapshot, PositionView) to represent immutable data
Extracted effectiveCollateral and effectiveDebt as pure functions
Refactored positionHealth to use new healthFactor pure function
Moved buildPositionView into Pool struct for proper scoping
Added parameter names for clarity in function calls

Bug Fixes

Fixed InternalBalance struct to use single parameterized initializer (Cadence best practice)
Resolved forward reference issues by moving structs before Pool resource
Updated all InternalBalance() calls to pass explicit parameters

Benefits

Improved testability: Pure functions can be tested in isolation
Better modularity: Clear separation between pure computations and stateful operations
Enhanced readability: Named parameters and focused functions make the code easier to understand
Reusability: Pure functions can be reused in other parts of the codebase

Testing

✅ All 12 tests passing successfully:

auto_borrow_behavior_test.cdc
funds_available_above_target_health_test.cdc
funds_required_for_target_health_test.cdc
platform_integration_test.cdc
pool_creation_workflow_test.cdc
position_lifecycle_happy_test.cdc
rebalance_overcollateralised_test.cdc
rebalance_undercollateralised_test.cdc
reserve_withdrawal_test.cdc
token_governance_addition_test.cdc
utils_test.cdc
zero_debt_withdrawal_test.cdc

The refactored code maintains the same functionality as the original implementation while following Cadence best practices.

codecov · 2025-08-04T14:00:48Z

Codecov Report

❌ Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cadence/contracts/TidalProtocol.cdc	95.89%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

- Created pure value types (RiskParams, TokenSnapshot, PositionView) - Extracted effectiveCollateral and effectiveDebt as pure functions - Refactored positionHealth to use new healthFactor pure function - Moved buildPositionView into Pool struct for proper scoping - Added parameter names for clarity in function calls This refactoring improves code modularity and testability by separating pure mathematical computations from stateful operations.

…mpatibility - Remove duplicate init functions (Cadence only allows one init per struct) - Use single parameterized initializer with explicit parameters - Update all InternalBalance() calls to pass direction and scaledBalance explicitly - All 12 tests now passing successfully

nialexsan · 2025-08-11T15:52:50Z

+• Mutable pieces that stay in storage  
+  – `Pool.globalLedger[..]` indexes, `InternalPosition.balances`, queues/reserves
+
+## 2. Code (only the slice; other commands stubbed with TODO)


can you regenerate cadence code in this file, it shows deprecated cadence syntax, and could cause more confusion if referenced

…add phase0 pure math tests; remove DeFiBlocks dir and REFACTOR_EVALUATION.md

…gn with Phase 0 implementation

sisyphusSmiling

Looks like this needs to merge latest from main after #33 and update the values to use the UInt128 with 24 decimal places.

sisyphusSmiling · 2025-08-13T22:20:34Z

+
+    // PURE HELPERS -------------------------------------------------------------
+
+    access(all) fun effectiveCollateral(credit: UInt256, snap: TokenSnapshot): UInt256 {


I believe this and the other pure helpers can be made view. They would also benefit from comments with a brief explanation of what they do and represent in context of the protocol

sisyphusSmiling · 2025-08-13T22:21:58Z

+    /// Copy-only representation of a position used by pure math
+    access(all) struct PositionView {
+        access(all) let balances: {Type: InternalBalance}
+        access(all) let tokenSnaps: {Type: TokenSnapshot}


np: snapshots is more intuitive IMO

sisyphusSmiling · 2025-08-13T22:29:45Z

+    access(all) fun healthFactor(view: PositionView): UInt256 {
+        var coll: UInt256 = 0
+        var debt: UInt256 = 0
+        for t in view.balances.keys {


I think being a bit more verbose is helpful for comprehension, maybe using token or type instead of t so a reader doesn't have to revisit the balances type definition.

sisyphusSmiling · 2025-08-13T22:30:35Z

+        var coll: UInt256 = 0
+        var debt: UInt256 = 0
+        for t in view.balances.keys {
+            let b = view.balances[t]!


Same comment as above - bal would be a bit clearer and consistent with code elsewhere in the contract

sisyphusSmiling · 2025-08-13T22:34:44Z

+    access(all) struct RiskParams {
+        access(all) let collateralFactor: UInt256
+        access(all) let borrowFactor: UInt256
+        access(all) let liquidationBonus: UInt256


Have we settled how we will go about liquidation? I thought it was going to be auction style, however this field makes me think it will be a set percentage

sisyphusSmiling · 2025-08-13T22:51:05Z

+
+        var effColl: UInt256 = 0
+        var effDebt: UInt256 = 0
+        for t in view.balances.keys {


Similar comments as above. I think these single letter and acronym variables can be difficult to follow.

sisyphusSmiling · 2025-08-13T22:51:56Z

+                    direction: bal.direction,
+                    scaledBalance: bal.scaledBalance
+                )
+                let ts = self._borrowUpdatedTokenState(type: t)


ts could be perceived further as TokenState or TokenSnapshot - I think state would be clear and concise enough

sisyphusSmiling · 2025-08-13T22:53:29Z

+                let bal = position.balances[t]!
+                balancesCopy[t] = TidalProtocol.InternalBalance(
+                    direction: bal.direction,
+                    scaledBalance: bal.scaledBalance
+                )


Not sure it matters too much since we need to iterate to build snaps, but we could add a method on InternalPosition like fun copyBalances() and copy from there without the need to iteratively reconstruct the balance. Up to you but noting since I've had to use that pattern elsewhere when copying a field from a reference to a composite type.

sisyphusSmiling · 2025-08-13T22:57:38Z

Perhaps this is something to add onto later, but I'd hope to see us add quite a few more test cases against these methods since part of the goal of this refactor was to increase test coverage over core calculations

…kenSnaps->snapshots; clearer var names; add InternalPosition.copyBalances; use TidalProtocolUtils.decimals in conversions

…solve conflicts; port pure helpers and PositionView types; keep origin/main positionHealth impl

…eFiActionsMathUtils; all Cadence tests passing

sisyphusSmiling · 2025-08-18T17:37:17Z

+        access(all) fun copyBalances(): {Type: InternalBalance} {
+            let copy: {Type: InternalBalance} = {}
+            for tokenType in self.balances.keys {
+                let balance = self.balances[tokenType]!
+                copy[tokenType] = TidalProtocol.InternalBalance(
+                    direction: balance.direction,
+                    scaledBalance: balance.scaledBalance
+                )
+            }
+            return copy
+        }


Suggested change

access(all) fun copyBalances(): {Type: InternalBalance} {

let copy: {Type: InternalBalance} = {}

for tokenType in self.balances.keys {

let balance = self.balances[tokenType]!

copy[tokenType] = TidalProtocol.InternalBalance(

direction: balance.direction,

scaledBalance: balance.scaledBalance

)

}

return copy

}

access(all) fun copyBalances(): {Type: InternalBalance} {

return self.balances

}

sisyphusSmiling · 2025-08-18T17:50:19Z

+                    risk: TidalProtocol.RiskParams(
+                        cf: DeFiActionsMathUtils.toUInt128(self.collateralFactor[t]!),
+                        bf: DeFiActionsMathUtils.toUInt128(self.borrowFactor[t]!),
+                        lb: DeFiActionsMathUtils.e24 + 50_000_000_000_000_000_000_000


Will we want to adjust the liquidation bonus at some point in the future? Potentially out of scope for this PR, but flagging in case we want this to be configurable

sisyphusSmiling · 2025-08-18T17:53:21Z

+                risk: TidalProtocol.RiskParams(
+                    cf: DeFiActionsMathUtils.toUInt128(self.collateralFactor[type]!),
+                    bf: DeFiActionsMathUtils.toUInt128(self.borrowFactor[type]!),
+                    lb: DeFiActionsMathUtils.e24 + 50_000_000_000_000_000_000_000


Same question I made in buildPositionView regarding ability to configure liquidation bonus value. Seems like the kind of thing we'd want under pool governance.

sisyphusSmiling · 2025-08-18T17:58:43Z

+                    cf: DeFiActionsMathUtils.toUInt128(self.collateralFactor[type]!),
+                    bf: DeFiActionsMathUtils.toUInt128(self.borrowFactor[type]!),


I noticed that we never actually use the UFix64 collateral or borrow factors. I think we could refactors those values to be UInt128 to avoid all the conversions whenever they're used. I suppose we could still accept them as UFix64 on addSupportedToken if we think that's less prone to input error, but perform the conversion once on write and just read the stored UInt128 value

…eturn value copy of balances

kgrgpg · 2025-08-19T14:34:25Z

Acknowledged the copyBalances suggestion. Since Cadence dictionaries are values, returning self.balances gives a safe copy. I simplified InternalPosition.copyBalances() to return the balances map directly. Governance-controlled liquidationBonus and storing risk factors as UInt128 (convert once on write) are good ideas; I’ll handle both in follow-up PRs to keep Phase 0 focused.

docs: add comprehensive refactoring plan for TidalProtocol

6c75572

kgrgpg changed the title ~~docs: add comprehensive refactoring plan for TidalProtocol~~ refactor: implement phase 0 - extract pure math functions from positionHealth Aug 7, 2025

nialexsan reviewed Aug 11, 2025

View reviewed changes

phase0: use pure helpers in availableBalance (preserve top-up path); …

a541a41

…add phase0 pure math tests; remove DeFiBlocks dir and REFACTOR_EVALUATION.md

kgrgpg mentioned this pull request Aug 12, 2025

Phase 0: availableBalance refactor to pure helpers + tests #36

Closed

docs: regenerate refactoring plan code to current Cadence syntax; ali…

7fa55e8

…gn with Phase 0 implementation

sisyphusSmiling reviewed Aug 14, 2025

View reviewed changes

kgrgpg added 3 commits August 18, 2025 15:59

refactor(core): make pure helpers view + docs; rename PositionView.to…

5cb14a7

…kenSnaps->snapshots; clearer var names; add InternalPosition.copyBalances; use TidalProtocolUtils.decimals in conversions

merge(main): adopt 24-decimal UInt128 math (DeFiActionsMathUtils); re…

65c6f50

…solve conflicts; port pure helpers and PositionView types; keep origin/main positionHealth impl

tests(core): migrate pure math tests and helpers to UInt128/e24 via D…

ea4cca6

…eFiActionsMathUtils; all Cadence tests passing

kgrgpg requested review from nialexsan and sisyphusSmiling August 18, 2025 14:26

sisyphusSmiling reviewed Aug 18, 2025

View reviewed changes

refactor(PositionView): simplify InternalPosition.copyBalances() to r…

6b21665

…eturn value copy of balances

kgrgpg merged commit faf2982 into main Aug 19, 2025
2 checks passed

kgrgpg deleted the refactor/better-code branch August 19, 2025 14:38


		// PURE HELPERS -------------------------------------------------------------

		access(all) fun effectiveCollateral(credit: UInt256, snap: TokenSnapshot): UInt256 {

		cf: DeFiActionsMathUtils.toUInt128(self.collateralFactor[type]!),
		bf: DeFiActionsMathUtils.toUInt128(self.borrowFactor[type]!),

Conversation

kgrgpg commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Implementation Status

Changes

Documentation

Implementation (Phase 0)

Bug Fixes

Benefits

Testing

Uh oh!

codecov Bot commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sisyphusSmiling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgrgpg commented Aug 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kgrgpg commented Aug 4, 2025 •

edited

Loading

codecov Bot commented Aug 4, 2025 •

edited

Loading