From 0e50c5e7c36e5ac3ba97aaef0f39ee6977af5c3d Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 26 Sep 2024 10:53:33 -0500 Subject: [PATCH 1/7] Make asset balance sorted for query --- crates/astria-sequencer/src/accounts/query.rs | 88 ++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/crates/astria-sequencer/src/accounts/query.rs b/crates/astria-sequencer/src/accounts/query.rs index 1aaa5f6412..e65223841a 100644 --- a/crates/astria-sequencer/src/accounts/query.rs +++ b/crates/astria-sequencer/src/accounts/query.rs @@ -88,7 +88,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 { @@ -100,6 +100,11 @@ pub(crate) async fn balance_request( }; } }; + + // Unstable sort does not allocate auxillary memory and is typically faster. Custom sorting + // function is deterministic and only allocates in the case that two values are equal. + balances.sort_unstable_by(compare_asset_balances); + let payload = BalanceResponse { height: height.value(), balances, @@ -215,3 +220,84 @@ async fn preprocess_request( }; Ok((address, snapshot, height)) } + +/// Custom deterministic sorting function for `AssetBalance` that sorts by balance in descending +/// order and then by denom in ascending order. This function should never return `Ordering::Equal`, +/// as calling `sort_unstable_by` on equal values has a non-deterministic result. +fn compare_asset_balances(lhs: &AssetBalance, rhs: &AssetBalance) -> std::cmp::Ordering { + use std::cmp::Ordering; + + match lhs.balance.cmp(&rhs.balance) { + // Denoms should never have the same display name + Ordering::Equal => lhs.denom.to_string().cmp(&rhs.denom.to_string()), + Ordering::Less => Ordering::Greater, + Ordering::Greater => Ordering::Less, + } +} + +mod test { + #[test] + fn compare_asset_balances_sorts_asset_balances_as_expected() { + use astria_core::{ + primitive::v1::asset::Denom, + protocol::account::v1alpha1::AssetBalance, + }; + + let trace_denom_1 = "first/new/asset".parse::().unwrap(); + let trace_denom_2 = "second/new/asset".parse::().unwrap(); + let ibc_denom_1 = format!("ibc/{}", hex::encode([0u8; 32])) + .parse::() + .unwrap(); + let ibc_denom_2 = format!("ibc/{}", hex::encode([1u8; 32])) + .parse::() + .unwrap(); + + let mut balances = vec![ + AssetBalance { + denom: trace_denom_1.clone(), + balance: 1, + }, + AssetBalance { + denom: trace_denom_1.clone(), + balance: 2, + }, + AssetBalance { + denom: trace_denom_2.clone(), + balance: 2, + }, + AssetBalance { + denom: trace_denom_2.clone(), + balance: 3, + }, + AssetBalance { + denom: ibc_denom_1.clone(), + balance: 3, + }, + AssetBalance { + denom: ibc_denom_1.clone(), + balance: 4, + }, + AssetBalance { + denom: ibc_denom_2.clone(), + balance: 4, + }, + AssetBalance { + denom: ibc_denom_2.clone(), + balance: 5, + }, + ]; + + let original_balances = balances.clone(); + + balances.sort_unstable_by(super::compare_asset_balances); + + assert_eq!(balances[0], original_balances[7]); + assert_eq!(balances[1], original_balances[5]); + assert_eq!(balances[2], original_balances[6]); + assert_eq!(balances[3], original_balances[4]); + assert_eq!(balances[4], original_balances[3]); + assert_eq!(balances[5], original_balances[1]); + assert_eq!(balances[6], original_balances[2]); + assert_eq!(balances[7], original_balances[0]); + } +} From fb7110f1f824b3eb13602e4ffc0f3590b336b2e6 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Fri, 27 Sep 2024 12:10:49 -0500 Subject: [PATCH 2/7] Requested changes --- .../src/primitive/v1/asset/denom.rs | 87 +++++++++++++++++++ crates/astria-sequencer/src/accounts/query.rs | 7 +- 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/crates/astria-core/src/primitive/v1/asset/denom.rs b/crates/astria-core/src/primitive/v1/asset/denom.rs index 91dd06bf24..03cb146499 100644 --- a/crates/astria-core/src/primitive/v1/asset/denom.rs +++ b/crates/astria-core/src/primitive/v1/asset/denom.rs @@ -147,6 +147,29 @@ impl FromStr for Denom { } } +impl PartialOrd for Denom { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Denom { + /// If the denoms are the same type, returns their comparison. Otherwise, returns IBC prefixed + /// denoms as less than trace prefixed denoms. + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + match self { + Self::TracePrefixed(lhs) => match other { + Self::TracePrefixed(rhs) => lhs.cmp(rhs), + Self::IbcPrefixed(_) => std::cmp::Ordering::Greater, + }, + Self::IbcPrefixed(lhs) => match other { + Self::IbcPrefixed(rhs) => lhs.cmp(rhs), + Self::TracePrefixed(_) => std::cmp::Ordering::Less, + }, + } + } +} + #[derive(Debug, thiserror::Error)] #[error(transparent)] pub struct ParseDenomError(ParseDenomErrorKind); @@ -284,6 +307,21 @@ impl TracePrefixed { } } +impl PartialOrd for TracePrefixed { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for TracePrefixed { + /// Returns trace comparison if not equal, otherwise returns base denom comparison. + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.trace + .cmp(&other.trace) + .then_with(|| self.base_denom.cmp(&other.base_denom)) + } +} + #[derive(Debug, Clone, Hash, PartialEq, Eq)] struct TraceSegments { inner: VecDeque, @@ -354,6 +392,28 @@ impl FromStr for TraceSegments { Ok(parsed_segments) } } + +impl PartialOrd for TraceSegments { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for TraceSegments { + /// Returns the first non-equal comparison between the two trace segments. If one doesn't exist, + /// returns the shortest, and if they are equal, returns 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, @@ -372,6 +432,21 @@ impl PortAndChannel { } } +impl PartialOrd for PortAndChannel { + fn partial_cmp(&self, other: &Self) -> Option { + 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 { @@ -549,6 +624,18 @@ impl FromStr for IbcPrefixed { } } +impl PartialOrd for IbcPrefixed { + fn partial_cmp(&self, other: &Self) -> Option { + 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::{ diff --git a/crates/astria-sequencer/src/accounts/query.rs b/crates/astria-sequencer/src/accounts/query.rs index e65223841a..75ce03afce 100644 --- a/crates/astria-sequencer/src/accounts/query.rs +++ b/crates/astria-sequencer/src/accounts/query.rs @@ -77,6 +77,9 @@ async fn get_trace_prefixed_account_balances( stream.try_collect::>().await } +/// Returns a list of `AssetBalance`s for the provided address. `AssetBalance`s are sorted first in +/// descending order by balance, then in ascending order by denom. IBC prefixed denoms are treated +/// as less than trace prefixed denoms. pub(crate) async fn balance_request( storage: Storage, request: request::Query, @@ -228,13 +231,13 @@ fn compare_asset_balances(lhs: &AssetBalance, rhs: &AssetBalance) -> std::cmp::O use std::cmp::Ordering; match lhs.balance.cmp(&rhs.balance) { - // Denoms should never have the same display name - Ordering::Equal => lhs.denom.to_string().cmp(&rhs.denom.to_string()), + Ordering::Equal => lhs.denom.cmp(&rhs.denom), Ordering::Less => Ordering::Greater, Ordering::Greater => Ordering::Less, } } +#[cfg(test)] mod test { #[test] fn compare_asset_balances_sorts_asset_balances_as_expected() { From 535c1c4357abc18bea779f3d02f03e90e3d93103 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 30 Sep 2024 10:01:51 -0500 Subject: [PATCH 3/7] requested changes --- .../src/primitive/v1/asset/denom.rs | 139 +++++++++++++++--- .../src/protocol/account/v1alpha1/mod.rs | 14 +- crates/astria-sequencer/src/accounts/query.rs | 94 +----------- 3 files changed, 142 insertions(+), 105 deletions(-) diff --git a/crates/astria-core/src/primitive/v1/asset/denom.rs b/crates/astria-core/src/primitive/v1/asset/denom.rs index 03cb146499..a964fdb85b 100644 --- a/crates/astria-core/src/primitive/v1/asset/denom.rs +++ b/crates/astria-core/src/primitive/v1/asset/denom.rs @@ -154,18 +154,18 @@ impl PartialOrd for Denom { } impl Ord for Denom { - /// If the denoms are the same type, returns their comparison. Otherwise, returns IBC prefixed - /// denoms as less than trace prefixed denoms. + /// 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 { - Self::TracePrefixed(lhs) => match other { - Self::TracePrefixed(rhs) => lhs.cmp(rhs), - Self::IbcPrefixed(_) => std::cmp::Ordering::Greater, - }, - Self::IbcPrefixed(lhs) => match other { - Self::IbcPrefixed(rhs) => lhs.cmp(rhs), - Self::TracePrefixed(_) => std::cmp::Ordering::Less, - }, + 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() + } } } } @@ -314,11 +314,30 @@ impl PartialOrd for TracePrefixed { } impl Ord for TracePrefixed { - /// Returns trace comparison if not equal, otherwise returns base denom comparison. + /// 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 { - self.trace - .cmp(&other.trace) - .then_with(|| self.base_denom.cmp(&other.base_denom)) + 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)), + } } } @@ -400,8 +419,9 @@ impl PartialOrd for TraceSegments { } impl Ord for TraceSegments { - /// Returns the first non-equal comparison between the two trace segments. If one doesn't exist, - /// returns the shortest, and if they are equal, returns equal. + /// 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() @@ -702,6 +722,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::{ @@ -829,4 +862,76 @@ mod tests { let denom = denom_str.parse::().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::() + .unwrap(), + format!("ibc/{}", hex::encode([4u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([0u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([240u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([60u8; 32])) + .parse::() + .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::().unwrap(), + "nria".parse::().unwrap(), + "pretty/long/trace/prefixed/denom" + .parse::() + .unwrap(), + "_using/underscore/here".parse::().unwrap(), + "astria/test/asset".parse::().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 mut denoms = vec![ + format!("ibc/{}", hex::encode([135u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([4u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([0u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([240u8; 32])) + .parse::() + .unwrap(), + format!("ibc/{}", hex::encode([60u8; 32])) + .parse::() + .unwrap(), + "ethan/was/here".parse::().unwrap(), + "nria".parse::().unwrap(), + "pretty/long/trace/prefixed/denom".parse::().unwrap(), + "_using/underscore/here".parse::().unwrap(), + "astria/test/asset".parse::().unwrap(), + ]; + let mut denoms_lexical = denoms.clone(); + denoms.sort_unstable(); + denoms_lexical.sort_unstable_by_key(ToString::to_string); + assert_eq!(denoms, denoms_lexical); + } } diff --git a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs index c5dadaf1c0..ea2a13c0e5 100644 --- a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs @@ -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, @@ -55,6 +55,18 @@ impl AssetBalance { } } +impl PartialOrd for AssetBalance { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for AssetBalance { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.denom.cmp(&other.denom) + } +} + impl raw::BalanceResponse { /// Converts an astria native [`BalanceResponse`] to a /// protobuf [`raw::BalanceResponse`]. diff --git a/crates/astria-sequencer/src/accounts/query.rs b/crates/astria-sequencer/src/accounts/query.rs index 75ce03afce..bbb28cce7b 100644 --- a/crates/astria-sequencer/src/accounts/query.rs +++ b/crates/astria-sequencer/src/accounts/query.rs @@ -77,9 +77,8 @@ async fn get_trace_prefixed_account_balances( stream.try_collect::>().await } -/// Returns a list of `AssetBalance`s for the provided address. `AssetBalance`s are sorted first in -/// descending order by balance, then in ascending order by denom. IBC prefixed denoms are treated -/// as less than trace prefixed denoms. +/// 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, @@ -104,9 +103,11 @@ pub(crate) async fn balance_request( } }; - // Unstable sort does not allocate auxillary memory and is typically faster. Custom sorting - // function is deterministic and only allocates in the case that two values are equal. - balances.sort_unstable_by(compare_asset_balances); + // Unstable sort does not allocate auxillary memory and is typically faster. This utilizes a + // custom `Ord` implementation which sorts by `Denom` in alphabetical order. The comparison + // should never return `Ordering::Equal` since `Denom` is a unique identifier, hence the + // unstable sort will be deterministic. + balances.sort_unstable(); let payload = BalanceResponse { height: height.value(), @@ -223,84 +224,3 @@ async fn preprocess_request( }; Ok((address, snapshot, height)) } - -/// Custom deterministic sorting function for `AssetBalance` that sorts by balance in descending -/// order and then by denom in ascending order. This function should never return `Ordering::Equal`, -/// as calling `sort_unstable_by` on equal values has a non-deterministic result. -fn compare_asset_balances(lhs: &AssetBalance, rhs: &AssetBalance) -> std::cmp::Ordering { - use std::cmp::Ordering; - - match lhs.balance.cmp(&rhs.balance) { - Ordering::Equal => lhs.denom.cmp(&rhs.denom), - Ordering::Less => Ordering::Greater, - Ordering::Greater => Ordering::Less, - } -} - -#[cfg(test)] -mod test { - #[test] - fn compare_asset_balances_sorts_asset_balances_as_expected() { - use astria_core::{ - primitive::v1::asset::Denom, - protocol::account::v1alpha1::AssetBalance, - }; - - let trace_denom_1 = "first/new/asset".parse::().unwrap(); - let trace_denom_2 = "second/new/asset".parse::().unwrap(); - let ibc_denom_1 = format!("ibc/{}", hex::encode([0u8; 32])) - .parse::() - .unwrap(); - let ibc_denom_2 = format!("ibc/{}", hex::encode([1u8; 32])) - .parse::() - .unwrap(); - - let mut balances = vec![ - AssetBalance { - denom: trace_denom_1.clone(), - balance: 1, - }, - AssetBalance { - denom: trace_denom_1.clone(), - balance: 2, - }, - AssetBalance { - denom: trace_denom_2.clone(), - balance: 2, - }, - AssetBalance { - denom: trace_denom_2.clone(), - balance: 3, - }, - AssetBalance { - denom: ibc_denom_1.clone(), - balance: 3, - }, - AssetBalance { - denom: ibc_denom_1.clone(), - balance: 4, - }, - AssetBalance { - denom: ibc_denom_2.clone(), - balance: 4, - }, - AssetBalance { - denom: ibc_denom_2.clone(), - balance: 5, - }, - ]; - - let original_balances = balances.clone(); - - balances.sort_unstable_by(super::compare_asset_balances); - - assert_eq!(balances[0], original_balances[7]); - assert_eq!(balances[1], original_balances[5]); - assert_eq!(balances[2], original_balances[6]); - assert_eq!(balances[3], original_balances[4]); - assert_eq!(balances[4], original_balances[3]); - assert_eq!(balances[5], original_balances[1]); - assert_eq!(balances[6], original_balances[2]); - assert_eq!(balances[7], original_balances[0]); - } -} From 0e5eeb5669259eebfc436515d90d603f65870de0 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 16 Oct 2024 14:07:51 -0500 Subject: [PATCH 4/7] requested changes --- .../src/primitive/v1/asset/denom.rs | 74 ++++++++++++------- .../src/protocol/account/v1alpha1/mod.rs | 13 +++- 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/crates/astria-core/src/primitive/v1/asset/denom.rs b/crates/astria-core/src/primitive/v1/asset/denom.rs index a964fdb85b..3872158614 100644 --- a/crates/astria-core/src/primitive/v1/asset/denom.rs +++ b/crates/astria-core/src/primitive/v1/asset/denom.rs @@ -907,31 +907,53 @@ mod tests { #[test] fn trace_and_ibc_prefixed_ord_matches_lexical_sort() { - let mut denoms = vec![ - format!("ibc/{}", hex::encode([135u8; 32])) - .parse::() - .unwrap(), - format!("ibc/{}", hex::encode([4u8; 32])) - .parse::() - .unwrap(), - format!("ibc/{}", hex::encode([0u8; 32])) - .parse::() - .unwrap(), - format!("ibc/{}", hex::encode([240u8; 32])) - .parse::() - .unwrap(), - format!("ibc/{}", hex::encode([60u8; 32])) - .parse::() - .unwrap(), - "ethan/was/here".parse::().unwrap(), - "nria".parse::().unwrap(), - "pretty/long/trace/prefixed/denom".parse::().unwrap(), - "_using/underscore/here".parse::().unwrap(), - "astria/test/asset".parse::().unwrap(), - ]; - let mut denoms_lexical = denoms.clone(); - denoms.sort_unstable(); - denoms_lexical.sort_unstable_by_key(ToString::to_string); - assert_eq!(denoms, denoms_lexical); + 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::().unwrap(); + let right_denom = right.parse::().unwrap(); + assert_eq!(left_denom.cmp(&right_denom), left.cmp(right)); } } diff --git a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs index ea2a13c0e5..078e9430b8 100644 --- a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs @@ -22,7 +22,7 @@ enum AssetBalanceErrorKind { InvalidDenom { source: ParseDenomError }, } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Eq)] pub struct AssetBalance { pub denom: Denom, pub balance: u128, @@ -63,7 +63,16 @@ impl PartialOrd for AssetBalance { impl Ord for AssetBalance { fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.denom.cmp(&other.denom) + match self.denom.cmp(&other.denom) { + std::cmp::Ordering::Equal => self.balance.cmp(&other.balance), + other => other, + } + } +} + +impl PartialEq for AssetBalance { + fn eq(&self, other: &Self) -> bool { + self.denom == other.denom && self.balance == other.balance } } From 1be7ecf8e751b0c97482a3b2fb5e06e64de5d96e Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 16 Oct 2024 14:22:03 -0500 Subject: [PATCH 5/7] fixes --- .../src/primitive/v1/asset/denom.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/astria-core/src/primitive/v1/asset/denom.rs b/crates/astria-core/src/primitive/v1/asset/denom.rs index 2bf486b968..cd359e4dcc 100644 --- a/crates/astria-core/src/primitive/v1/asset/denom.rs +++ b/crates/astria-core/src/primitive/v1/asset/denom.rs @@ -1003,21 +1003,21 @@ mod tests { 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_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_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_3, trace_4); + assert_ord_matches_lexical(trace_3, trace_5); - assert_ord_matches_lexical(&trace_4, &trace_5); + assert_ord_matches_lexical(trace_4, trace_5); } #[track_caller] From 0af3bcf0daade6e8454de54a2947a452434ffac8 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 17 Oct 2024 11:17:36 -0500 Subject: [PATCH 6/7] requested changes --- .../src/protocol/account/v1alpha1/mod.rs | 23 +------------------ crates/astria-sequencer/src/accounts/query.rs | 2 +- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs index 078e9430b8..697696ad66 100644 --- a/crates/astria-core/src/protocol/account/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/account/v1alpha1/mod.rs @@ -22,7 +22,7 @@ enum AssetBalanceErrorKind { InvalidDenom { source: ParseDenomError }, } -#[derive(Clone, Debug, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct AssetBalance { pub denom: Denom, pub balance: u128, @@ -55,27 +55,6 @@ impl AssetBalance { } } -impl PartialOrd for AssetBalance { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for AssetBalance { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - match self.denom.cmp(&other.denom) { - std::cmp::Ordering::Equal => self.balance.cmp(&other.balance), - other => other, - } - } -} - -impl PartialEq for AssetBalance { - fn eq(&self, other: &Self) -> bool { - self.denom == other.denom && self.balance == other.balance - } -} - impl raw::BalanceResponse { /// Converts an astria native [`BalanceResponse`] to a /// protobuf [`raw::BalanceResponse`]. diff --git a/crates/astria-sequencer/src/accounts/query.rs b/crates/astria-sequencer/src/accounts/query.rs index 62c6b462a8..ab9c98caaa 100644 --- a/crates/astria-sequencer/src/accounts/query.rs +++ b/crates/astria-sequencer/src/accounts/query.rs @@ -107,7 +107,7 @@ pub(crate) async fn balance_request( // custom `Ord` implementation which sorts by `Denom` in alphabetical order. The comparison // should never return `Ordering::Equal` since `Denom` is a unique identifier, hence the // unstable sort will be deterministic. - balances.sort_unstable(); + balances.sort_unstable_by(|a, b| a.denom.cmp(&b.denom)); let payload = BalanceResponse { height: height.value(), From 6fdf57404c1cebc2d37bcc15d55978b7312d0f12 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 21 Oct 2024 07:45:12 -0500 Subject: [PATCH 7/7] remove comment above --- crates/astria-sequencer/src/accounts/query.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/query.rs b/crates/astria-sequencer/src/accounts/query.rs index 6e092fa3a5..9944136a7d 100644 --- a/crates/astria-sequencer/src/accounts/query.rs +++ b/crates/astria-sequencer/src/accounts/query.rs @@ -103,10 +103,6 @@ pub(crate) async fn balance_request( } }; - // Unstable sort does not allocate auxillary memory and is typically faster. This utilizes a - // custom `Ord` implementation which sorts by `Denom` in alphabetical order. The comparison - // should never return `Ordering::Equal` since `Denom` is a unique identifier, hence the - // unstable sort will be deterministic. balances.sort_unstable_by(|a, b| a.denom.cmp(&b.denom)); let payload = BalanceResponse {