Skip to content

Fix precision-related bugs in TidalProtocol calculations#26

Merged
sisyphusSmiling merged 1 commit intogio/update-decimal-precisionfrom
precision-fixes-integration-final
Jul 14, 2025
Merged

Fix precision-related bugs in TidalProtocol calculations#26
sisyphusSmiling merged 1 commit intogio/update-decimal-precisionfrom
precision-fixes-integration-final

Conversation

@kgrgpg
Copy link
Copy Markdown
Contributor

@kgrgpg kgrgpg commented Jul 14, 2025

Summary

This PR fixes critical bugs discovered during integration testing of the decimal precision updates.

Changes

1. Fixed Interest Multiplication Overflow

  • Updated interestMul function to use correct divisor for 18-decimal precision
  • Changed from dividing by e8 (100M) to dividing by 10^9 (1B)
  • This matches the new 18-decimal precision for interest indices

2. Fixed Underflow in fundsRequiredForTargetHealthAfterWithdrawing

  • Corrected the formula for calculating required effective debt
  • Changed from (effectiveDebtAfterWithdrawal - effectiveCollateralAfterWithdrawal) / targetHealth
  • To: effectiveDebtAfterWithdrawal - (effectiveCollateralAfterWithdrawal / targetHealth)
  • Added underflow protection when subtracting debt values

3. Enhanced Error Logging

  • Added detailed logging when withdrawals fail
  • Logs position ID, token type, requested amount, available balance, and required deposit

Impact

These fixes ensure:

  1. Interest calculations work correctly with the new 18-decimal precision
  2. Health calculations don't cause underflow errors
  3. Better debugging information for failed operations

Related Issues

Part of the decimal precision migration from UFix64 to UInt256.

- Changed interestMul division from e8 to 10^9 to match 18-decimal indices
- Fixed missing division by targetHealth in fundsRequiredForTargetHealthAfterWithdrawing
- Added underflow protection when subtracting debt values
@kgrgpg
Copy link
Copy Markdown
Contributor Author

kgrgpg commented Jul 14, 2025

Test Results Update

All rebalance scenario tests in tidal-sc have been updated with precise expected values from the Google Sheet (truncated to 8 decimal places for UFix64 compatibility).

Current Test Results:

  • Scenario 1 (Flow Price Changes): ✅ PASS - Excellent precision with 4 perfect matches
  • Scenario 2 (Yield Price Increases): ✅ PASS - Consistent small negative differences
  • Scenario 3a: ❌ FAIL - Insufficient funds on tide closure (0.00000001 shortfall)
  • Scenario 3b: ✅ PASS
  • Scenario 3c: ✅ PASS
  • Scenario 3d: ❌ FAIL - Insufficient funds on tide closure (precision beyond 8 decimals)

Key Findings:

  • Maximum precision differences are extremely small (< 0.00001 in all cases)
  • Failures in 3a and 3d are due to accumulated precision differences when closing tides
  • These differences are expected and acceptable when migrating from UFix64 to UInt256

Recommendations:

  1. Add tolerance (e.g., 0.00000001) when checking available funds

The enhanced error logging added in this PR has been very helpful in debugging these precision issues. Full precision comparison report is available in the tidal-sc repository.

Comment on lines +1838 to +1839
let aScaled = a / 1_000_000_000 // 10^9
let bScaled = b / 1_000_000_000 // 10^9
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update TidalProtocolUtils.e8 to e9 = 1_000_000_000

@sisyphusSmiling sisyphusSmiling merged commit 3460ad5 into gio/update-decimal-precision Jul 14, 2025
1 check failed
@sisyphusSmiling sisyphusSmiling deleted the precision-fixes-integration-final branch July 14, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants