[pallet-broker] decouple coretime auction logic from the pallet #10916
Conversation
| /// market itself) | ||
| /// - `state` - market state, the caller is responsible for storing it | ||
| fn place_order( | ||
| since_timeslice_start: BlockNumber, |
There was a problem hiding this comment.
What block number is this? Relay chain or local coretime chain?
Maybe its overkill, but one way to do this would be to create some MarketTime (or whatever name) that uses the same RelayChainBlockNumberProvider as the CoretimeInterface and the TimeslicePeriod and calculates this instead of forcing the caller to do it correctly.
There was a problem hiding this comment.
It's a relay chain block as both old and new implementations depend on it, so I guess if that would need abstraction in the future the appropriate adjustments can be made
There was a problem hiding this comment.
Then maybe change the typename to RelayBlockNumber, its quite easy to confuse 😆
| /// This method may or may not create a bid, according to the market rules. | ||
| /// | ||
| /// - `since_timeslice_start` - amount of blocks passed since the current timeslice start | ||
| /// - `amount` - maximum price which the buyer is willing to pay (or None if it's defined by the |
There was a problem hiding this comment.
DQ but how much coretime do they buy for this amount? Like 1 region, or 2 or 10?
There was a problem hiding this comment.
Do you mean to specify that only one region is bought explicitly in the function name or to add a parameter specifying how much regions we want to buy?
| state: &mut Self::State, | ||
| ) -> Result<(), Self::Error>; | ||
|
|
||
| /// Logic that gets called in `on_initialize` hook. |
There was a problem hiding this comment.
Or the coretime chain? Please describe how often it must be called (otherwise Polkadot will stop working) and how often it should be called (Polkadot keeps working but the market may be a bit slow).
This is important to distinguish since on_initialize should only be used for protocol level security relevant execution. Everything else can go into on_idle or on_poll to reduce risk of stalling the chain when there is an issue with the code that is being run.
There was a problem hiding this comment.
Actually it can be rephrased to Logic that gets called periodically because any assumptions about the consequences of calling it not in the on_initialize cannot be made without looking into the implementation
| /// Internal market state that must be preserved between the method calls. If the market logic | ||
| /// allows creating bids they should be stored there as well as the bid structure depends on the | ||
| /// market implementation. | ||
| type State; |
There was a problem hiding this comment.
What would you guess would be in this state? Just a vector or some more complicated struct?
I am asking to figure out the storage layout that you could use. Normally we use the FRAME primitives like StorageMap and StorageValue, but these are imperative - not functional - like here.
So if we pass around a big struct and then write it to a StorageValue it could be suboptimal since it has to read and write that big struct every time even just for small changes.
Alternatively you could use a State interface with functions like insert_order or get_order, or make the interface imperative without State being passed around. But I get that having it functional is cleaner.
There was a problem hiding this comment.
Discussed in DM, the conclusion is that it can be removed and the pallet_broker may implement Market trait instead of exposing it in the config. In this setup the market functions can directly read/write the pallet storage
Part of #10900
Introduce the
Markettrait and expose it to allow the pallet users to configure the market mechanism by substituting the implementation of this trait. This PR is done as a part of RFC-17 implementation and the refactoring done here will be helpful to update the logic according to the RFC-17.