feat(sequencer): make ABCI response for account balances deterministic

[ethanoroshiba]

Summary

Ensures that a response to an ABCI request for account balances is deterministic.

Background

Previously was not sorted, and the AssetBalance stream is non-deterministic, which isn't great for an external API.

Changes

• Created deterministic sorting function for sorting the AssetBalances firstly in descending order by balance, then in ascending order by name if the balances are equal.

Testing

Added unit test to ensure the sorting function works as expected.

Related Issues

closes #1451

[SuperFluffy]

Note on the PR title:

1. I removed ! because I don't think it's breaking. If you had gone from ordered -> unordered I would have agreed (since external users might have relied on the response being ordered). But going from unordered -> ordered is more structure so that's fine.
2. I changed it from chore to feat because I was mildly leaning towards this being more of a fix (but maybe chore or fix would have been also good).
3. I edited the PR title to be more descriptive for users who don't have intimate knowledge of the particular module you are changing. In general I try to avoid mentioning specifics item names (types, functions) in favor of being more descriptive.

[SuperFluffy]

Don't nest matches but instead match on tuples:

    use Self::{TracePrefixed, IbcPrefixed};
    match (self, other) {
        (TracePrefixed(lhs), TracePrefixed(rhs)) => lhs.cmp(rhs),
        (TracePrefixed(_), IbcPrefixed(_)) => Ordering::Less,
        (IbcPrefixed(lhs), IbcPrefixed(rhs)) => lhs.cmp(rhs),
        (IbcPrefixed(_), TracePrefixed(_)) => Prdering::Greater,
    }

[ethanoroshiba]

Was unable to import Self due to Rust's restrictions, but implemented the tuples in 535c1c4.

[SuperFluffy]

For the Ord impl here, can you provide exhaustive tests that demonstrate and document the order of the denoms?

Naively, I would expect a lexical ordering among trace-prefixed and ibc-prefixed denoms, so it would be great to have tests that also demonstrate that ordering.

I think I should also walk back my suggestion to order ibc-prefixed, trace-prefixed later: a naive implementation that keeps ibc denominations in string form (without parsing) would just use lexical ordering of the strings. I think the parsed variant here should also reflect that.

In addition to unit tests for IbcPrefixed and TracePrefixed, do a more complex example:

1. create a "string list" of ibc denomination strings (mixing both trace-prefixed and ibc-prefixed).
2. parse this list as a Denom list.
3. sort the string list (this will give lexical order).
4. sort the denom list by its Ord implementation and then use to_string to convert all elements to strings again
5. assert that results of steps 3 and 4 are the same.

[SuperFluffy]

Rethinking this: these should just be sorted by lexical order of the denominations.

You cannot compare the magnitude of different quantities, so we shouldn't order by them.

[SuperFluffy]

I'd like to have these be more atomic and not sort a vec (it's pretty difficult to read).

Instead do something like:

#[track_caller]
fn assert_denom_order<T: FromStr>(left: &str, right: &str]) {
    let left_denom: T = left.parse().unwrap();
    let right_denom: T = right.parse().unwrap();
    assert_eq!(left_denom.cmp(&right_denom), left.cmp(right));
}

#[test]
fn denoms_are_lexically_ordered() {
    assert_denom_order::<Denom>("ibc/etc", "trace/etc");
    todo!("and all the rest");
}

[ethanoroshiba]

Did something similar in 0e5eeb5, let me know if it works for you!

[SuperFluffy]

This looks great, thanks! The PartialOrd/Ord impls on Denom will be useful for a small PR I am writing right now.

Please remove the comment above balances.sort_unstable_by, but otherwise this looks good (for someone with knowledge of the Rust std the use of sort_unstable is not unexpected, and its behavior does not need to be further explained: even if there were two keys of the same value and their position exchanged, this would not be an observable effect).

[SuperFluffy]

Are any of the PartialEq, Eq, Ord, and PartialOrd impls even necessary at this point?

If they are not necessary, I'd rather remove them.

[ethanoroshiba]

I've removed the impls for Ord and PartialOrd from AssetBalance, but aside from that all of them are necessary. Denom::cmp calls either IbcPrefixed::cmp or TracePrefixed::cmp, and trace calls TraceSegments::cmp, etc.

[SuperFluffy]

Yeah, those make sense. I just thought of AssetBalance :)