zeitgeist icon indicating copy to clipboard operation
zeitgeist copied to clipboard

Make structs more object-oriented

Open maltekliemann opened this issue 2 years ago • 2 comments

Currently, Pool is defined as follows:

#[derive(TypeInfo, Clone, Encode, Eq, Decode, PartialEq, RuntimeDebug)]
pub struct Pool<Balance, MarketId>
where
    MarketId: MaxEncodedLen,
{
    pub assets: Vec<Asset<MarketId>>,
    pub base_asset: Option<Asset<MarketId>>,
    pub market_id: MarketId,
    pub pool_status: PoolStatus,
    pub scoring_rule: ScoringRule,
    pub swap_fee: Option<Balance>,
    pub total_subsidy: Option<Balance>,
    pub total_weight: Option<u128>,
    pub weights: Option<BTreeMap<Asset<MarketId>, u128>>,
}

Note that invariants like scoring_rule are public. Scoring rules should not change throughout the life-time of a pool.

Furthermore, swaps is full of checks like this:

if pool.scoring_rule == ScoringRule::CPMM {
    // -- snip --
} else {
    // -- snip --
}

I would say that these should be handled using a trait which each scoring rule implements.

Another example: assets is modified when the market resolved (losing assets are removed). Instead, Pool should implement a function which handles the effect of market resolution on the pool and deletes losing assets.

maltekliemann avatar Mar 30 '22 15:03 maltekliemann

Another example: The Market struct contains a MarketDisputeMechanism, which is an enum, but each member of the enum basically refers to one implementation of DisputeApi. When that implementation is to be used, our code contains these match expressions:

match market.mdm {
    MarketDisputeMechanism::Authorized(_) => {
        T::Authorized::on_dispute(&disputes, &market_id, &market)?
    }
    MarketDisputeMechanism::Court => {
        T::Court::on_dispute(&disputes, &market_id, &market)?
    }
    MarketDisputeMechanism::SimpleDisputes => {
        T::SimpleDisputes::on_dispute(&disputes, &market_id, &market)?
    }
}

Instead, can't we just let mdm implement DisputeApi and just call mdm.on_dispute? At the very least, a dispatcher should handle the association with the members of the MarketDisputeMechanism enum and the implementations of DisputeApi.

maltekliemann avatar Mar 31 '22 12:03 maltekliemann

I think this is a long-running issue. Leaving this open. Writing that dispatcher is a fun exercise in rust macros.

maltekliemann avatar Oct 24 '23 23:10 maltekliemann

Too vague to ever be completed.

maltekliemann avatar May 03 '24 15:05 maltekliemann