refactor: Address PR comments (Oracle abstraction, naming, safe redemption)

kgrgpg · kgrgpg · commit 26ebbe9622a9 · 2025-11-19T15:55:51.000+01:00
diff --git a/cadence/REDEMPTION_GUIDE.md b/cadence/REDEMPTION_GUIDE.md
diff --git a/cadence/contracts/RedemptionWrapper.cdc b/cadence/contracts/RedemptionWrapper.cdc
@@ -23,7 +23,7 @@ access(all) contract RedemptionWrapper {
         moetBurned: UFix64,
         collateralType: String,
         collateralReceived: UFix64,
-        oraclePrice: UFix64,
+        collateralOraclePrice: UFix64,
         preRedemptionHealth: UFix128,
         postRedemptionHealth: UFix128
     )
@@ -42,7 +42,7 @@ access(all) contract RedemptionWrapper {
     access(all) var minRedemptionAmount: UFix64
     
     // Rate limiting
-    access(all) var redemptionCooldown: UFix64
+    access(all) var redemptionCooldownSeconds: UFix64
     access(all) var dailyRedemptionLimit: UFix64
     access(all) var dailyRedemptionUsed: UFix64
     access(all) var lastRedemptionResetDay: UFix64
@@ -51,6 +51,7 @@ access(all) contract RedemptionWrapper {
     // Oracle and health protections
     access(all) var maxPriceAge: UFix64
     access(all) var minPostRedemptionHealth: UFix128
+    access(all) var oracle: {DeFiActions.PriceOracle}
     
     // Position tracking
     access(all) var positionID: UInt64?
@@ -77,22 +78,26 @@ access(all) contract RedemptionWrapper {
         }
 
         access(all) fun setProtectionParams(
-            redemptionCooldown: UFix64,
+            redemptionCooldownSeconds: UFix64,
             dailyRedemptionLimit: UFix64,
             maxPriceAge: UFix64,
             minPostRedemptionHealth: UFix128
         ) {
             pre {
-                redemptionCooldown <= 3600.0: "Cooldown too long (max 1 hour)"
+                redemptionCooldownSeconds <= 3600.0: "Cooldown too long (max 1 hour)"
                 dailyRedemptionLimit > 0.0: "Daily limit must be positive"
                 maxPriceAge <= 7200.0: "Max price age too long (max 2 hours)"
                 minPostRedemptionHealth >= FlowALPMath.toUFix128(1.0): "Min post-redemption health must be >= 1.0"
             }
-            RedemptionWrapper.redemptionCooldown = redemptionCooldown
+            RedemptionWrapper.redemptionCooldownSeconds = redemptionCooldownSeconds
             RedemptionWrapper.dailyRedemptionLimit = dailyRedemptionLimit
             RedemptionWrapper.maxPriceAge = maxPriceAge
             RedemptionWrapper.minPostRedemptionHealth = minPostRedemptionHealth
         }
+        
+        access(all) fun setOracle(_ newOracle: {DeFiActions.PriceOracle}) {
+            RedemptionWrapper.oracle = newOracle
+        }
 
         access(all) fun pause() {
             RedemptionWrapper.paused = true
@@ -123,7 +128,7 @@ access(all) contract RedemptionWrapper {
             moet: @MOET.Vault,
             preferredCollateralType: Type?,
             receiver: Capability<&{FungibleToken.Receiver}>
-        ) {
+        ): @MOET.Vault? {
             pre {
                 !RedemptionWrapper.reentrancyGuard: "Reentrancy detected"
                 !RedemptionWrapper.paused: "Redemptions are paused"
@@ -146,7 +151,7 @@ access(all) contract RedemptionWrapper {
             let userAddr = receiver.address
             if let lastTime = RedemptionWrapper.userLastRedemption[userAddr] {
                 assert(
-                    getCurrentBlock().timestamp - lastTime >= RedemptionWrapper.redemptionCooldown,
+                    getCurrentBlock().timestamp - lastTime >= RedemptionWrapper.redemptionCooldownSeconds,
                     message: "Redemption cooldown not elapsed"
                 )
             }
@@ -179,23 +184,20 @@ access(all) contract RedemptionWrapper {
             let sink = position.createSink(type: Type<@MOET.Vault>())
             sink.depositCapacity(from: &moet as auth(FungibleToken.Withdraw) &{FungibleToken.Vault})
             let repaid = moetAmount - moet.balance
-            destroy moet
-
-            // Validate MOET was burned
-            assert(repaid > 0.0, message: "No MOET was repaid/burned")
+            
+            // Validate MOET was repaid
+            assert(repaid > 0.0, message: "No MOET was repaid")
 
             // Determine collateral type (default to FlowToken if not specified)
             let collateralType = preferredCollateralType ?? Type<@FlowToken.Vault>()
             
             // Get oracle price for the collateral
-            // Create a PriceOracle struct instance to access prices
-            let oracle = MockOracle.PriceOracle()
-            let collateralPrice = oracle.price(ofToken: collateralType) 
+            let collateralPriceUSD = RedemptionWrapper.oracle.price(ofToken: collateralType) 
                 ?? panic("Oracle price unavailable for collateral type")
             
             // Calculate exact collateral amount for 1:1 USD parity
-            // MOET is pegged to $1, so: collateralAmount = moetAmount / collateralPriceUSD
-            let collateralAmount = repaid / collateralPrice
+            // MOET is pegged to $1, so: collateralAmount = repaid / collateralPriceUSD
+            let collateralAmount = repaid / collateralPriceUSD
             
             // Validate sufficient collateral is available
             let available = position.availableBalance(type: collateralType, pullFromTopUpSource: false)
@@ -236,27 +238,34 @@ access(all) contract RedemptionWrapper {
                 moetBurned: repaid,
                 collateralType: collateralType.identifier,
                 collateralReceived: actualWithdrawn,
-                oraclePrice: collateralPrice,
+                collateralOraclePrice: collateralPriceUSD,
                 preRedemptionHealth: preHealth,
                 postRedemptionHealth: postHealth
             )
+
+            if moet.balance > 0.0 {
+                return <-moet
+            }
+            destroy moet
+            return nil
         }
     }
 
     /// Setup the redemption position with initial collateral
     /// This must be called once before any redemptions can occur
     /// If called multiple times (e.g., in tests), it will overwrite the previous position
     access(all) fun setup(
+        admin: &Admin,
         initialCollateral: @{FungibleToken.Vault},
         issuanceSink: {DeFiActions.Sink},
         repaymentSource: {DeFiActions.Source}?
     ) {
         // Allow re-setup for testing - clean up previous position if exists
         if self.positionID != nil {
             // Remove old position (structs don't need destroying)
-            self.account.storage.load<FlowALP.Position>(from: self.RedemptionPositionStoragePath)
+            let _ = self.account.storage.load<FlowALP.Position>(from: self.RedemptionPositionStoragePath)
             // Remove old pool cap (capabilities don't need destroying)
-            self.account.storage.load<Capability<auth(FlowALP.EParticipant, FlowALP.EPosition) &FlowALP.Pool>>(from: self.PoolCapStoragePath)
+            let unusedCap = self.account.storage.load<Capability<auth(FlowALP.EParticipant, FlowALP.EPosition) &FlowALP.Pool>>(from: self.PoolCapStoragePath)
         }
         
         let poolCap = self.account.storage.load<Capability<auth(FlowALP.EParticipant, FlowALP.EPosition) &FlowALP.Pool>>(
@@ -318,7 +327,7 @@ access(all) contract RedemptionWrapper {
         self.minRedemptionAmount = 10.0     // Prevent spam
         
         // Rate limiting for MEV protection
-        self.redemptionCooldown = 60.0         // 1 minute cooldown per user
+        self.redemptionCooldownSeconds = 60.0  // 1 minute cooldown per user
         self.dailyRedemptionLimit = 100000.0   // 100k MOET per day circuit breaker
         self.dailyRedemptionUsed = 0.0
         self.lastRedemptionResetDay = UFix64(getCurrentBlock().timestamp) / 86400.0
@@ -327,6 +336,7 @@ access(all) contract RedemptionWrapper {
         // Oracle and health protections
         self.maxPriceAge = 3600.0              // 1 hour max price age
         self.minPostRedemptionHealth = FlowALPMath.toUFix128(1.15)  // Require 115% health after redemption
+        self.oracle = MockOracle.PriceOracle() // Default to MockOracle
         
         // Position tracking
         self.positionID = nil
diff --git a/cadence/tests/redemption_wrapper_test.cdc b/cadence/tests/redemption_wrapper_test.cdc
@@ -216,7 +216,9 @@ fun test_user_cooldown_enforcement() {
     
     // NOTE: Cannot test cooldown expiration without BlockchainHelpers.commitBlock()
     // The cooldown enforcement is validated above - the expiration would require
-    // time advancement which isn't available in the current test framework
+    // time advancement which isn't available in the current test framework.
+    // Attempted to use empty transactions to advance blocks, but Test.executeTransaction
+    // does not advance block height or timestamp in this environment.
 }
 
 /// Test 5: Min/Max Redemption Amounts
@@ -353,7 +355,7 @@ fun test_sequential_redemptions() {
         log("User ".concat(i.toString()).concat(" redeemed. Position health: ").concat(health.toString()))
         
         // Health should remain above minimum (1.15 = 115%)
-        Test.assert(health >= 1.15 as UFix128, message: "Position health below minimum after redemption")
+        Test.assert(health >= 1.15, message: "Position health below minimum after redemption")
         
         i = i + 1
     }
@@ -401,6 +403,69 @@ fun test_liquidation_prevention() {
     }
 }
 
+/// Test 11: Excess MOET Handling
+/// Verifies that if user sends more MOET than debt, the system handles it (accepts surplus)
+access(all)
+fun test_excess_moet_return() {
+    safeReset()
+    
+    // Ensure price is 2.0
+    // Use protocolAccount as signer to match other tests, just in case
+    setMockOraclePrice(signer: protocolAccount, forTokenIdentifier: flowTokenIdentifier, price: 2.0)
+    
+    // Verify price
+    let price = _executeScript("../scripts/mocks/oracle/get_price.cdc", [flowTokenIdentifier]).returnValue! as! UFix64
+    log("Oracle Price: ".concat(price.toString()))
+    Test.assertEqual(2.0, price)
+
+    // Setup redemption wrapper with initial collateral
+    setupMoetVault(protocolAccount, beFailed: false)
+    giveFlowTokens(to: protocolAccount, amount: 1000.0)
+    
+    // Setup redemption position
+    let setupRes = setupRedemptionPosition(signer: protocolAccount, flowAmount: 500.0)
+    Test.expect(setupRes, Test.beSucceeded())
+    
+    // Get initial debt
+    let initialRes = _executeScript("./scripts/redemption/get_position_details.cdc", [])
+    Test.expect(initialRes, Test.beSucceeded())
+    let initialState = initialRes.returnValue! as! {String: UFix64}
+    let initialDebt = initialState["moetDebt"]!
+    log("Initial Debt: ".concat(initialDebt.toString()))
+    
+    // Ensure we have some debt
+    if initialDebt == 0.0 {
+        Test.assert(initialDebt > 0.0, message: "Initial debt must be > 0 for this test")
+    }
+    
+    let user = Test.createAccount()
+    setupMoetVault(user, beFailed: false)
+    
+    // Mint MORE than debt
+    let mintAmount = initialDebt + 50.0
+    mintMoet(signer: flowALPAccount, to: user.address, amount: mintAmount, beFailed: false)
+    
+    // Execute redemption with ALL minted MOET
+    let redeemRes = redeemMoet(user: user, amount: mintAmount)
+    Test.expect(redeemRes, Test.beSucceeded())
+    
+    // Verify user received correct Flow for FULL amount (since sink accepts surplus)
+    // Price is 2.0
+    let userFlowBalance = getBalance(address: user.address, vaultPublicPath: /public/flowTokenBalance) ?? 0.0
+    let expectedFlow = mintAmount / 2.0
+    
+    log("User Flow Balance: ".concat(userFlowBalance.toString()))
+    log("Expected Flow: ".concat(expectedFlow.toString()))
+
+    Test.assert(equalAmounts(a: expectedFlow, b: userFlowBalance, tolerance: 0.00000001), message: "User flow balance mismatch")
+    
+    // Verify user used all MOET (none returned as sink accepted all)
+    let userMoetBalance = getBalance(address: user.address, vaultPublicPath: MOET.VaultPublicPath) ?? 0.0
+    Test.assertEqual(0.0, userMoetBalance)
+    
+    log("User MOET balance: ".concat(userMoetBalance.toString()))
+}
+
 /* --- Helper Functions --- */
 
 access(all)
@@ -449,4 +514,3 @@ fun giveFlowTokens(to: Test.TestAccount, amount: UFix64) {
     // Use the test_helpers function to transfer Flow tokens
     transferFlowTokens(to: to, amount: amount)
 }
-
diff --git a/cadence/tests/transactions/redemption/configure_protections.cdc b/cadence/tests/transactions/redemption/configure_protections.cdc
@@ -8,7 +8,7 @@ transaction(cooldown: UFix64, dailyLimit: UFix64, maxPriceAge: UFix64, minHealth
         ) ?? panic("No admin resource")
         
         adminRef.setProtectionParams(
-            redemptionCooldown: cooldown,
+            redemptionCooldownSeconds: cooldown,
             dailyRedemptionLimit: dailyLimit,
             maxPriceAge: maxPriceAge,
             minPostRedemptionHealth: FlowALPMath.toUFix128(minHealth)
diff --git a/cadence/tests/transactions/redemption/redeem_moet.cdc b/cadence/tests/transactions/redemption/redeem_moet.cdc
@@ -20,11 +20,19 @@ transaction(moetAmount: UFix64) {
             ?? panic("No redeemer capability")
         
         // Execute redemption (uses default collateral type)
-        redeemer.redeem(
+        let change <- redeemer.redeem(
             moet: <-moetVault,
             preferredCollateralType: nil,
             receiver: flowReceiver
         )
+
+        if change != nil {
+             // Deposit change back to user's MOET vault
+             signer.storage.borrow<&MOET.Vault>(from: MOET.VaultStoragePath)!
+                .deposit(from: <-change!)
+        } else {
+            destroy change
+        }
     }
 }
 
diff --git a/cadence/tests/transactions/redemption/setup_redemption_position.cdc b/cadence/tests/transactions/redemption/setup_redemption_position.cdc
@@ -22,8 +22,14 @@ transaction(flowAmount: UFix64) {
             uniqueID: nil
         )
         
+        // Borrow Admin resource
+        let adminRef = signer.storage.borrow<&RedemptionWrapper.Admin>(
+            from: RedemptionWrapper.AdminStoragePath
+        ) ?? panic("No admin resource - setup requires Admin authorization")
+
         // Setup redemption position (no repayment source for testing simplicity)
         RedemptionWrapper.setup(
+            admin: adminRef,
             initialCollateral: <-flowVault,
             issuanceSink: issuanceSink,
             repaymentSource: nil

Original file line number	Diff line number	Diff line change
`@@ -20,11 +20,19 @@ transaction(moetAmount: UFix64) {`
`20`	`20`	`?? panic("No redeemer capability")`
`21`	`21`
`22`	`22`	`// Execute redemption (uses default collateral type)`
`23`		`- redeemer.redeem(`
	`23`	`+ let change <- redeemer.redeem(`
`24`	`24`	`moet: <-moetVault,`
`25`	`25`	`preferredCollateralType: nil,`
`26`	`26`	`receiver: flowReceiver`
`27`	`27`	`)`
	`28`	`+`
	`29`	`+ if change != nil {`
	`30`	`+ // Deposit change back to user's MOET vault`
	`31`	`+ signer.storage.borrow<&MOET.Vault>(from: MOET.VaultStoragePath)!`
	`32`	`+ .deposit(from: <-change!)`
	`33`	`+ } else {`
	`34`	`+ destroy change`
	`35`	`+ }`
`28`	`36`	`}`
`29`	`37`	`}`
`30`	`38`