Add market ID to Market struct

[maltekliemann]

What does it do?

Adds market ID to Market struct. To safely handle writing the market ID, we use a MarketBuilder trait. This may look like a bit much, but is exactly the right pattern here.

I've added build_market to the market-commons interface to replace push_market. The latter will stick around so that I can avoid rewriting the tests of all other pallets depending on push_market. But it is deprecated and pallet maintainers are encouraged to get rid of it!

What important points should reviewers know?

We're doing this in order to be able to implement Market::asset(&self), which requires access to the market ID.

Is there something left for follow-up PRs?

• Implement the migration.
• Implement Market::asset.
• Remove edit_market; move validation logic into PredictionMarketBuilder.

What alternative implementations were considered?

We considered the following alternatives:

• Implement assets(&self) for (MarketId, Market) instead of Market.
• Implement assets as a free function.
• Remove edit_market and move validation logic to MarketBuilder::build(). We're currently not doing this because the validation is a bit iffy with edit_market still in the mix.

We settled on this particular option since it seemed to be safest. The only concern is that it's possible to add a Market instance market to the storage map Markets with key market_id so that market_id != market.market_id. However, this is not the case if the market is added using build_market.

The whole MarketBuilder trait is only used to avoid a user being able to mess with a market's ID. We could have gone with market_id: Some(MarketId) as field in the Market struct, but that would have meant that need more storage per market for no reason other than that we're too lazy to implement the builder struct.

Are there relevant PRs or issues?

References

[codecov-commenter]

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (8ee0d1a) 92.83% compared to head (5e403c6) 92.91%.
Report is 1 commits behind head on mkl-market-id-feature.

Files	Patch %	Lines
primitives/src/types/xcm_metadata.rs	0.00%	29 Missing ⚠️
primitives/src/types/primitive_market_builder.rs	86.66%	4 Missing ⚠️
primitives/src/market.rs	33.33%	2 Missing ⚠️
primitives/src/types/result_with_weight_info.rs	0.00%	2 Missing ⚠️
zrml/prediction-markets/src/lib.rs	95.55%	2 Missing ⚠️
zrml/authorized/src/mock.rs	0.00%	1 Missing ⚠️
zrml/court/src/mock.rs	0.00%	1 Missing ⚠️
zrml/global-disputes/src/mock.rs	0.00%	1 Missing ⚠️
zrml/simple-disputes/src/mock.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                    Coverage Diff                    @@
##           mkl-market-id-feature    #1248      +/-   ##
=========================================================
+ Coverage                  92.83%   92.91%   +0.08%     
=========================================================
  Files                        154      158       +4     
  Lines                      31321    31444     +123     
=========================================================
+ Hits                       29076    29216     +140     
+ Misses                      2245     2228      -17     

Flag	Coverage Δ
tests	92.91% <85.17%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Chralt98]

Why do we have a builder pattern, if only the complete market (with all fields) can be built? Shouldn't we rather provide an update function for all the fields instead of having the possibility of initialising with an error?

[maltekliemann]

Ignoring edit_market, which is on its way out, we actually wouldn't be using the update function. The whole reason why I'm using the builder pattern here is to ensure that part of the data can be set in prediction-markets, the builder then passed to market-commons, where the market ID is set. This means that we have to allow some of the fields to be incomplete (at least for a while).

[Chralt98]

I would avoid all this confusion of the tangle of Option and maintainer error possibility by using this approach.

[Chralt98]

I would simply avoid the builder pattern by peeking the next market id, then construct the market structure and then push the market.

[Chralt98]

Why don't we even decouple market id creation from push_market?

[Chralt98]

So basically just throw let market_id = Self::next_market_id()?; out of push_market and use it directly.

[Chralt98]

Again, builder pattern makes total sense, if you just care about a few fields for example in mocks, benchmarking and tests to get to a working structure quick and not having the full structure initialisation and associated maintenance if the underlying structure changes all the time.

[Chralt98]

I also generally seem to derive a sense of a will of you to put all sorts of "clean code" patterns in the business logic without taking a step back to reconsider if those patterns are really appropriate for this specific scenario and couldn't be done more simple. I don't encourage anti-patterns either. I am just open to avoid "clean code" patterns without introducing "anti-patterns" for the sake of simplicity. In your specific case, it's even preventing a maintenance error named IncompleteMarketBuilder.

