-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split OrderState
into OrderState
and TradeState
#29
Comments
OrderStatus
into OrderStatus
and TradeStatus
OrderState
into OrderState
and TradeState
Here is an example of where OrderState is used to report the state of a trade. frequenz-api-electricity-trading/proto/frequenz/api/electricity_trading/v1/electricity_trading.proto Line 381 in f5768a1
The following change would need to be applied too: message PublicTrade {
// ID of the trade from the public order book.
uint64 id = 1;
// Delivery area code of the buy side.
frequenz.api.common.v1.grid.DeliveryArea buy_delivery_area = 2;
// Delivery area code of the sell side.
frequenz.api.common.v1.grid.DeliveryArea sell_delivery_area = 3;
// The delivery period for the contract.
frequenz.api.common.v1.grid.DeliveryPeriod delivery_period = 4;
// UTC Timestamp of the last order update or matching.
google.protobuf.Timestamp modification_time = 5;
// The limit price at which the contract is to be traded.
frequenz.api.common.v1.market.Price price = 6;
// The quantity of the contract being traded in MWh.
frequenz.api.common.v1.market.Energy quantity = 7;
// Final state of the trade.
- OrderState state = 8;
+ TradeState state = 8;
} |
I like the idea or separating the state of orders and trades better than the renaming into a single State, as there are some states in orders that don't apply to trades (e.g. pending) |
I also support the separation. About the state fields I am missing background. Is there some documentation related to this? |
Looks good to me as well. Is there anything more to discuss or can we pull that change in? |
Move fast, break things... |
What's needed?
Currently
OrderState
is used to also return the state of a trade. This might be confusing to understand. There are two solutions to the problem. One is to seperate the state of orders and trades or to renameOrderState
to justState
.Proposed solution
Remove the trade statuses from OrderState and create a new TradeState message.
Here is also an updated
OrderStatus
enum:Use cases
No response
Alternatives and workarounds
Rename
OrderState
to justState
Additional context
No response
The text was updated successfully, but these errors were encountered: