Improve treatment of divisible assets

[alexdewar]

There are a number of possible improvements, so I've grouped them into an umbrella issue.

[alexdewar]

I've been thinking about how we might want the data structures for this to look a bit today. It makes sense to do #1041, before moving on to anything else as that'll make the rest of this easier.

These are all just suggestions, so thoughts welcome!

We probably eventually want to have an extra field in Asset indicating the number of assets the object represents (it may only apply to some states, so we might want to have it inside AssetState, like we do for some other values). We could set this to one for non-divisible assets, but I suspect we might want to distinguish these from divisible assets with a count of one, so it might be better to make it an Option<u32> (or Option<NonZero<u32>>). We'd need a way to get total capacity, so maybe it makes sense to have separate total_capacity() and unit_capacity() methods (which would give the same value for non-divisible assets).

Unfortunately, there are different parts of the code which all need to be made "divisible asset aware" (dispatch, investment, output), so we have two options: fix them all in one go or one at a time. I think we should do the latter, but that will essentially mean that we have two representations for divisble assets at the same time in short run. This means that we could e.g. update the dispatch code to handle these assets with a count >1, while investment and output stay the same. It's a little ugly, but I think it's better than trying to rework the API all over the place at the same time.

Here's my idea of how to do that:

• Add the extra "count" field I mentioned above
• Revive the idea of parent assets 😄! Parent assets should contain a Vec<Rc<Asset>> representing child assets. We could also add a "parent" field to child assets, if we need to get parents from children (it should be a WeakRef<Asset> to avoid reference cycles).
• We should change AssetPool so we can have separate methods for iterating over active assets with children and active assets with parents. At first, all the code will use the "children" version of this method (corresponding to the current behaviour)
• We then incrementally change over the different parts of the code to use the "parent" versions of these assets
• Once there are no users of the "child" assets, we can remove this functionality, which effectively means just using the "parent" API everywhere

What do you think?

[tsmbland]

I don't know if I fully understand what you're proposing. I guess it will make more sense when I start to see it in action.

I don't really understand this "count" field. Currently all Asset object either represent a non-divisible asset, or a single unit of a divisible asset (we chuck away the pre-division parent assets, although we store their ids), so wouldn't this just be 1 for everything? (At least for the time being)

I think most intuitive for me would be to make Asset::capacity an enum which could either be a Capacity value (in the case of non-divisible assets) or a usize number of units (or possibly an indexset of ids if we still need ids representing each unit). If I understand correctly we'll eventually no longer need child assets (i.e. these won't be their own Asset object, we'd have a single parent Asset instead, is that correct??), so I think that would work.

It seems to me (and, again, not really sure I fully understand your proposal) that the long term solution will be much simpler than the intermediate, in which case I'm not totally opposed to the "do it all in one go" approach

[alexdewar]

You're right that it could be easier to do it in one go. It just depends on how long it takes to get the new API working with all parts of the code. If we go down that route, then it would be good to have all the details worked out in advance so we avoid having this feature branch living for ages.

I probably wasn't clear (I was writing hastily before the nursery run). Sorry!

By the count field, I meant that this would be a new field that would be used by divisible assets. It would be part of the new API; you're right that we wouldn't need it currently. The idea would be that we basically have two APIs -- a new one and an old one -- that live side by side while we migrate over the code to using the new API. The new API would involve the count field (or an enum as you suggest) and the old one would involve representing each unit as a separate Asset. So: old API == children only, new API == parents only.

I don't think this will be too painful. Once we've got #1070 merged, I'm happy to knock together a draft PR for this, if you like. Might be easier to talk about with actual code there.

[tsmbland]

I don't think this will be too painful. Once we've got #1070 merged, I'm happy to knock together a draft PR for this, if you like. Might be easier to talk about with actual code there.

Sounds good!