[maltekliemann]

Why don't we even decouple market id creation from push_market?

Because they belong together. Placing the Market object in storage is the responsibility of market-commons. Part of that is ensuring that 1) the correct next market ID is used; 2) (this is new) the storage key matches the market_id field of the market.

If we let other modules set the market ID, we take that responsiblity away from market-commons and place it in the hands of the callers. This risks that they put the wrong market ID into the market struct, overwrite old markets or skip market IDs. This is just basic encapsulation.

To maintain encapsulation, we need to be able to pass the market creation parameters to market-commons without setting the market_id field. The builder pattern seemed appropriate to me.

Moreover, as I've pointed out in the description, the builder pattern will make even more sense when we get rid of edit_market and can move valdiation functionality into the trait implementation. This allows us to move validation out of extrinsics and free functions of a bloated lib.rs into separate objects.

And once edit_market is done for, we can even remove certain setters. For example, why would we have a setter for the early_close field? Clearly, a newly created market should never have an early close scheduled, right? These will be removed as soon as edit_market is gone. This further encapsulates the market creation.

I also generally seem to derive a sense of a will of you to put all sorts of "clean code" patterns in the business logic without taking a step back to reconsider if those patterns are really appropriate for this specific scenario and couldn't be done more simple.

I'm indeed very concerned with increasing the code quality of our code base. As for "[not] taking a step back", I'm surprised I'm making that impression. Not sure what to tell you except that I always weigh the available options very carefully.

[Chralt98]

I understand your point about encapsulation. My approach wasn't meant to be the best either ;-), but a first attempt. Yeah, this wasn't in my mind, but I still don't like the ImcompleteMarketBuilder error either. And the fact that we have a "complex" market builder because of one field for this huge struct. Another option would be to have a function with a lot of parameters for all market fields or a PartialMarket struct passed to this function.

Moreover, as I've pointed out in the description, the builder pattern will make even more sense when we get rid of edit_market and can move valdiation functionality into the trait implementation. This allows us to move validation out of extrinsics and free functions of a bloated lib.rs into separate objects.

This sounds interesting, I like the idea a lot. Why not using a function check_market(market) and validate everything inside it? Then we could also get rid of the bloated lib.rs. I mean the logic of validation needs to sit somewhere anyways.

And once edit_market is done for, we can even remove certain setters. For example, why would we have a setter for the early_close field? Clearly, a newly created market should never have an early close scheduled, right? These will be removed as soon as edit_market is gone. This further encapsulates the market creation.

This is a good point. However for market creation, you could just use a PartialMarket without an early close field and assign None to it inside the market-commons pallet. In that way the market's early close hasn't to be assigned in the prediction markets pallet either.

As for "[not] taking a step back", I'm surprised I'm making that impression.

That's why I said seem. It's very subtle and I wanted to alert about things early to improve overall readability and wanted to challenge assumptions. At the end, I try (also fail a lot ^^) to make you even better ;-) nothing else. It was never about me offending your code style. I just want to find out the truth about the best possible solution.

Not sure what to tell you except that I always weigh the available options very carefully.

❤️

[sea212]

The whole purpose of a builder pattern is to be capable to create different representations of the same object in a clean and structured manner. So my question is, is there any foreseen utilization of that feature? Will the Market object ever be created with different fields set or not set? If the answer is yes, then this is the way to go. If the answer is no, then simply create a constructor for Market that is invoked in market-commons within a function that resembles the constructor but does not contain the market_id (as it is derived within that function).

I am not quite sure how updating markets is planned to work in the future, but I think in any case it makes sense to apply a good design pattern and make the fields of Market private while providing getters for all fields and setters for fields that can change during the lifetime of a market.

[maltekliemann]

That's not the only purpose of the builder struct. Here, the purpose is to allow the Market object to be created piece-by-piece across multiple functions. This is the case here. It also allows placing the validation logic somewhere outside of the prediction-marktes pallet.

I think I've made my point here and in the long discussion above. Could somebody else please make a decision so we can get this out the door?

[sea212]

I see, if that's the requirement than this is the correct design choice. Move forward.