Conversation
…date tests/scripts; use native ops; register TidalMath; remove DeFiActionsMathUtils uses
…sitive dependency)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ent, and justify UFix128 usage for recordDeposit/recordWithdrawal and TokenState
cae7c0d to
e3276ff
Compare
…ain and TidalMath; adopt Type in events; remove DeFiActionsMathUtils from risk math; keep UFix128 health APIs and conversions
… in risk math, adopt Type in events, fix healthComputation (∞ for 0/0 and debt=0), adjust tests/scripts to UFix128 health; all tests passing
…nst 0.0 as UFix128
Kay-Zee
left a comment
There was a problem hiding this comment.
A few things, it definitely felt like the mul function is kinda unnecessary, any reason we're keeping it instead of just using *?
Also, we have a constant for 1.0 and 0.0 as UFix128 but we pretty much only use the 1.0 and not really use the 0.0 constant. Any reasoning for why?
| } | ||
| Test.assert(quote.seizeAmount > 0.0, message: "Expected positive seizeAmount") | ||
| Test.assert(quote.newHF > hAfter && quote.newHF < DeFiActionsMathUtils.e24) | ||
| Test.assert(quote.newHF > hAfter && quote.newHF < TidalMath.toUFix128(1.0)) |
There was a problem hiding this comment.
looks like we have a constant for this, no?
| Test.assert(quote.newHF > hAfter && quote.newHF < TidalMath.toUFix128(1.0)) | |
| Test.assert(quote.newHF > hAfter && quote.newHF < TidalMath.one) |
| lb: DeFiActionsMathUtils.e24 | ||
| cf: UFix128(cf), | ||
| bf: UFix128(bf), | ||
| lb: UFix128(0.05) |
| } | ||
| } | ||
| } No newline at end of file | ||
| "contracts": { |
There was a problem hiding this comment.
was this mostly an indentation issue?
| @@ -0,0 +1,92 @@ | |||
| access(all) contract TidalMath { | |||
There was a problem hiding this comment.
You mention in the PR that:
DeFiActionsUtils and DeFiActionsMathUtils are no longer required by TidalProtocol; TidalMath supersedes them.
Can we give a quick explainer why we don't just update DeFiActionsUtils to use ufix128 then continue to use the DeFiActionsUtils?
| self.debitInterestIndex = DeFiActionsMathUtils.e24 | ||
| self.currentCreditRate = UInt128(DeFiActionsMathUtils.e24) | ||
| self.currentDebitRate = UInt128(DeFiActionsMathUtils.e24) | ||
| self.totalCreditBalance = 0.0 as UFix128 |
There was a problem hiding this comment.
Did we not also have this as a constant?
| @@ -1701,19 +1729,19 @@ access(all) contract TidalProtocol { | |||
| if potentialHealth <= targetHealth { | |||
| // We will hit the health target before using up all of the withdraw token credit. We can easily | |||
| // compute how many units of the token would bring the position down to the target health. | |||
| // let availableHealth = healthAfterDeposit == UInt128.max ? UInt128.max : healthAfterDeposit - targetHealth | |||
| // let availableEffectiveValue = (effectiveDebtAfterDeposit == 0 || availableHealth == UInt128.max) | |||
| // let availableHealth = healthAfterDeposit == UFix128.max ? UFix128.max : healthAfterDeposit - targetHealth | |||
There was a problem hiding this comment.
is this suppose to be part of the comments, or is it just commented out code? if so should we just remove?
| let uintPrice = DeFiActionsMathUtils.toUInt128(self.priceOracle.price(ofToken: type)!) | ||
| let uintCollateralFactor = DeFiActionsMathUtils.toUInt128(self.collateralFactor[type]!) | ||
| let uintBorrowFactor = DeFiActionsMathUtils.toUInt128(self.borrowFactor[type]!) | ||
| let uintAmount = TidalMath.toUFix128(amount) |
There was a problem hiding this comment.
Noticed that most of these, even though the type is converted to ufix, the naming is still uint, should we adjust them, up to you how you want to name them, buti just feel like uint is misleading now that the type is changed
| assert(perSecondScaledValue < UInt128.max, message: "Per-second interest rate \(perSecondScaledValue) is too high") | ||
| return UInt128(perSecondScaledValue + DeFiActionsMathUtils.e24) | ||
| access(all) view fun perSecondInterestRate(yearlyRate: UFix128): UFix128 { | ||
| let secondsInYearE24 = TidalMath.mul(31_536_000.0 as UFix128, TidalMath.one) |
There was a problem hiding this comment.
probably don't need to bother with this mul
| } | ||
|
|
||
| /// Returns the compounded interest index reflecting the passage of time | ||
| /// The result is: newIndex = oldIndex * perSecondRate ^ seconds | ||
| access(all) view fun compoundInterestIndex(oldIndex: UInt128, perSecondRate: UInt128, elapsedSeconds: UFix64): UInt128 { | ||
| access(all) view fun compoundInterestIndex(oldIndex: UFix128, perSecondRate: UFix128, elapsedSeconds: UFix64): UFix128 { | ||
| // For UFix128, we use repeated multiplication per second conservatively for now. |
There was a problem hiding this comment.
hmm, then should this be an area where we would consider keeping UInt128?
| access(all) case RoundEven | ||
| } | ||
|
|
||
| access(all) view fun mul(_ x: UFix128, _ y: UFix128): UFix128 { |
There was a problem hiding this comment.
If mul is just a convenience function for * i do wonder what benefit there is to having it?
…lMath.one/zero, simplify perSecondInterestRate, rename locals for clarity, update tests; all Cadence tests passing
UFix128 cleanup and math utilities consolidationWhat changed
Why
Responses to review points
Tests
|
Interest compounding performance updateWhat changed
Why
Tests
|
…erestIndex to exponentiation-by-squaring; tests passing
| TidalMath.div(trueDebt * depositPrice2, depositBorrowFactor2) | ||
| effectiveCollateralAfterDeposit = balanceSheet.effectiveCollateral + | ||
| TidalMath.mul(TidalMath.mul(uintDepositAmount - trueDebt, uintDepositPrice), uintDepositCollateralFactor) | ||
| (depositAmountU - trueDebt) * depositPrice2 * depositCollateralFactor2 |
There was a problem hiding this comment.
nit: not super fond of the 2 in the naming
| let remainder: UFix128 = value - truncatedAs128 | ||
|
|
||
| if remainder <= 0.0 as UFix128 { | ||
| if remainder == 0.0 as UFix128 { |
There was a problem hiding this comment.
nit: might as well use the constant
| @@ -0,0 +1,33 @@ | |||
| ### UFix128 cleanup and math utilities consolidation | |||
There was a problem hiding this comment.
Let's not commit these, best for these just to live on the PR, i saw you had added the original PR body to your git ignore, but maybe this folder should be as well
|
Could we also take a look at coverage before merging please |
…exponent checks vs naive multiplication
|
Addressed latest comments and increased coverage |
Overview
This PR migrates
TidalProtocolfrom scaledUInt128andDeFiActionsMathUtilsto nativeUFix128and a newTidalMathhelper.Why
TidalMath.What changed
cadence/lib/TidalMath.cdcwith constants (one, zero), toUFix128, and UFix64 rounding helpers (toUFix64Round, toUFix64RoundUp, toUFix64RoundDown).cadence/contracts/TidalProtocol.cdc:cadence/tests/test_helpers.cdc: UFix128 constants/helpers; deploy TidalMath before TidalProtocol.phase0_pure_math_test.cdcwith new TokenSnapshot and RiskParams fields.flow.jsonto register TidalMath; remove DeFiActionsMathUtils from emulator deployments.How it works
Breaking changes
Validation
DeFiActions utilities
Follow-ups