DO NOT MERGE: A dummy PR to capture code review comments.#44
DO NOT MERGE: A dummy PR to capture code review comments.#44dete wants to merge 1 commit intodete/review-basefrom
Conversation
| access(all) event Deposited(pid: UInt64, poolUUID: UInt64, type: String, amount: UFix64, depositedUUID: UInt64) | ||
| access(all) event Withdrawn(pid: UInt64, poolUUID: UInt64, type: String, amount: UFix64, withdrawnUUID: UInt64) |
There was a problem hiding this comment.
Can we not use Type objects in events? If clients want strings, they can still get a "stringified" version of the type, but in general, we should use Type objects when possible.
| /// BalanceDirection | ||
| /// | ||
| /// The direction of a given balance | ||
| access(all) enum BalanceDirection: UInt8 { | ||
| /// Denotes that a balance that is withdrawable from the protocol | ||
| access(all) case Credit | ||
| /// Denotes that a balance that is due to the protocol | ||
| access(all) case Debit | ||
| } |
There was a problem hiding this comment.
I find it odd to have these definitions way at the bottom. 😕
| /// to update the scaled balance when the user deposits or withdraws funds. The interest index | ||
| /// is a number relatively close to 1.0, so the scaled balance will be roughly of the same order | ||
| /// of magnitude as the actual balance (thus we can use UFix64 for the scaled balance). | ||
| access(all) var scaledBalance: UInt128 |
There was a problem hiding this comment.
We should look at the possibility of using UFix128 for this. It should be available in time for Forte.
| /// provided TokenState. It's assumed the TokenState and InternalBalance relate to the same token Type, but | ||
| /// since neither struct have values defining the associated token, callers should be sure to make the arguments | ||
| /// do in fact relate to the same token Type. | ||
| access(all) fun recordDeposit(amount: UInt128, tokenState: auth(EImplementation) &TokenState) { |
There was a problem hiding this comment.
Why do we use UInt128 for amount here?
There was a problem hiding this comment.
Overall, it seems like we use UInt128 more than is strictly necessary. I think it would make sense for there to be something somewhere in the code that makes reference to why we are using it all, and to "defend" each place we need it.
For example here, I would like to see why the deposit amount would be a UInt128. All deposits (and withdrawals) are Vaults, and all Vaults use UFix64 to measure their contents. I can't think of any possible circumstances where a value could be deposited that can't be represented as a UFix64.
We definitely need to use a 128-bit type for the interest rate and interest index. Possibly for the scaled balance, and then for any internal computations involving those values. But I would think that everything else can be Fix64 or UFix64.
(To be clear: I'm making two points here. One: It seems like we use 128-bit types more than is necessary. Two: Regardless of how much we use 128-bit types, we should have clear reasons in the code for every variable/computation that uses them.)
| let trueBalance = TidalProtocol.scaledBalanceToTrueBalance(self.scaledBalance, | ||
| interestIndex: tokenState.debitInterestIndex) | ||
|
|
||
| if trueBalance > amount { |
There was a problem hiding this comment.
The equivalent line in recordWithdrawal uses >=. We should either harmonize these or include a comment explaining why they are different. I think it should be >= in both cases.
| withdrawAmount: UFix64 | ||
| ): BalanceSheet { | ||
| var effectiveCollateralAfterWithdrawal = balanceSheet.effectiveCollateral | ||
| var effectiveDebtAfterWithdrawal = balanceSheet.effectiveDebt |
There was a problem hiding this comment.
I would initialize these after the withdrawAmount == 0.0 guard.
| // If the position doesn't have any collateral for the withdrawn token, we can just compute how much | ||
| // additional effective debt the withdrawal will create. | ||
| effectiveDebtAfterWithdrawal = balanceSheet.effectiveDebt + | ||
| DeFiActionsMathUtils.div(DeFiActionsMathUtils.mul(uintWithdrawAmount, uintWithdrawPrice), uintWithdrawBorrowFactor) |
There was a problem hiding this comment.
Maybe factor this into two lines for better readability? First compute a newDebt variable and then sum balanceSheet.effectiveDebt with newDebt.
|
|
||
| // The user has a collateral position in the given token, we need to figure out if this withdrawal | ||
| // will flip over into debt, or just draw down the collateral. | ||
| let collateralBalance = maybeBalance!.scaledBalance |
There was a problem hiding this comment.
I find this awkward, since we only use this variable once, in the very next line. (I realize that this note and the previous one are polar opposites. I guess everyone has their preferences!)
| var effectiveCollateralAfterWithdrawal = effectiveCollateral | ||
| var effectiveDebtAfterWithdrawal = effectiveDebt | ||
|
|
||
| log(" [CONTRACT] effectiveCollateralAfterWithdrawal: \(effectiveCollateralAfterWithdrawal)") | ||
| log(" [CONTRACT] effectiveDebtAfterWithdrawal: \(effectiveDebtAfterWithdrawal)") | ||
|
|
||
| // We now have new effective collateral and debt values that reflect the proposed withdrawal (if any!) | ||
| // Now we can figure out how many of the given token would need to be deposited to bring the position | ||
| // to the target health value. | ||
| var healthAfterWithdrawal = TidalProtocol.healthComputation( | ||
| effectiveCollateral: effectiveCollateralAfterWithdrawal, | ||
| effectiveDebt: effectiveDebtAfterWithdrawal | ||
| ) | ||
| log(" [CONTRACT] healthAfterWithdrawal: \(healthAfterWithdrawal)") | ||
|
|
||
| if healthAfterWithdrawal >= targetHealth { | ||
| // The position is already at or above the target health, so we don't need to deposit anything. | ||
| return 0.0 | ||
| } |
There was a problem hiding this comment.
Seems like more vestigial aspects to this code post-refactor. At the very least, I think these variable names can change (there is no "withdrawal" in this version of the function).
| // We need to increase the effective collateral from its current value to the required value, so we | ||
| // multiply the required health change by the effective debt, and turn that into a token amount. | ||
| let uintHealthChange = targetHealth - healthAfterWithdrawal | ||
| // TODO: apply the same logic as below to the early return blocks above |
There was a problem hiding this comment.
We should address or remove this TODO.
dete
left a comment
dete
left a comment
There was a problem hiding this comment.
Another round of reviews. More yet to come!
| if depositAmount == 0.0 { | ||
| return BalanceSheet(effectiveCollateral: effectiveCollateralAfterDeposit, effectiveDebt: effectiveDebtAfterDeposit) | ||
| } |
There was a problem hiding this comment.
I think this guard would be better at the top of the function.
| let uintDepositBorrowFactor = DeFiActionsMathUtils.toUInt128(self.borrowFactor[depositType]!) | ||
| let uintDepositCollateralFactor = DeFiActionsMathUtils.toUInt128(self.collateralFactor[depositType]!) | ||
| let maybeBalance = position.balances[depositType] | ||
| if maybeBalance == nil || maybeBalance!.direction == BalanceDirection.Credit { |
| depositType: Type, | ||
| depositAmount: UFix64 | ||
| ): BalanceSheet { | ||
| var effectiveCollateralAfterDeposit = balanceSheet.effectiveCollateral |
There was a problem hiding this comment.
Suggested comment: // Initialize with the current collateral and debt values
| if trueDebt >= uintDepositAmount { | ||
| // This deposit will pay down some debt, but won't result in net collateral, we | ||
| // just need to account for the debt decrease. | ||
| // TODO - validate if this should deal with withdrawType or depositType |
There was a problem hiding this comment.
Address or remove TODO.
| } else { | ||
| // The deposit will wipe out all of the debt, and create some collateral. | ||
| // TODO - validate if this should deal with withdrawType or depositType | ||
| effectiveDebtAfterDeposit = balanceSheet.effectiveDebt - |
There was a problem hiding this comment.
Another leftover TODO.
Also, we might want a bit more detail in this comment (which would apply to the withdraw case above, as well).
| log(" [CONTRACT] Requested amount: \(amount)") | ||
| log(" [CONTRACT] Available balance (without topUp): \(availableBalance)") | ||
| log(" [CONTRACT] Required deposit for minHealth: \(requiredDeposit)") | ||
| log(" [CONTRACT] Pull from topUpSource: \(pullFromTopUpSource)") |
There was a problem hiding this comment.
Are we expecting to remove all of this logging when we go to production? It seems like a lot (not just this part, but the whole contract).
| // Reflect the withdrawal in the position's balance | ||
| let uintAmount = DeFiActionsMathUtils.toUInt128(amount) | ||
| position.balances[type]!.recordWithdrawal(amount: uintAmount, tokenState: tokenState) | ||
| if self.positionHealth(pid: pid) != 0 { |
There was a problem hiding this comment.
This guard is suspicious. Why would the health be zero? And if it is, wouldn't we want to abort in that case?
There was a problem hiding this comment.
Maybe we're thinking about the case where the position is now empty? If so, we should add a comment to that effect… (But I thought an empty account has a health value of 1.0.)
| let withdrawn <- reserveVault.withdraw(amount: amount) | ||
|
|
||
| emit Withdrawn(pid: pid, poolUUID: self.uuid, type: type.identifier, amount: withdrawn.balance, withdrawnUUID: withdrawn.uuid) | ||
|
|
||
| return <- withdrawn |
There was a problem hiding this comment.
It doesn't matter when we emit events. They are just logged for later. This can just be two lines of code...
|
|
||
| /// Sets the InternalPosition's drawDownSink. If `nil`, the Pool will not be able to push overflown value when | ||
| /// the position exceeds its maximum health. Note, if a non-nil value is provided, the Sink MUST accept the | ||
| /// Pool's default deposits or the operation will revert. |
There was a problem hiding this comment.
Really? I feel like we should allow the sink to reject deposits... 🤔
There was a problem hiding this comment.
Looking at the code that pushes into the sink, I don't think it does revert if the sink rejects the deposit...
| access(all) fun getTargetHealth(): UFix64 { | ||
| return 0.0 // TODO | ||
| } | ||
| /// Sets the target health of the Position | ||
| access(all) fun setTargetHealth(targetHealth: UFix64) { | ||
| // TODO | ||
| } | ||
| /// Returns the minimum health of the Position | ||
| access(all) fun getMinHealth(): UFix64 { | ||
| return 0.0 // TODO | ||
| } | ||
| /// Sets the minimum health of the Position | ||
| access(all) fun setMinHealth(minHealth: UFix64) { | ||
| // TODO | ||
| } | ||
| /// Returns the maximum health of the Position | ||
| access(all) fun getMaxHealth(): UFix64 { | ||
| // TODO | ||
| return 0.0 | ||
| } | ||
| /// Sets the maximum health of the position | ||
| access(all) fun setMaxHealth(maxHealth: UFix64) { | ||
| // TODO | ||
| } |
There was a problem hiding this comment.
These should be implemented or removed.
|
Cross-reference: Follow-up implementation PR opened at #46. It addresses a large set of review items captured here (see PR_REVIEW_TRIAGE_dete.md in the repo for mapping), including:
Deferred for a subsequent PR: numeric type refinements (UFix64 at boundaries, potential UFix128), deeper dedup of health computations, and optional logging/config standardization across liquidation flows. Liquidation work in #41/#43 complements this by adding liquidation params/bonus and execution paths; we’ll harmonize logging and (optionally) event type fields post-merge. |
|
Closing this in preparation for open sourcing. The comments have either been addressed or noted for future improvements, as stated by the comment above |
3 participants
This PR should not be merged, but it allows us to use the GH code commenting and discussion system so I can provide a comprehensive review of the TidalProtocol.cdc file.