Skip to content

Commit

Permalink
feat(sequencer): make ABCI response for account balances deterministic (
Browse files Browse the repository at this point in the history
#1574)

## 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 `AssetBalance`s
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
  • Loading branch information
ethanoroshiba authored Oct 21, 2024
1 parent 9318650 commit b7be904
Show file tree
Hide file tree
Showing 3 changed files with 221 additions and 2 deletions.
214 changes: 214 additions & 0 deletions crates/astria-core/src/primitive/v1/asset/denom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,29 @@ impl FromStr for Denom {
}
}

impl PartialOrd for Denom {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for Denom {
/// Comparison meant to mirror the lexical ordering based on a [`Denom`]'s display-formatted
/// string, but without allocation. If both denoms have the same prefix, the prefix
/// comparison function is called. Otherwise, the [`TracePrefixed`] denom is compared with the
/// IBC prefix `ibc/`.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match (self, other) {
(Self::TracePrefixed(lhs), Self::TracePrefixed(rhs)) => lhs.cmp(rhs),
(Self::TracePrefixed(trace), Self::IbcPrefixed(_)) => compare_trace_to_ibc(trace),
(Self::IbcPrefixed(lhs), Self::IbcPrefixed(rhs)) => lhs.cmp(rhs),
(Self::IbcPrefixed(_), Self::TracePrefixed(trace)) => {
compare_trace_to_ibc(trace).reverse()
}
}
}
}

#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct ParseDenomError(ParseDenomErrorKind);
Expand Down Expand Up @@ -350,6 +373,40 @@ impl TracePrefixed {
}
}

