Conversation
| // open wrapped position (pushToDrawDownSink) | ||
| let openRes = executeTransaction( | ||
| "../transactions/flow-credit-market/position/create_position.cdc", | ||
| [minimum-0.1, FLOW_VAULT_STORAGE_PATH, true], |
There was a problem hiding this comment.
maybe continue to verify the same tx with minimum amount would pass
There was a problem hiding this comment.
i do now continue with a successful create position
| // Ensure that the remaining balance meets the minimum requirement (or is zero) | ||
| let balanceRecord = position.balances[type]! | ||
| let interestIndex = balanceRecord.direction == BalanceDirection.Credit | ||
| ? tokenState.creditInterestIndex | ||
| : tokenState.debitInterestIndex | ||
| let remainingBalance = FlowCreditMarket.scaledBalanceToTrueBalance( | ||
| balanceRecord.scaledBalance, | ||
| interestIndex: interestIndex | ||
| ) |
There was a problem hiding this comment.
There's already a function PositionView.trueBalance which pretty much does this. Maybe reuse it
There was a problem hiding this comment.
the main argument against using positionView here was that it technically copies all balances, to create the new view. With that said, a position is unlikely to have too many different balances, so maybe that won't really be an "impactful" optimization. Will change implementation, but make a note
| let minimumRequired = UFix128(tokenState.minimumTokenBalancePerPosition) | ||
|
|
||
| assert( | ||
| remainingBalance == 0.0 || remainingBalance >= minimumRequired, | ||
| message: "Withdrawal would leave position below minimum balance requirement of \(minimumRequired). Remaining balance would be \(remainingBalance)." | ||
| ) |
There was a problem hiding this comment.
| let minimumRequired = UFix128(tokenState.minimumTokenBalancePerPosition) | |
| assert( | |
| remainingBalance == 0.0 || remainingBalance >= minimumRequired, | |
| message: "Withdrawal would leave position below minimum balance requirement of \(minimumRequired). Remaining balance would be \(remainingBalance)." | |
| ) | |
| assert( | |
| positionSatisfiesMinimumCreditBalance(type, remainingBalance) | |
| message: "Withdrawal would leave position below minimum balance requirement of \(minimumRequired). Remaining balance would be \(remainingBalance)." | |
| ) |
There was a problem hiding this comment.
added, though i did have to do some type conversions. I think still worth though.
| // Amount should now be exactly the minimum, so withdrawal should fail | ||
| let withdrawResFail = _executeTransaction( | ||
| "./transactions/position-manager/withdraw_from_position.cdc", | ||
| [positionId, FLOW_TOKEN_IDENTIFIER, amountAboveMin, true], |
There was a problem hiding this comment.
| [positionId, FLOW_TOKEN_IDENTIFIER, amountAboveMin, true], | |
| [positionId, FLOW_TOKEN_IDENTIFIER, minimum-0.1, true], |
To make sure we are withdrawing a amount smaller than current balance, independent of amountAboveMin
There was a problem hiding this comment.
will adjust, but going to just div by 2
| 1. **Set minimums based on token economics**: Higher-value tokens may need lower minimums in token units (e.g., 0.1 ETH) while lower-value tokens need higher minimums (e.g., 100 USDC) | ||
| 2. **Consider gas/transaction costs**: Minimum should be high enough that position operations are economically justified |
There was a problem hiding this comment.
Higher-value tokens may need lower minimums in token units
They just do need lower minimums, there is no may. :)
I think what we want to say is:
- Conceptually the minimum balance should reflect a minimum balance value, denominated in the default token ($ / MOET). We have implemented this as a per-token balance, denominated in the token itself, because it is easier to implement and reason about.
- Possibly there are per-token reasons why the minim value should be higher or lower (like volatility), but the dominating factor is that we need the dollar value of the balance to exceed some threshold
And governance should:
- Pick a dollar-denominated minimum balance
- Set each token minimum so that it equates to the dollar-denominated minimum balance, maybe with an added buffer for expected volatility
- Periodically update the token minimums to match the dollar-denominated minimum (this could be automated)
There was a problem hiding this comment.
did another pass with you suggestions
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
| /// @param balance: The balance amount to validate | ||
| /// @return true if the balance meets or exceeds the minimum requirement, false otherwise | ||
| access(self) view fun positionSatisfiesMinimumBalance(type: Type, balance: UFix128): Bool { | ||
| return balance >= UFix128(self.globalLedger[type]!.minimumTokenBalancePerPosition) |
There was a problem hiding this comment.
I have a slight preference toward including the empty balance case here, so that this validator function captures the full range of valid balances.
return balance == 0.0 || balance >= UFix128(self.globalLedger[type]!.minimumTokenBalancePerPosition)Then we can simplify the assertion on line 3014:
assert(self.positionSatisfiesMinimumBalance(type: type, balance: remainingBalance), ...)There was a problem hiding this comment.
my main problem with doing that was that technically, that would make this function "less" useful for the create position case. Since for that case, the balance == 0.0 is not a "true" case. Also, if i was to include the balance check, i'd probably also change the name of the function, as 0.0 obviously does NOT satisfy the minimum balance.
I think i'd still prefer to keep the balance == 0.0 check outside this function as it only applies in one of the locations where this function is used.
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com> Co-authored-by: Alex <12097569+nialexsan@users.noreply.github.com>
Closes: #143
Description
Re-enforces a minimum balance on a position.
For contributor use:
masterbranchFiles changedin the Github PR explorer