Update Pool & Position creation (replaces #6)#10
Conversation
- TidalProtocol: 100% restoration of Dieter's AlpenFlow functionality - Oracle-based pricing and health calculations - Deposit rate limiting and position queues - Advanced health management functions - Async position updates - MOET: Mock stablecoin for multi-token testing - TidalPoolGovernance: Role-based governance system - AlpenFlow_dete_original: Reference implementation Note: Old tests removed as they're incompatible with new contracts. Updated tests coming in follow-up PR. No breaking changes. Foundation for multi-token lending protocol.
sisyphusSmiling
changed the title
7279ff5 to
d49bcc9
Compare
…pulling from topUpSource
…lization rebalancing
|
FYI the behavioral CI tests will fail due to an error in Update: latest DeFiBlocks changes have been updated & the basic position open & rebalance test is passing |
sisyphusSmiling
changed the title
joshuahannan
left a comment
joshuahannan
left a comment
There was a problem hiding this comment.
The general comments in my call still apply. I'll leave some of them here just to have a record.
I spent a lot of time trying to understand the health, interest calculations, and rebalancing business logic, but ended up just giving up because they aren't documented well enough. I mostly just reviewed for Cadence best practices and such.
I wish we could split this up into multiple contracts, reorganize the types so they are introduced, in a more logical order, and add better comments and docs.
I think the error messages in the contract need another pass with including values for clearer messages, such as including token type identifiers in Pool error messages
I also think that putting a global ledger in a single spot (Pool) resource feels like a red flag as a Cadence developer and we should maybe find a way to split it up better. At a bare minimum, the user state, the reserves, the price oracle, and the global ledger should be in different places.
Position should be a resource.
The naming of Pool and Position confuses me because they are singular, but when I look at their definitions, they seem like they should be plural.
Other comments that I can't leave in the code because they aren't changes:
Can health computation be view? Also there are a lot of other functions that should be view. I'm not going to go through an mention all of them here, but I'd recommend just going through and checking everything
Line 108: It feels weird doing contract state changing operations in public struct definitions. Like I know technically there is an entitlement on the TokenState reference that is passed in, but it just feels kind of convoluted and sketchy do be doing it like that.
Interest rates and all the associated calculations feel like they should be in their own contract
Line 255: I'm not sure what the purpose of interestMul is. The comment doesn't explain it well. It would be really nice to just get a whole document that explains interest calculations because I am really struggling to understand any of it from just looking at the code
Line 362: Why does this function exist if it just routes to a different function?
Line 1481: Why does this function exist if there is no limit?
| access(all) struct interface InterestCurve { | ||
| access(all) fun interestRate(creditBalance: UFix64, debitBalance: UFix64): UFix64 | ||
| { | ||
| access(all) fun interestRate(creditBalance: UFix64, debitBalance: UFix64): UFix64 { |
There was a problem hiding this comment.
should this and all the other getter functions for be prefixed with get? Also, can they be view?
|
|
||
| init(id: UInt64, pool: Capability<auth(EPosition) &Pool>) { | ||
| pre { | ||
| pool.check(): "Invalid Pool Capability provided - cannot construct Position" |
There was a problem hiding this comment.
Should this check that the id isn't already in use?
|
@joshuahannan I followed up on your suggestion to better organize the code and expand on code comments in #12 |
…AfterDeposit == 0.0
* update PositionDetails and PositionBalance comments and fields * consolidate public methods to the bottom of the contract * remove comment references to restored functionality * update contract comments * update contract comments and reorganize for clarity * update contract and construct internal methods * update contract comments * revert changes to PositionDetails balances field * update contract comments * Update cadence/contracts/TidalProtocol.cdc Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org> * update interest index & balance type comments --------- Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org>
| /// A simple contract enabling the persistent storage of a Position similar to a pattern expected for platforms | ||
| /// building on top of TidalProtocol's lending protocol | ||
| /// | ||
| access(all) contract MockTidalProtocolConsumer { |
There was a problem hiding this comment.
Why do we need a wrapper to enable persistent storage?
There was a problem hiding this comment.
I suppose we don't. I originally added it thinking we might use Capabilities on it only to later realize they weren't needed and that we can actually issue Capabilities on structs as well as resources.
There was a problem hiding this comment.
I'll note this as part of the post-tracer refactor, so it can be removed in a subsequent PR. Leaving for now to avoid merge conflicts in #19
6 participants
Replaces #6 after base branch changes
Stacked on: #8
Description
AutoBalanceradapter FlowActions#6