impl PartialOrd for TracePrefixed {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for TracePrefixed {
/// This comparison is meant to mirror the lexical ordering of a [`TracePrefixed`]'s
/// display-formatted string without allocation. It returns the collowing comparisons:
/// - If both traces are empty, compares the base denoms.
/// - If one trace is empty, compares the base denom to the leading port of the other trace.
/// - If both traces are non-empty, compares the traces, then compares the base denoms.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match (self.trace.is_empty(), other.trace.is_empty()) {
(true, true) => self.base_denom.cmp(&other.base_denom),
(true, false) => self.base_denom.as_str().cmp(
other
.trace
.leading_port()
.expect("leading port should be `Some` after checking for its existence"),
),
(false, true) => self
.trace
.leading_port()
.expect("leading port should be `Some` after checking for its existence")
.cmp(other.base_denom.as_str()),
(false, false) => self
.trace
.cmp(&other.trace)
.then_with(|| self.base_denom.cmp(&other.base_denom)),
}
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
struct TraceSegments {
inner: VecDeque<PortAndChannel>,
Expand Down Expand Up @@ -420,6 +477,29 @@ impl FromStr for TraceSegments {
Ok(parsed_segments)
}
}

impl PartialOrd for TraceSegments {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for TraceSegments {
/// Returns the first non-equal comparison between two trace segments. If one doesn't exist,
/// returns the shortest segment, and if they are entirely equal, returns
/// [`std::cmp::Ordering::Equal`].
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.inner
.iter()
.zip(other.inner.iter())
.find_map(|(self_segment, other_segment)| {
Some(self_segment.cmp(other_segment))
.filter(|&cmp| cmp != std::cmp::Ordering::Equal)
})
.unwrap_or(self.inner.len().cmp(&other.inner.len()))
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
pub struct PortAndChannel {
port: String,
Expand All @@ -438,6 +518,21 @@ impl PortAndChannel {
}
}

impl PartialOrd for PortAndChannel {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for PortAndChannel {
/// Returns port comparison if not equal, otherwise returns channel comparison.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.port
.cmp(&other.port)
.then_with(|| self.channel.cmp(&other.channel))
}
}

impl std::fmt::Display for TracePrefixed {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for segment in &self.trace.inner {
Expand Down Expand Up @@ -617,6 +712,18 @@ impl FromStr for IbcPrefixed {
}
}

impl PartialOrd for IbcPrefixed {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for IbcPrefixed {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.id.cmp(&other.id)
}
}

#[cfg(feature = "serde")]
mod serde_impl {
use serde::{
Expand Down Expand Up @@ -683,6 +790,19 @@ mod serde_impl {
}
}

/// Compares a trace prefixed denom to an IBC prefixed denom. This is meant to mirror the lexical
/// ordering of [`TracePrefixed`] and [`IbcPrefixed`] display-formatted strings without allocation.
/// If the trace prefixed denom has a leading port, it is compared to the IBC prefix `ibc/`.
/// Otherwise, the trace's base denom is compared to the IBC prefix.
fn compare_trace_to_ibc(trace: &TracePrefixed) -> std::cmp::Ordering {
// A trace prefixed denom should never begin with "ibc/", so we can compare direclty to the ibc
// prefix as opposed to the entire ibc-prefixed denomination.
match trace.trace.leading_port() {
Some(port) => port.cmp("ibc/"),
None => trace.base_denom.as_str().cmp("ibc/"),
}
}

#[cfg(test)]
mod tests {
use super::{
Expand Down Expand Up @@ -812,4 +932,98 @@ mod tests {
let denom = denom_str.parse::<Denom>().unwrap();
assert_eq!(denom_str.len(), denom.display_len());
}

#[test]
fn ibc_prefixed_ord_matches_lexical_sort() {
let mut ibc_prefixed = vec![
format!("ibc/{}", hex::encode([135u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([4u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([0u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([240u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([60u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
];
let mut ibc_prefixed_lexical = ibc_prefixed.clone();
ibc_prefixed.sort_unstable();
ibc_prefixed_lexical.sort_unstable_by_key(ToString::to_string);
assert_eq!(ibc_prefixed, ibc_prefixed_lexical);
}

#[test]
fn trace_prefixed_ord_matches_lexical_sort() {
let mut trace_prefixed = vec![
"ethan/was/here".parse::<TracePrefixed>().unwrap(),
"nria".parse::<TracePrefixed>().unwrap(),
"pretty/long/trace/prefixed/denom"
.parse::<TracePrefixed>()
.unwrap(),
"_using/underscore/here".parse::<TracePrefixed>().unwrap(),
"astria/test/asset".parse::<TracePrefixed>().unwrap(),
];
let mut trace_prefixed_lexical = trace_prefixed.clone();
trace_prefixed.sort_unstable();
trace_prefixed_lexical.sort_unstable_by_key(ToString::to_string);
assert_eq!(trace_prefixed, trace_prefixed_lexical);
}

#[test]
fn trace_and_ibc_prefixed_ord_matches_lexical_sort() {
let ibc_1 = format!("ibc/{}", hex::encode([135u8; 32]));
let ibc_2 = format!("ibc/{}", hex::encode([4u8; 32]));
let ibc_3 = format!("ibc/{}", hex::encode([0u8; 32]));
let ibc_4 = format!("ibc/{}", hex::encode([240u8; 32]));
let ibc_5 = format!("ibc/{}", hex::encode([60u8; 32]));
let trace_1 = "ethan/was/here";
let trace_2 = "nria";
let trace_3 = "pretty/long/trace/prefixed/denom";
let trace_4 = "_using/underscore/here";
let trace_5 = "astria/test/asset";

assert_ord_matches_lexical(&ibc_1, &ibc_1);
assert_ord_matches_lexical(&ibc_1, &ibc_2);
assert_ord_matches_lexical(&ibc_1, &ibc_3);
assert_ord_matches_lexical(&ibc_1, &ibc_4);
assert_ord_matches_lexical(&ibc_1, &ibc_5);

assert_ord_matches_lexical(&ibc_2, &ibc_3);
assert_ord_matches_lexical(&ibc_2, &ibc_4);
assert_ord_matches_lexical(&ibc_2, &ibc_5);

assert_ord_matches_lexical(&ibc_3, &ibc_4);
assert_ord_matches_lexical(&ibc_3, &ibc_5);

assert_ord_matches_lexical(&ibc_4, &ibc_5);

assert_ord_matches_lexical(trace_1, trace_1);
assert_ord_matches_lexical(trace_1, trace_2);
assert_ord_matches_lexical(trace_1, trace_2);
assert_ord_matches_lexical(trace_1, trace_3);
assert_ord_matches_lexical(trace_1, trace_4);
assert_ord_matches_lexical(trace_1, trace_5);

assert_ord_matches_lexical(trace_2, trace_3);
assert_ord_matches_lexical(trace_2, trace_4);
assert_ord_matches_lexical(trace_2, trace_5);

assert_ord_matches_lexical(trace_3, trace_4);
assert_ord_matches_lexical(trace_3, trace_5);

assert_ord_matches_lexical(trace_4, trace_5);
}

#[track_caller]
fn assert_ord_matches_lexical(left: &str, right: &str) {
let left_denom = left.parse::<Denom>().unwrap();
let right_denom = right.parse::<Denom>().unwrap();
assert_eq!(left_denom.cmp(&right_denom), left.cmp(right));
}
}
2 changes: 1 addition & 1 deletion crates/astria-core/src/protocol/account/v1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ enum AssetBalanceErrorKind {
InvalidDenom { source: ParseDenomError },
}

#[derive(Clone, Debug, PartialEq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct AssetBalance {
pub denom: Denom,
pub balance: u128,
Expand Down
7 changes: 6 additions & 1 deletion crates/astria-sequencer/src/accounts/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ async fn get_trace_prefixed_account_balances<S: StateRead>(
stream.try_collect::<Vec<_>>().await
}

/// Returns a list of [`AssetBalance`]s for the provided address. `AssetBalance`s are sorted
/// alphabetically by [`asset::Denom`].
pub(crate) async fn balance_request(
storage: Storage,
request: request::Query,
Expand All @@ -88,7 +90,7 @@ pub(crate) async fn balance_request(
Err(err_rsp) => return err_rsp,
};

let balances = match get_trace_prefixed_account_balances(&snapshot, &address).await {
let mut balances = match get_trace_prefixed_account_balances(&snapshot, &address).await {
Ok(balance) => balance,
Err(err) => {
return response::Query {
Expand All @@ -100,6 +102,9 @@ pub(crate) async fn balance_request(
};
}
};

balances.sort_unstable_by(|a, b| a.denom.cmp(&b.denom));

let payload = BalanceResponse {
height: height.value(),
balances,
Expand Down

0 comments on commit b7be904

Please sign in to comment.