fix(core, sequencer): prefix removal source non-refund ics20 packet (#…

…1162) ## Summary Rewrites the IBC denomation construction by parsing its prefix into segments. This fixes the incorrect removal of prefxies for source non-refund ics20 packets. ## Background follow up to #851 which was an audit fix - this fixes the fix by removing the source prefix correctly, in the case assets we're the source of are transferred in. previously, the entire prefix was removed, would would have been invalid if the asset was prefixed by multiple hops, eg. transfer/channel-1/transfer/channel-2/denom would have become just denom when it should have become transfer/channel-2/denom. ## Changes - refactor `astria_core::primitive::1::asset::Denom` to express its prefix as a list of segments - ensure that only full prefix segments are removed - make `Denom` construction a fallible operation by parsing it - removed a bunch of constructors as they were only used in tests and not otherwise ## Testing Provide exhaustive units. ## Related Issues Replaces #1131
astriaorg · Jun 10, 2024 · 6c29d39 · 6c29d39
1 parent 2d79f7d
commit 6c29d39
Show file tree

Hide file tree

Showing 31 changed files with 632 additions and 295 deletions.
diff --git a/.gitignore b/.gitignore
@@ -13,3 +13,5 @@ target/
 
 # We don't want to commit the dependency directory of charts
 charts/*/charts
+
+crates/astria-bridge-withdrawer/src/withdrawer/ethereum/generated/
diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs
@@ -131,9 +131,8 @@ fn event_to_ics20_withdrawal(
     let sender = event.sender.to_fixed_bytes();
     let denom = rollup_asset_denom.clone();
 
-    let (_, channel) = denom
-        .prefix()
-        .rsplit_once('/')
+    let channel = denom
+        .channel()
         .ok_or_eyre("denom must have a channel to be withdrawn via IBC")?;
 
     let memo = Ics20WithdrawalFromRollupMemo {
@@ -189,12 +188,14 @@ fn calculate_packet_timeout_time(timeout_delta: Duration) -> eyre::Result<u64> {
 
 #[cfg(test)]
 mod tests {
+    use asset::default_native_asset;
+
     use super::*;
     use crate::withdrawer::ethereum::astria_withdrawer_interface::SequencerWithdrawalFilter;
 
     #[test]
     fn event_to_bridge_unlock() {
-        let denom = Denom::from("nria".to_string());
+        let denom = default_native_asset();
         let event_with_meta = EventWithMetadata {
             event: WithdrawalEvent::Sequencer(SequencerWithdrawalFilter {
                 sender: [0u8; 20].into(),
@@ -238,7 +239,7 @@ mod tests {
 
     #[test]
     fn event_to_bridge_unlock_divide_value() {
-        let denom = Denom::from("nria".to_string());
+        let denom = default_native_asset();
         let event_with_meta = EventWithMetadata {
             event: WithdrawalEvent::Sequencer(SequencerWithdrawalFilter {
                 sender: [0u8; 20].into(),
@@ -283,7 +284,7 @@ mod tests {
 
     #[test]
     fn event_to_ics20_withdrawal() {
-        let denom = Denom::from("transfer/channel-0/utia".to_string());
+        let denom = "transfer/channel-0/utia".parse::<Denom>().unwrap();
         let destination_chain_address = "address".to_string();
         let event_with_meta = EventWithMetadata {
             event: WithdrawalEvent::Ics20(Ics20WithdrawalFilter {

diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs
@@ -79,7 +79,7 @@ impl Builder {
         let contract_address = address_from_string(&ethereum_contract_address)
             .wrap_err("failed to parse ethereum contract address")?;
 
-        if rollup_asset_denom.prefix().is_empty() {
+        if !rollup_asset_denom.is_prefixed() {
             warn!(
                 "rollup asset denomination is not prefixed; Ics20Withdrawal actions will not be \
                  submitted"
@@ -419,6 +419,7 @@ fn address_from_string(s: &str) -> Result<ethers::types::Address> {
 
 #[cfg(test)]
 mod tests {
+    use asset::default_native_asset;
     use astria_core::{
         primitive::v1::{
             Address,
@@ -547,8 +548,8 @@ mod tests {
             block_number: receipt.block_number.unwrap(),
             transaction_hash: receipt.transaction_hash,
         };
-        let denom: Denom = Denom::from_base_denom("nria");
         let bridge_address = crate::astria_address([1u8; 20]);
+        let denom = default_native_asset();
         let expected_action =
             event_to_action(expected_event, denom.id(), denom.clone(), 1, bridge_address).unwrap();
         let Action::BridgeUnlock(expected_action) = expected_action else {
@@ -638,8 +639,8 @@ mod tests {
             block_number: receipt.block_number.unwrap(),
             transaction_hash: receipt.transaction_hash,
         };
-        let denom = Denom::from("transfer/channel-0/utia".to_string());
         let bridge_address = crate::astria_address([1u8; 20]);
+        let denom = "transfer/channel-0/utia".parse::<Denom>().unwrap();
         let Action::Ics20Withdrawal(mut expected_action) =
             event_to_action(expected_event, denom.id(), denom.clone(), 1, bridge_address).unwrap()
         else {
@@ -761,7 +762,7 @@ mod tests {
             block_number: receipt.block_number.unwrap(),
             transaction_hash: receipt.transaction_hash,
         };
-        let denom: Denom = Denom::from_base_denom("nria");
+        let denom = default_native_asset();
         let bridge_address = crate::astria_address([1u8; 20]);
         let expected_action =
             event_to_action(expected_event, denom.id(), denom.clone(), 1, bridge_address).unwrap();
@@ -862,7 +863,7 @@ mod tests {
             block_number: receipt.block_number.unwrap(),
             transaction_hash: receipt.transaction_hash,
         };
-        let denom = Denom::from("transfer/channel-0/utia".to_string());
+        let denom = "transfer/channel-0/utia".parse::<Denom>().unwrap();
         let bridge_address = crate::astria_address([1u8; 20]);
         let Action::Ics20Withdrawal(mut expected_action) =
             event_to_action(expected_event, denom.id(), denom.clone(), 1, bridge_address).unwrap()

diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/mod.rs b/crates/astria-bridge-withdrawer/src/withdrawer/mod.rs
@@ -5,7 +5,10 @@ use std::{
 };
 
 use astria_core::primitive::v1::{
-    asset,
+    asset::{
+        self,
+        Denom,
+    },
     Address,
 };
 use astria_eyre::eyre::{
@@ -97,7 +100,9 @@ impl Service {
             submitter_handle,
             shutdown_token: shutdown_handle.token(),
             state: state.clone(),
-            rollup_asset_denom: asset::Denom::from(rollup_asset_denomination),
+            rollup_asset_denom: rollup_asset_denomination
+                .parse::<Denom>()
+                .wrap_err("failed to parse ROLLUP_ASSET_DENOMINATION as Denom")?,
             bridge_address: sequencer_bridge_address,
         }
         .build()

diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs b/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs
@@ -13,6 +13,7 @@ use astria_core::{
     primitive::v1::{
         asset::{
             self,
+            default_native_asset,
             Denom,
         },
         Address,
@@ -91,7 +92,6 @@ const SEQUENCER_CHAIN_ID: &str = "test_sequencer-1000";
 const DEFAULT_LAST_ROLLUP_HEIGHT: u64 = 1;
 const DEFAULT_LAST_SEQUENCER_HEIGHT: u64 = 0;
 const DEFAULT_SEQUENCER_NONCE: u32 = 0;
-const DEFAULT_NATIVE_DEMON: &str = "nria";
 const DEFAULT_IBC_DENOM: &str = "transfer/channel-0/utia";
 
 static TELEMETRY: Lazy<()> = Lazy::new(|| {
@@ -151,7 +151,7 @@ impl TestSubmitter {
             sequencer_chain_id: SEQUENCER_CHAIN_ID.to_string(),
             sequencer_cometbft_endpoint,
             state,
-            expected_fee_asset_id: Denom::from(DEFAULT_NATIVE_DEMON.to_string()).id(),
+            expected_fee_asset_id: default_native_asset().id(),
             min_expected_fee_asset_balance: 1_000_000,
         }
         .build()
@@ -214,7 +214,7 @@ async fn register_default_chain_id_guard(cometbft_mock: &MockServer) -> MockGuar
 }
 
 async fn register_default_fee_asset_ids_guard(cometbft_mock: &MockServer) -> MockGuard {
-    let fee_asset_ids = vec![Denom::from(DEFAULT_NATIVE_DEMON.to_string()).id()];
+    let fee_asset_ids = vec![default_native_asset().id()];
     register_allowed_fee_asset_ids_response(fee_asset_ids, cometbft_mock).await
 }
 
@@ -223,7 +223,7 @@ async fn register_default_min_expected_fee_asset_balance_guard(
 ) -> MockGuard {
     register_get_latest_balance(
         vec![AssetBalance {
-            denom: Denom::from(DEFAULT_NATIVE_DEMON.to_string()),
+            denom: default_native_asset(),
             balance: 1_000_000u128,
         }],
         cometbft_mock,
@@ -270,7 +270,7 @@ async fn register_sync_guards(cometbft_mock: &MockServer) -> HashMap<String, Moc
 }
 
 fn make_ics20_withdrawal_action() -> Action {
-    let denom = Denom::from(DEFAULT_IBC_DENOM.to_string());
+    let denom = DEFAULT_IBC_DENOM.parse::<Denom>().unwrap();
     let destination_chain_address = "address".to_string();
     let inner = Ics20Withdrawal {
         denom: denom.clone(),
@@ -299,7 +299,7 @@ fn make_ics20_withdrawal_action() -> Action {
 }
 
 fn make_bridge_unlock_action() -> Action {
-    let denom = Denom::from(DEFAULT_NATIVE_DEMON.to_string());
+    let denom = default_native_asset();
     let inner = BridgeUnlockAction {
         to: Address::builder()
             .array([0u8; 20])

diff --git a/crates/astria-cli/src/commands/sequencer.rs b/crates/astria-cli/src/commands/sequencer.rs
@@ -1,6 +1,9 @@
 use astria_core::{
     crypto::SigningKey,
-    primitive::v1::asset,
+    primitive::v1::{
+        asset,
+        asset::default_native_asset,
+    },
     protocol::transaction::v1alpha1::{
         action::{
             Action,
@@ -172,17 +175,15 @@ pub(crate) async fn get_block_height(args: &BlockHeightGetArgs) -> eyre::Result<
 /// * If the http client cannot be created
 /// * If the latest block height cannot be retrieved
 pub(crate) async fn send_transfer(args: &TransferArgs) -> eyre::Result<()> {
-    use astria_core::primitive::v1::asset::default_native_asset_id;
-
     let res = submit_transaction(
         args.sequencer_url.as_str(),
         args.sequencer_chain_id.clone(),
         args.private_key.as_str(),
         Action::Transfer(TransferAction {
             to: args.to_address.0,
             amount: args.amount,
-            asset_id: default_native_asset_id(),
-            fee_asset_id: default_native_asset_id(),
+            asset_id: default_native_asset().id(),
+            fee_asset_id: default_native_asset().id(),
         }),
     )
     .await
@@ -254,10 +255,7 @@ pub(crate) async fn ibc_relayer_remove(args: &IbcRelayerChangeArgs) -> eyre::Res
 /// * If the http client cannot be created
 /// * If the transaction failed to be included
 pub(crate) async fn init_bridge_account(args: &InitBridgeAccountArgs) -> eyre::Result<()> {
-    use astria_core::primitive::v1::{
-        asset::default_native_asset_id,
-        RollupId,
-    };
+    use astria_core::primitive::v1::RollupId;
 
     let rollup_id = RollupId::from_unhashed_bytes(args.rollup_name.as_bytes());
     let res = submit_transaction(
@@ -266,8 +264,8 @@ pub(crate) async fn init_bridge_account(args: &InitBridgeAccountArgs) -> eyre::R
         args.private_key.as_str(),
         Action::InitBridgeAccount(InitBridgeAccountAction {
             rollup_id,
-            asset_id: default_native_asset_id(),
-            fee_asset_id: default_native_asset_id(),
+            asset_id: default_native_asset().id(),
+            fee_asset_id: default_native_asset().id(),
             sudo_address: None,
             withdrawer_address: None,
         }),
@@ -293,17 +291,15 @@ pub(crate) async fn init_bridge_account(args: &InitBridgeAccountArgs) -> eyre::R
 /// * If the http client cannot be created
 /// * If the transaction failed to be included
 pub(crate) async fn bridge_lock(args: &BridgeLockArgs) -> eyre::Result<()> {
-    use astria_core::primitive::v1::asset::default_native_asset_id;
-
     let res = submit_transaction(
         args.sequencer_url.as_str(),
         args.sequencer_chain_id.clone(),
         args.private_key.as_str(),
         Action::BridgeLock(BridgeLockAction {
             to: args.to_address.0,
-            asset_id: default_native_asset_id(),
+            asset_id: default_native_asset().id(),
             amount: args.amount,
-            fee_asset_id: default_native_asset_id(),
+            fee_asset_id: default_native_asset().id(),
             destination_chain_address: args.destination_chain_address.clone(),
         }),
     )

diff --git a/crates/astria-composer/src/collectors/geth.rs b/crates/astria-composer/src/collectors/geth.rs
@@ -17,7 +17,7 @@ use std::time::Duration;
 
 use astria_core::{
     primitive::v1::{
-        asset::default_native_asset_id,
+        asset::default_native_asset,
         RollupId,
     },
     protocol::transaction::v1alpha1::action::SequenceAction,
@@ -211,7 +211,7 @@ impl Geth {
                         let seq_action = SequenceAction {
                             rollup_id,
                             data,
-                            fee_asset_id: default_native_asset_id(),
+                            fee_asset_id: default_native_asset().id(),
                         };
 
                         metrics::counter!(

diff --git a/crates/astria-composer/src/collectors/grpc.rs b/crates/astria-composer/src/collectors/grpc.rs
@@ -9,7 +9,7 @@ use astria_core::{
         SubmitRollupTransactionResponse,
     },
     primitive::v1::{
-        asset::default_native_asset_id,
+        asset::default_native_asset,
         RollupId,
     },
     protocol::transaction::v1alpha1::action::SequenceAction,
@@ -63,7 +63,7 @@ impl GrpcCollectorService for Grpc {
         let sequence_action = SequenceAction {
             rollup_id,
             data: submit_rollup_tx_request.data,
-            fee_asset_id: default_native_asset_id(),
+            fee_asset_id: default_native_asset().id(),
         };
 
         metrics::counter!(
Original file line number	Diff line number	Diff line change
Expand Up		@@ -13,3 +13,5 @@ target/

		# We don't want to commit the dependency directory of charts
		charts/*/charts

		crates/astria-bridge-withdrawer/src/withdrawer/ethereum/generated/