New Architecture (workflow)

[antoncoding]

TLDR for new Architecture / workflow

Current System

Since we introduced different "margin engines" into the system, the contract flow now becomes a little bit messy. The current workflow is:

1. User interact with Grappa as the entry point, it then delegates call to the corresponding engine.
2. Each MarginEngine contract need to implement corresponding logic to deal with adding collateral, minting option ...etc. Grappa will loop through an array of actions and call the engine to update the "account" correspondingly.
3. Grappa is the only contract authorised to mints option token
4. Each MarginEngine take care of "pulling collateral"
5. Grappa can be used to batch settle options from different engine, but also need to each MarginEngine to authorised Grappa for moving the collateral.

The problem

1. un-determined workflow: each margin engine can theoretically still manage collateral out of the control of Grappa, making "add collateral through Grappa" unnecessary and seem redundant
2. the current scheme will update the "state" in a margin engine multiple times in a single transaction, if a user want to batch multiple actions in a single transactions, costing a lot more gas.
3. hard to design an interface good enough for all kind of margin engines (especially around liquidation). we will probably ended up being very similar to passing bytes and let each engine decode.

Solution

One solution is to reverse the flow, let each margin engine become the entry point and listen to a series of user actions.
The main grappa contract will become a registry for assets, oracles and engines, possibly also handle "access management".

When user need to deposit / remove collateral, they only need to interact with each margin engine.
When it comes to minting and burning, the engines can directly call the OptionToken contract. OptionToken will decide if each engine is authorised to mint a certain type of option.

This will simplify the relationship between contracts by quite a lot, and add a lot of complexity to each margin engine. (almost all of the logic)

Concerns

1. One advantage of using Grappa as a single entry point is that, in the future we can easily let user pass in different engineId and switch engine, which might be better in terms of liquidation. But this can also be achieved by having another aggregator which takes in engine id to call different address, or interact with multiple engine in the same transaction.
2. Another big advantage of the current structure is that, we can enforce everyone to have the same user facing interface. This can be both good and bad: we ensure in the future it's easier for integration because of the identical interface, but we loss the flexibility to do more fancy stuff. Note: Even though some kind of "interface" is enforced at the contract level, we cannot guaranteed each engine do things properly (won't revert) without looking at the code, so the enforced interface doesn't really guarantee much about each engine's behaviour.
   • I think as long as we have the registration process, making an "engine" comply with a single interface before adding it to the system should also be consider mandatory, so the trust assumption is the same, we can also enforce a very generic interface that all engines have to comply with, and not add one the Grappa until it's tested to comply.

Solution is implemented in #108

[antoncoding]

Old Flow of minting:

New flow of minting

[dsshap]

I generally like this updated approach. After reviewing the code it feels like the more natural approach.

Concerns

1. doesn't seem important. Even from a liquidation perspective i have a hard time justifying the added functionality.
2. Is applicable, but this could be achieved by making it a requirement that every registered engine inherit from BaseEngine and BaseEngine implements IMarginEngine. These inherited methods can be virtual allowing the the inheriting engine to override functionality for their own purposes.

Some ideas:

1. There are a number of common methods (and implementations) that currently exist within FullMargin and AdvancedMargin Engines. I suggest consolidating them into BaseEngine. Some examples: execute, payCashValue, transferAccount.
2. All interfaces are currently stored in the /interfaces directory. Some of these interfaces are Engine specific (such as IVolOracle -> AdvanceMarginEngine). It could be helpful to more clearly differentiate which interfaces are used at the protocol level and what is engine specific. This can be either in a subfolder of /interfaces or a folder within /engines. There might be some instances where one engine will implement an interface used in another engine, having duplicates in the repo seems like a justifiable trade off for organization.

Observations:
I noticed that you recreate errors based on which Engine they are thrown in. How much value does this bring to the Account? The Account already knows which engine it is transacting with, does it really need the error to be prefixed with the Engines abbreviation?

[antoncoding]

re 1) that what i was doing in #113 too!
So i now let BaseEngine implement the execute function, and use virtual functions like _addCollatearlInAccount for data-structure specific updates. So each engine only has to implement how to update the data structure, or revert is an action is not supported.

[antoncoding]

re 2)
I agree with some more folder separation. I'd still prefer using some global interfaces to reduce duplication, but agree that bring thing like IVolOracle to engine level would make the structure better. I opened #115 for this.

What I was thinking back and forth is should we put struct and errors in different folders too. (base on the same argument about interfaces), would love to hear your thoughts too.

[antoncoding]

re prefix about errors, I did it primarily for easier debug purpose, so errors prefixed with BM_ is thrown in the BaseEngine contract, would be easier for people to find what is reverting while writing tests.
Using different prefix for Advanced Margin and Full Margin is simply for following the same rule. I know that right now they have a lot of common errors that seems duplicated, but if we prefix them with the same Account prefix, it would implied other engines also need to have this behaviour. (e.g. not supporting certain action).