Skip to content

Inconsistent semantics (and bad performance?) when comparing asset types #1215

@alexdewar

Description

@alexdewar

The asset types (AssetRef and Asset) have slightly odd and inconsistent semantics when it comes to comparisons, as with AssetCapacity (see here).

Firstly, we derive PartialEq for Assets rather than implementing it manually, which means we're comparing all the fields recursively (including all the maps!). This will be bad for performance.

On top of this, AssetRef implements Eq and Ord, but the cmp() method panics if either of the assets is not commissioned (i.e. doesn't have an ID). As discussed in the AssetCapacity case, it's not good practice to panic in cmp(), but, apart from that, we also should have consistent semantics for Ord and Eq implementations -- possibly as well as Hash. PartialOrd and PartialEq may describe the semantics better, though Eq and Hash are both required to use an object as a key (a requirement for AssetRef).

Note that we may want to do a "deep" comparison for the sake of unit tests, but we should think carefully about how we do this. In any case, we shouldn't be doing these deep comparisons by default when running the code normally.

Note: Unfortunately we can't just compare the underlying pointers of AssetRefs (via Rc::ptr_eq) because I can think of one place where we want different objects to be treated as equal (code in #1211) and there may be more.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    Status

    📋 Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions