Value derives Ord but implements PartialOrd

[SebastienGllmt]

We rely on the automatically generated Ord implementation for Value in a few places. However, this auto-generated implementation is wrong because Values are not total which is why we had to implement PartialOrd for it. We need to fix this because it could cause subtle issues.

The same thing applies to MultiAsset

[rooooooooob]

We rely on the automatically generated Ord implementation for Value in a few places

It's entirely possible to define a total ordering using an arbitrary definition of order for the multiasset parts, which is useful for when it's used e.g. as a hash map key, but then there's a separate thing where we might want to have a more Cardano-semantics specific ordering which is where the partial ordering comes in for when one is effectively a subset (every asset <= the other).

People wanting the second definition would be expecting entirely different behavior than the first one and I don't know how we can handle this using the built in Ord and PartialOrd definitions. For anywhere the partial order exists the ordering is consistent in the total ordering, but in cases where it's not (e.g. neither asset is a subset of the other) it's impossible to define a total ordering that respects the partial ordering as far as I can see.

Is it possible to have both be consistent (ignoring whether it's confusing having the difference)? e.g. anywhere the partial ordering is defined the total ordering would give the same value, and this also would preserve transitivity. Anywhere else (i.e. neither is a subset of the other) would be an arbitrary total ordering that should not be interpreted as anything relating to the value semantics of Cardano. Making it return the same value as the partial order could be easy if that's just an explicit rule, but then it could cause issues with internal Ord consistency maybe?

Thinking out loud here we'd have a, b and c where a <= b and b <= c requires a <= c to also be true. Reflexivity is a given with any sane ordering, and anti-reflexivity would also be a given too. The cases we need to think about for transitivity is when you're mixing up whether the Ord definition uses the arbitrary one or the partial order one since otherwise they would just be consistent already.

Let's assume that a <= b by partial order and b <= c is the total order's arbitrary definition. If the only arbitraryness in the total order is e.g. doing a per-unit comparison in each asset in e.g. lex order (or any consistent ordering) then this should still be consistent in the arbitrary total order.

If it's the other way around and a <= b is by the arbitrary total order and b <= c is given by our partial order then this should also be consistent as if every asset in c is >= to the same asset in b then the arbitrary ordering should hold here too between a and c directly.

Rust's ordering actually have 3 (since equal is a possibility) so it's not a direct <= but I don't think this changes anything when evaluating consistency since those traits can all be derived from a consistent <=.

So I think this is possible to do, but this is more of an issue for documentation that for Cardano-semantics only the partial ordering holds true. Otherwise we would have to keep the partial order only and then not be able to use the Value in a hash map anywhere.

Although to be sure we might want to write our own Ord trait implementation in case the auto-derived one does something bizarre but it's probably fine as long as it picks one of the two fields (ADA or multiasset), does the comparison there, then for the mutliasset hashmap just does a comparison on each one using some well-defined (e.g. using Ord on the keys). I'm pretty sure that's what this would do and in that case this is a documentation issue.

[rooooooooob]

Although even just documenting that Ord is arbitrary might not be enough as I'm sure 99% of users wouldn't read that and would just assume that Ord is giving them the ordering in Cardano semantics e.g. if a <= b then b is enough to cover the cost of a which would not be true. But then if we just get rid of the Ord trait to avoid this user pitfall, we can no longer use Value anywhere in any maps. Where are we even using Ord for Value or Multiasset or Mint anywhere then? It's not used as a key in the binary spec, is this used somewhere in one of the utility functions? If it's because basically every struct except where we couldn't (e.g. it contained crypto/other things hand-inserted to replace the byte strings in the CDDL) we imlement those traits just in case they're used as a key, then this shouldn't be an issue after the regeneration. With the latest cddl-codegen those traits are only generated when they're used as a key, or indirectly used as a key (e.g. something used as a key contains one). We can probably just drop the Ord trait completely then if we don't need it in some utility functionality.

[rooooooooob]

We don't implement Ord anymore. See comment in chain/rust/assets.rs:

// note: we purposefully don't derive or implement Ord for Value to avoid potentially confusing
// orderings that don't obey Cardano semantics i.e. if x >= y then input x can cover cost y
// If you need to use Value or something in a tree map please consider using a hash map instead.