Add Strategy composition & AutoBalancer management contracts#7
Conversation
…outside of the struct
|
@joshuahannan good call. I've reverted back to |
|
@nialexsan looks like CI is failing due to missing token in the repo secrets. I think you added one to TidalProtocol repo, could you add one to this repo as well please? |
joshuahannan
left a comment
joshuahannan
left a comment
There was a problem hiding this comment.
Maybe all the strategy stuff and the Tide stuff should be in separate contracts? All the different layers of strategy composer, factory, issuer, etc is kind of confusing and they are still somewhat separate from the Tide stuff, right? Up to you
| /// | ||
| access(all) resource StrategyFactory { | ||
| /// A mapping of StrategyComposers indexed on the related Strategies they can compose | ||
| access(self) let composers: @{Type: {StrategyComposer}} |
There was a problem hiding this comment.
So is this implying that a given strategy can only be created by one type of Strategy Composer? What if someone wants to have multiple strategy composers that share a strategy type?
There was a problem hiding this comment.
The short answer is that yes, it's implied that a Strategy is composed by a single Composer. The same Strategy could be created by another Composer, but it would require that the new Composer replace the old one. Ultimately, the composition of a Strategy is a Strategy conceptually and if there's any meaningful difference in how it's composed (in that funds flow differently), then the two should be different Strategy definitions.
This question points to something I think might be a bit of a code smell with this design - that two distinct Strategy instances don't differ much in fields or functionality and that their composition is identified by the Type.
The distinct Type is instead used to distinguish how those instances are composed as that's the real value of a conceptual strategy. That's largely the reason I restrict the initial TracerStrategy creation through the TracerStrategyComposer and further through the TracerStrategyComposerIssuer.
The generalized constraints here are that we need to build something (a Strategy) with a common interface such that two different instances can be captured and used from within a Tide. Two instances of that thing may largely look similar in fields and functions, but how they are constructed (i.e. the sequencing of DeFiBlocks connectors during the Strategy ultimately provides top-level access to) can vary greatly. So how do we:
- Convey meaning around how these are constructed as that's ultimately the differentiator
- Protect construction of these things since some may utilize resources (e.g. account storage)
- Maintain common interfaces such that they can be captured and used in a
Tide - Minimize the amount of configurations needed to initialize a
Strategygiven the design space between them
My thinking then is that we put that meaning in the Type definition. Then as more strategies are added to the platform their composition logic can be defined in a StrategyComposer and a request for the composition be identified by a new Strategy Type.
Do you have any feedback on that approach?
There was a problem hiding this comment.
I think we need to try to simplify this somehow. I think there are too many layers and it is getting way too hard to keep everything straight. I'm really worried that actually managing something like this is going to be prohibitively complicated for any given wallet or app. We have strategy, strategy composer, strategy composer issuer, strategy factory, and more and that is just this contract alone. it doesn't even include all the other defi blocks components and tide protocol stuff. I am really struggling to understand everything and see why all these are necessary.
There was a problem hiding this comment.
Strategy composer, strategy composer issuer, and strategy factory all sound like they could be simplified to one construct. If I am having such a hard time with this, I can't imagine how any external developers or wallets are going to be willing to deal with this kind of complexity unless they are making a ton of money off it
There was a problem hiding this comment.
While i do agree on the sentiment, i think that's likely more of a closed beta/productionizing task.
I've generated an Issue for it here: #9
I agree @joshuahannan, I think that makes things cleaner - maybe in a contract like I'll follow up with another PR to include those changes as I don't want to introduce minor changes that would for a redeployment while @nialexsan is setting up the initial MVP. |
549cf50 to
051302c
Compare
joshuahannan
left a comment
joshuahannan
left a comment
There was a problem hiding this comment.
I'm still not satisfied with the structure of the code, but I'll approve this for now since you need it for the prototype and we can revisit my comments later
| } | ||
|
|
||
| /// Returns an authorized reference on the AutoBalancer with the associated UniqueIdentifier.id. If none is found, | ||
| /// the operation reverts. |
There was a problem hiding this comment.
Should it just return nil instead?
There was a problem hiding this comment.
It could, though it would revert where called since it's required for write operations
| ) | ||
| // get Sink & Source connectors relating to the new Position | ||
| let positionSink = position.createSinkWithOptions(type: collateralType, pushToDrawDownSink: true) | ||
| let positionSource = position.createSourceWithOptions(type: collateralType, pullFromTopUpSource: true) // TODO: may need to be false |
…ral (#11) * Make adjustments to allow closing the tide and retain the full collateral * Add reserves to tests to allow closing tide after yield value increase
|
Merging to main with the expectation we refactor after Tracer retrospective. |
4 participants
Description