From c893c69f8d9a8308392cc158f57cd03166b76a25 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 16:24:21 -0400 Subject: [PATCH 01/16] add sudo and withdrawer addresses to bridge account and InitBridgeAccount action --- .../astria.protocol.transactions.v1alpha1.rs | 11 +++ .../protocol/transaction/v1alpha1/action.rs | 41 ++++++++++ ...ransaction_with_every_action_snapshot.snap | 59 ++++++++------- .../src/app/tests_breaking_changes.rs | 2 + .../src/app/tests_execute_transaction.rs | 6 ++ .../src/bridge/init_bridge_account_action.rs | 3 + .../astria-sequencer/src/bridge/state_ext.rs | 74 +++++++++++++++++++ .../transactions/v1alpha1/types.proto | 5 ++ 8 files changed, 171 insertions(+), 30 deletions(-) diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 09b82f6408..669ec62c77 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -299,6 +299,17 @@ pub struct InitBridgeAccountAction { /// the asset used to pay the transaction fee #[prost(bytes = "vec", tag = "3")] pub fee_asset_id: ::prost::alloc::vec::Vec, + /// the address corresponding to the key which has sudo capabilities; + /// ie. can change the sudo and withdrawer addresses for this bridge account. + #[prost(message, optional, tag = "4")] + pub sudo_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, + /// the address corresponding to the key which can withdraw funds from this bridge account. + #[prost(message, optional, tag = "5")] + pub withdrawer_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, } impl ::prost::Name for InitBridgeAccountAction { const NAME: &'static str = "InitBridgeAccountAction"; diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 04034202a9..a7ab60148c 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -1009,6 +1009,13 @@ pub struct InitBridgeAccountAction { pub asset_id: asset::Id, // the fee asset which to pay this action's fees with pub fee_asset_id: asset::Id, + // the address corresponding to the key which has sudo capabilities; + // ie. can change the sudo and withdrawer addresses for this bridge account. + // if unset, this is set to the sender of the transaction. + pub sudo_address: Option
, + // the address corresponding to the key which can withdraw funds from this bridge account. + // if unset, this is set to the sender of the transaction. + pub withdrawer_address: Option
, } impl InitBridgeAccountAction { @@ -1018,6 +1025,8 @@ impl InitBridgeAccountAction { rollup_id: Some(self.rollup_id.to_raw()), asset_id: self.asset_id.get().to_vec(), fee_asset_id: self.fee_asset_id.get().to_vec(), + sudo_address: self.sudo_address.map(Address::into_raw), + withdrawer_address: self.withdrawer_address.map(Address::into_raw), } } @@ -1027,6 +1036,8 @@ impl InitBridgeAccountAction { rollup_id: Some(self.rollup_id.to_raw()), asset_id: self.asset_id.get().to_vec(), fee_asset_id: self.fee_asset_id.get().to_vec(), + sudo_address: self.sudo_address.as_ref().map(Address::to_raw), + withdrawer_address: self.withdrawer_address.as_ref().map(Address::to_raw), } } @@ -1048,11 +1059,25 @@ impl InitBridgeAccountAction { .map_err(InitBridgeAccountActionError::invalid_asset_id)?; let fee_asset_id = asset::Id::try_from_slice(&proto.fee_asset_id) .map_err(InitBridgeAccountActionError::invalid_fee_asset_id)?; + let sudo_address = proto + .sudo_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(InitBridgeAccountActionError::invalid_sudo_address)?; + let withdrawer_address = proto + .withdrawer_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(InitBridgeAccountActionError::invalid_withdrawer_address)?; Ok(Self { rollup_id, asset_id, fee_asset_id, + sudo_address, + withdrawer_address, }) } } @@ -1081,6 +1106,18 @@ impl InitBridgeAccountActionError { fn invalid_fee_asset_id(err: asset::IncorrectAssetIdLength) -> Self { Self(InitBridgeAccountActionErrorKind::InvalidFeeAssetId(err)) } + + #[must_use] + fn invalid_sudo_address(err: IncorrectAddressLength) -> Self { + Self(InitBridgeAccountActionErrorKind::InvalidSudoAddress(err)) + } + + #[must_use] + fn invalid_withdrawer_address(err: IncorrectAddressLength) -> Self { + Self(InitBridgeAccountActionErrorKind::InvalidWithdrawerAddress( + err, + )) + } } // allow pedantic clippy as the errors have the same prefix (for consistency @@ -1097,6 +1134,10 @@ enum InitBridgeAccountActionErrorKind { InvalidAssetId(#[source] asset::IncorrectAssetIdLength), #[error("the `fee_asset_id` field was invalid")] InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), + #[error("the `sudo_address` field was invalid")] + InvalidSudoAddress(#[source] IncorrectAddressLength), + #[error("the `withdrawer_address` field was invalid")] + InvalidWithdrawerAddress(#[source] IncorrectAddressLength), } #[allow(clippy::module_name_repetitions)] diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap index e06db4b717..9621eb4005 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap @@ -1,39 +1,38 @@ --- source: crates/astria-sequencer/src/app/tests_breaking_changes.rs -assertion_line: 274 expression: app.app_hash.as_bytes() --- [ - 30, - 232, - 210, - 6, - 194, - 12, + 165, + 172, + 46, 154, - 179, - 145, - 105, - 40, - 101, - 30, - 224, - 10, - 195, - 119, - 124, - 207, - 182, - 132, - 175, - 9, + 72, + 37, 191, - 104, - 213, + 74, + 77, + 165, + 8, + 99, 200, - 146, - 180, - 192, - 229, - 97 + 106, + 154, + 76, + 234, + 9, + 143, + 107, + 152, + 181, + 71, + 82, + 69, + 20, + 130, + 206, + 237, + 240, + 210, + 249 ] diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index cccf427a2b..0815455101 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -227,6 +227,8 @@ async fn app_execute_transaction_with_every_action_snapshot() { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), asset_id: get_native_asset().id(), fee_asset_id: get_native_asset().id(), + sudo_address: None, + withdrawer_address: None, } .into(), BridgeLockAction { diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 7ea28736c9..0dc1835775 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -608,6 +608,8 @@ async fn app_execute_transaction_init_bridge_account_ok() { rollup_id, asset_id, fee_asset_id: asset_id, + sudo_address: None, + withdrawer_address: None, }; let tx = UnsignedTransaction { params: TransactionParams { @@ -663,6 +665,8 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( rollup_id, asset_id, fee_asset_id: asset_id, + sudo_address: None, + withdrawer_address: None, }; let tx = UnsignedTransaction { params: TransactionParams { @@ -679,6 +683,8 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( rollup_id, asset_id, fee_asset_id: asset_id, + sudo_address: None, + withdrawer_address: None, }; let tx = UnsignedTransaction { params: TransactionParams { diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 86cd28a561..101ba9a302 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -82,6 +82,9 @@ impl ActionHandler for InitBridgeAccountAction { state .put_bridge_account_asset_id(&from, &self.asset_id) .context("failed to put asset ID")?; + state.put_bridge_account_sudo_address(&from, &self.sudo_address.unwrap_or(from)); + state + .put_bridge_account_withdrawer_address(&from, &self.withdrawer_address.unwrap_or(from)); state .decrease_balance(from, self.fee_asset_id, fee) diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index b303733379..dd79d9012f 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -57,6 +57,8 @@ impl From<&asset::Id> for AssetId { struct Fee(u128); const BRIDGE_ACCOUNT_PREFIX: &str = "bridgeacc"; +const BRIDGE_ACCOUNT_SUDO_PREFIX: &str = "bsudo"; +const BRIDGE_ACCOUNT_WITHDRAWER_PREFIX: &str = "bwithdrawer"; const DEPOSIT_PREFIX: &str = "deposit"; const INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY: &str = "initbridgeaccfee"; const BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "bridgelockmultiplier"; @@ -85,6 +87,14 @@ fn deposit_nonce_storage_key(rollup_id: &RollupId) -> Vec { format!("depositnonce/{}", rollup_id.encode_hex::()).into() } +fn bridge_account_sudo_address_storage_key(address: &Address) -> String { + format!("{BRIDGE_ACCOUNT_SUDO_PREFIX}/{address}") +} + +fn bridge_account_withdrawer_address_storage_key(address: &Address) -> String { + format!("{BRIDGE_ACCOUNT_WITHDRAWER_PREFIX}/{address}") +} + #[async_trait] pub(crate) trait StateReadExt: StateRead { #[instrument(skip(self))] @@ -114,6 +124,46 @@ pub(crate) trait StateReadExt: StateRead { Ok(asset_id) } + #[instrument(skip(self))] + async fn get_bridge_account_sudo_address( + &self, + bridge_address: &Address, + ) -> Result> { + let Some(sudo_address_bytes) = self + .get_raw(&bridge_account_sudo_address_storage_key(bridge_address)) + .await + .context("failed reading raw bridge account sudo address from state")? + else { + debug!("bridge account sudo address not found, returning None"); + return Ok(None); + }; + + let sudo_address = + Address::try_from_slice(&sudo_address_bytes).context("invalid sudo address bytes")?; + Ok(Some(sudo_address)) + } + + #[instrument(skip(self))] + async fn get_bridge_account_withdrawer_address( + &self, + bridge_address: &Address, + ) -> Result> { + let Some(withdrawer_address_bytes) = self + .get_raw(&bridge_account_withdrawer_address_storage_key( + bridge_address, + )) + .await + .context("failed reading raw bridge account withdrawer address from state")? + else { + debug!("bridge account withdrawer address not found, returning None"); + return Ok(None); + }; + + let withdrawer_address = Address::try_from_slice(&withdrawer_address_bytes) + .context("invalid withdrawer address bytes")?; + Ok(Some(withdrawer_address)) + } + #[instrument(skip(self))] async fn get_deposit_nonce(&self, rollup_id: &RollupId) -> Result { let bytes = self @@ -229,6 +279,30 @@ pub(crate) trait StateWriteExt: StateWrite { Ok(()) } + #[instrument(skip(self))] + fn put_bridge_account_sudo_address( + &mut self, + bridge_address: &Address, + sudo_address: &Address, + ) { + self.put_raw( + bridge_account_sudo_address_storage_key(bridge_address), + sudo_address.to_vec(), + ); + } + + #[instrument(skip(self))] + fn put_bridge_account_withdrawer_address( + &mut self, + bridge_address: &Address, + withdrawer_address: &Address, + ) { + self.put_raw( + bridge_account_withdrawer_address_storage_key(bridge_address), + withdrawer_address.to_vec(), + ); + } + // the deposit "nonce" for a given rollup ID during a given block. // this is only used to generate storage keys for each of the deposits within a block, // and is reset to 0 at the beginning of each block. diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 17d462f2ff..850c414db8 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -160,6 +160,11 @@ message InitBridgeAccountAction { bytes asset_id = 2; // the asset used to pay the transaction fee bytes fee_asset_id = 3; + // the address corresponding to the key which has sudo capabilities; + // ie. can change the sudo and withdrawer addresses for this bridge account. + astria.primitive.v1.Address sudo_address = 4; + // the address corresponding to the key which can withdraw funds from this bridge account. + astria.primitive.v1.Address withdrawer_address = 5; } // `BridgeLockAction` represents a transaction that transfers From 0df83f023c0fc96dbbec633a70e85ea66162c740 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 16:49:02 -0400 Subject: [PATCH 02/16] update BridgeUnlock to contain a from address, allow withdraws from withdrawer key --- .../astria.protocol.transactions.v1alpha1.rs | 5 +++ .../protocol/transaction/v1alpha1/action.rs | 23 ++++++++++++ .../src/app/tests_breaking_changes.rs | 1 + .../src/app/tests_execute_transaction.rs | 1 + .../src/bridge/bridge_unlock_action.rs | 37 +++++++++++++++---- .../transactions/v1alpha1/types.proto | 4 ++ 6 files changed, 63 insertions(+), 8 deletions(-) diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 669ec62c77..097751f6da 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -370,6 +370,11 @@ pub struct BridgeUnlockAction { /// memo for double spend prevention #[prost(bytes = "vec", tag = "4")] pub memo: ::prost::alloc::vec::Vec, + /// the address of the bridge account to transfer from, + /// if the bridge account's withdrawer address is not the same as the bridge address. + /// if unset, the signer of the transaction is used. + #[prost(message, optional, tag = "5")] + pub from: ::core::option::Option, } impl ::prost::Name for BridgeUnlockAction { const NAME: &'static str = "BridgeUnlockAction"; diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index a7ab60148c..22b803e2c1 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -1047,6 +1047,8 @@ impl InitBridgeAccountAction { /// /// - if the `rollup_id` field is not set /// - if the `rollup_id` field is invalid + /// - if the `sudo_address` field is invalid + /// - if the `withdrawer_address` field is invalid pub fn try_from_raw( proto: raw::InitBridgeAccountAction, ) -> Result { @@ -1260,6 +1262,10 @@ pub struct BridgeUnlockAction { pub fee_asset_id: asset::Id, // memo for double spend protection. pub memo: Vec, + // the address of the bridge account to transfer from, + // if the bridge account's withdrawer address is not the same as the bridge address. + // if unset, the signer of the transaction is used. + pub from: Option
, } impl BridgeUnlockAction { @@ -1270,6 +1276,7 @@ impl BridgeUnlockAction { amount: Some(self.amount.into()), fee_asset_id: self.fee_asset_id.as_ref().to_vec(), memo: self.memo, + from: self.from.map(Address::into_raw), } } @@ -1280,6 +1287,7 @@ impl BridgeUnlockAction { amount: Some(self.amount.into()), fee_asset_id: self.fee_asset_id.as_ref().to_vec(), memo: self.memo.clone(), + from: self.from.as_ref().map(Address::to_raw), } } @@ -1291,6 +1299,7 @@ impl BridgeUnlockAction { /// - if the `to` field is invalid /// - if the `amount` field is invalid /// - if the `fee_asset_id` field is invalid + /// - if the `from` field is invalid pub fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result { let Some(to) = proto.to else { return Err(BridgeUnlockActionError::field_not_set("to")); @@ -1301,11 +1310,18 @@ impl BridgeUnlockAction { .ok_or(BridgeUnlockActionError::missing_amount())?; let fee_asset_id = asset::Id::try_from_slice(&proto.fee_asset_id) .map_err(BridgeUnlockActionError::invalid_fee_asset_id)?; + let from = proto + .from + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(BridgeUnlockActionError::invalid_from_address)?; Ok(Self { to, amount: amount.into(), fee_asset_id, memo: proto.memo, + from, }) } } @@ -1334,6 +1350,11 @@ impl BridgeUnlockActionError { fn invalid_fee_asset_id(err: asset::IncorrectAssetIdLength) -> Self { Self(BridgeUnlockActionErrorKind::InvalidFeeAssetId(err)) } + + #[must_use] + fn invalid_from_address(err: IncorrectAddressLength) -> Self { + Self(BridgeUnlockActionErrorKind::InvalidFromAddress(err)) + } } #[derive(Debug, thiserror::Error)] @@ -1346,6 +1367,8 @@ enum BridgeUnlockActionErrorKind { MissingAmount, #[error("the `fee_asset_id` field was invalid")] InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), + #[error("the `from` field was invalid")] + InvalidFromAddress(#[source] IncorrectAddressLength), } #[derive(Debug, Clone)] diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 0815455101..77573cb2ee 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -261,6 +261,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { amount: 10, fee_asset_id: get_native_asset().id(), memo: vec![0u8; 32], + from: None, } .into(), ], diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 0dc1835775..13915d7417 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1051,6 +1051,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { amount, fee_asset_id: asset_id, memo: b"lilywashere".to_vec(), + from: None, }; let tx = UnsignedTransaction { diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index ba6abcdfad..4cfacd1437 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -28,12 +28,29 @@ impl ActionHandler for BridgeUnlockAction { state: &S, from: Address, ) -> Result<()> { + // the bridge address to withdraw funds from + // if unset, use the tx sender's address + let bridge_address = self.from.unwrap_or(from); + // grab the bridge account's asset let asset_id = state - .get_bridge_account_asset_id(&from) + .get_bridge_account_asset_id(&bridge_address) .await .context("failed to get bridge's asset id, must be a bridge account")?; + // check that the sender of this tx is the authorized withdrawer for the bridge account + // NOTE: this is made backwards-compatible by allowing the + // sudo address to be the bridge address if unset + let withdrawer_address = state + .get_bridge_account_withdrawer_address(&bridge_address) + .await + .context("failed to get bridge account sudo address")? + .unwrap_or(bridge_address); + + if withdrawer_address != from { + anyhow::bail!("unauthorized to unlock bridge account"); + } + let transfer_action = TransferAction { to: self.to, asset_id, @@ -41,16 +58,17 @@ impl ActionHandler for BridgeUnlockAction { fee_asset_id: self.fee_asset_id, }; - // TODO use the BridgeUnlock action's `memo` field. - // this performs the same checks as a normal `TransferAction` - transfer_check_stateful(&transfer_action, state, from).await + transfer_check_stateful(&transfer_action, state, bridge_address).await } #[instrument(skip_all)] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { + // the bridge address to withdraw funds from + let bridge_address = self.from.unwrap_or(from); + let asset_id = state - .get_bridge_account_asset_id(&from) + .get_bridge_account_asset_id(&bridge_address) .await .context("failed to get bridge's asset id, must be a bridge account")?; @@ -62,7 +80,7 @@ impl ActionHandler for BridgeUnlockAction { }; transfer_action - .execute(state, from) + .execute(state, bridge_address) .await .context("failed to execute bridge unlock action as transfer action")?; @@ -102,6 +120,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], + from: None, }; // not a bridge account, should fail @@ -116,7 +135,7 @@ mod test { } #[tokio::test] - async fn bridge_unlock_fee_check_stateful() { + async fn bridge_unlock_fee_check_stateful_from_none() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); @@ -141,6 +160,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], + from: None, }; // not enough balance to transfer asset; should fail @@ -167,7 +187,7 @@ mod test { } #[tokio::test] - async fn bridge_unlock_execute() { + async fn bridge_unlock_execute_from_none() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); @@ -192,6 +212,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], + from: None, }; // not enough balance; should fail diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 850c414db8..f351baefe5 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -200,6 +200,10 @@ message BridgeUnlockAction { bytes fee_asset_id = 3; // memo for double spend prevention bytes memo = 4; + // the address of the bridge account to transfer from, + // if the bridge account's withdrawer address is not the same as the bridge address. + // if unset, the signer of the transaction is used. + astria.primitive.v1.Address from = 5; } message FeeChangeAction { From 5e187587969f29339ee909d1dc4ef7843ac86c51 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 21:29:21 -0400 Subject: [PATCH 03/16] add unit tests and sudo change fee --- .../astria.protocol.transactions.v1alpha1.rs | 33 ++- .../protocol/transaction/v1alpha1/action.rs | 134 ++++++++++++ ...ransaction_with_every_action_snapshot.snap | 64 +++--- ..._changes__app_finalize_block_snapshot.snap | 64 +++--- ...reaking_changes__app_genesis_snapshot.snap | 62 +++--- crates/astria-sequencer/src/app/test_utils.rs | 1 + .../src/app/tests_breaking_changes.rs | 76 ++++--- .../src/bridge/bridge_sudo_change_action.rs | 181 +++++++++++++++++ .../src/bridge/bridge_unlock_action.rs | 191 +++++++++++++++++- .../astria-sequencer/src/bridge/component.rs | 1 + crates/astria-sequencer/src/bridge/mod.rs | 1 + .../astria-sequencer/src/bridge/state_ext.rs | 26 ++- crates/astria-sequencer/src/genesis.rs | 2 + .../src/ibc/ics20_withdrawal.rs | 2 + .../src/transaction/checks.rs | 18 +- .../astria-sequencer/src/transaction/mod.rs | 37 ++-- .../test-genesis-app-state.json | 1 + .../transactions/v1alpha1/types.proto | 14 +- 18 files changed, 760 insertions(+), 148 deletions(-) create mode 100644 crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 097751f6da..dde98e036f 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -59,7 +59,7 @@ impl ::prost::Name for TransactionParams { pub struct Action { #[prost( oneof = "action::Value", - tags = "1, 2, 11, 12, 13, 21, 22, 50, 51, 52, 53, 55" + tags = "1, 2, 11, 12, 13, 14, 21, 22, 50, 51, 52, 53, 55" )] pub value: ::core::option::Option, } @@ -80,6 +80,8 @@ pub mod action { BridgeLockAction(super::BridgeLockAction), #[prost(message, tag = "13")] BridgeUnlockAction(super::BridgeUnlockAction), + #[prost(message, tag = "14")] + BridgeSudoChangeAction(super::BridgeSudoChangeAction), /// IBC user actions are defined on 21-30 #[prost(message, tag = "21")] IbcAction(::penumbra_proto::core::component::ibc::v1::IbcRelay), @@ -385,6 +387,35 @@ impl ::prost::Name for BridgeUnlockAction { } #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] +pub struct BridgeSudoChangeAction { + /// the address of the bridge account to change the sudo or withdrawer addresses for + #[prost(message, optional, tag = "1")] + pub bridge_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, + /// the new sudo address; unchanged if unset + #[prost(message, optional, tag = "2")] + pub new_sudo_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, + /// the new withdrawer address; unchanged if unset + #[prost(message, optional, tag = "3")] + pub new_withdrawer_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, + /// the asset used to pay the transaction fee + #[prost(bytes = "vec", tag = "4")] + pub fee_asset_id: ::prost::alloc::vec::Vec, +} +impl ::prost::Name for BridgeSudoChangeAction { + const NAME: &'static str = "BridgeSudoChangeAction"; + const PACKAGE: &'static str = "astria.protocol.transactions.v1alpha1"; + fn full_name() -> ::prost::alloc::string::String { + ::prost::alloc::format!("astria.protocol.transactions.v1alpha1.{}", Self::NAME) + } +} +#[allow(clippy::derive_partial_eq_without_eq)] +#[derive(Clone, PartialEq, ::prost::Message)] pub struct FeeChangeAction { /// note that the proto number ranges are doubled from that of `Action`. /// this to accomodate both `base_fee` and `byte_cost_multiplier` for each action. diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 22b803e2c1..1c0f342364 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -36,6 +36,7 @@ pub enum Action { InitBridgeAccount(InitBridgeAccountAction), BridgeLock(BridgeLockAction), BridgeUnlock(BridgeUnlockAction), + BridgeSudoChange(BridgeSudoChangeAction), FeeChange(FeeChangeAction), } @@ -55,6 +56,7 @@ impl Action { Action::InitBridgeAccount(act) => Value::InitBridgeAccountAction(act.into_raw()), Action::BridgeLock(act) => Value::BridgeLockAction(act.into_raw()), Action::BridgeUnlock(act) => Value::BridgeUnlockAction(act.into_raw()), + Action::BridgeSudoChange(act) => Value::BridgeSudoChangeAction(act.into_raw()), Action::FeeChange(act) => Value::FeeChangeAction(act.into_raw()), }; raw::Action { @@ -79,6 +81,7 @@ impl Action { Action::InitBridgeAccount(act) => Value::InitBridgeAccountAction(act.to_raw()), Action::BridgeLock(act) => Value::BridgeLockAction(act.to_raw()), Action::BridgeUnlock(act) => Value::BridgeUnlockAction(act.to_raw()), + Action::BridgeSudoChange(act) => Value::BridgeSudoChangeAction(act.to_raw()), Action::FeeChange(act) => Value::FeeChangeAction(act.to_raw()), }; raw::Action { @@ -137,6 +140,10 @@ impl Action { Value::BridgeUnlockAction(act) => Self::BridgeUnlock( BridgeUnlockAction::try_from_raw(act).map_err(ActionError::bridge_unlock)?, ), + Value::BridgeSudoChangeAction(act) => Self::BridgeSudoChange( + BridgeSudoChangeAction::try_from_raw(act) + .map_err(ActionError::bridge_sudo_change)?, + ), Value::FeeChangeAction(act) => Self::FeeChange( FeeChangeAction::try_from_raw(&act).map_err(ActionError::fee_change)?, ), @@ -221,6 +228,12 @@ impl From for Action { } } +impl From for Action { + fn from(value: BridgeSudoChangeAction) -> Self { + Self::BridgeSudoChange(value) + } +} + impl From for Action { fn from(value: FeeChangeAction) -> Self { Self::FeeChange(value) @@ -281,6 +294,10 @@ impl ActionError { Self(ActionErrorKind::BridgeUnlock(inner)) } + fn bridge_sudo_change(inner: BridgeSudoChangeActionError) -> Self { + Self(ActionErrorKind::BridgeSudoChange(inner)) + } + fn fee_change(inner: FeeChangeActionError) -> Self { Self(ActionErrorKind::FeeChange(inner)) } @@ -312,6 +329,8 @@ enum ActionErrorKind { BridgeLock(#[source] BridgeLockActionError), #[error("bridge unlock action was not valid")] BridgeUnlock(#[source] BridgeUnlockActionError), + #[error("bridge sudo change action was not valid")] + BridgeSudoChange(#[source] BridgeSudoChangeActionError), #[error("fee change action was not valid")] FeeChange(#[source] FeeChangeActionError), } @@ -1371,6 +1390,121 @@ enum BridgeUnlockActionErrorKind { InvalidFromAddress(#[source] IncorrectAddressLength), } +#[allow(clippy::module_name_repetitions)] +#[derive(Debug, Clone)] +pub struct BridgeSudoChangeAction { + pub bridge_address: Address, + pub new_sudo_address: Option
, + pub new_withdrawer_address: Option
, + pub fee_asset_id: asset::Id, +} + +impl BridgeSudoChangeAction { + #[must_use] + pub fn into_raw(self) -> raw::BridgeSudoChangeAction { + raw::BridgeSudoChangeAction { + bridge_address: Some(self.bridge_address.to_raw()), + new_sudo_address: self.new_sudo_address.map(Address::into_raw), + new_withdrawer_address: self.new_withdrawer_address.map(Address::into_raw), + fee_asset_id: self.fee_asset_id.get().to_vec(), + } + } + + #[must_use] + pub fn to_raw(&self) -> raw::BridgeSudoChangeAction { + raw::BridgeSudoChangeAction { + bridge_address: Some(self.bridge_address.to_raw()), + new_sudo_address: self.new_sudo_address.as_ref().map(Address::to_raw), + new_withdrawer_address: self.new_withdrawer_address.as_ref().map(Address::to_raw), + fee_asset_id: self.fee_asset_id.get().to_vec(), + } + } + + /// Convert from a raw, unchecked protobuf [`raw::BridgeSudoChangeAction`]. + /// + /// # Errors + /// + /// - if the `bridge_address` field is not set + /// - if the `bridge_address` field is invalid + /// - if the `new_sudo_address` field is invalid + /// - if the `new_withdrawer_address` field is invalid + pub fn try_from_raw( + proto: raw::BridgeSudoChangeAction, + ) -> Result { + let Some(bridge_address) = proto.bridge_address else { + return Err(BridgeSudoChangeActionError::field_not_set("bridge_address")); + }; + let bridge_address = Address::try_from_raw(&bridge_address) + .map_err(BridgeSudoChangeActionError::invalid_bridge_address)?; + let new_sudo_address = proto + .new_sudo_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(BridgeSudoChangeActionError::invalid_new_sudo_address)?; + let new_withdrawer_address = proto + .new_withdrawer_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(BridgeSudoChangeActionError::invalid_new_withdrawer_address)?; + let fee_asset_id = asset::Id::try_from_slice(&proto.fee_asset_id) + .map_err(BridgeSudoChangeActionError::invalid_fee_asset_id)?; + + Ok(Self { + bridge_address, + new_sudo_address, + new_withdrawer_address, + fee_asset_id, + }) + } +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct BridgeSudoChangeActionError(BridgeSudoChangeActionErrorKind); + +impl BridgeSudoChangeActionError { + #[must_use] + fn field_not_set(field: &'static str) -> Self { + Self(BridgeSudoChangeActionErrorKind::FieldNotSet(field)) + } + + #[must_use] + fn invalid_bridge_address(err: IncorrectAddressLength) -> Self { + Self(BridgeSudoChangeActionErrorKind::InvalidBridgeAddress(err)) + } + + #[must_use] + fn invalid_new_sudo_address(err: IncorrectAddressLength) -> Self { + Self(BridgeSudoChangeActionErrorKind::InvalidNewSudoAddress(err)) + } + + #[must_use] + fn invalid_new_withdrawer_address(err: IncorrectAddressLength) -> Self { + Self(BridgeSudoChangeActionErrorKind::InvalidNewWithdrawerAddress(err)) + } + + #[must_use] + fn invalid_fee_asset_id(err: asset::IncorrectAssetIdLength) -> Self { + Self(BridgeSudoChangeActionErrorKind::InvalidFeeAssetId(err)) + } +} + +#[derive(Debug, thiserror::Error)] +enum BridgeSudoChangeActionErrorKind { + #[error("the expected field in the raw source type was not set: `{0}`")] + FieldNotSet(&'static str), + #[error("the `bridge_address` field was invalid")] + InvalidBridgeAddress(#[source] IncorrectAddressLength), + #[error("the `new_sudo_address` field was invalid")] + InvalidNewSudoAddress(#[source] IncorrectAddressLength), + #[error("the `new_withdrawer_address` field was invalid")] + InvalidNewWithdrawerAddress(#[source] IncorrectAddressLength), + #[error("the `fee_asset_id` field was invalid")] + InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), +} + #[derive(Debug, Clone)] pub enum FeeChange { TransferBaseFee, diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap index 9621eb4005..dd88e3d0eb 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap @@ -3,36 +3,36 @@ source: crates/astria-sequencer/src/app/tests_breaking_changes.rs expression: app.app_hash.as_bytes() --- [ - 165, - 172, - 46, - 154, - 72, - 37, - 191, - 74, - 77, - 165, - 8, - 99, - 200, - 106, - 154, - 76, - 234, - 9, - 143, - 107, - 152, - 181, - 71, - 82, - 69, - 20, - 130, - 206, - 237, - 240, - 210, - 249 + 230, + 183, + 103, + 137, + 250, + 198, + 226, + 41, + 80, + 149, + 115, + 44, + 145, + 68, + 160, + 86, + 33, + 83, + 81, + 255, + 91, + 220, + 49, + 68, + 92, + 93, + 220, + 155, + 93, + 66, + 187, + 30 ] diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap index 9c81d4d7ae..27f6380cad 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap @@ -3,36 +3,36 @@ source: crates/astria-sequencer/src/app/tests_breaking_changes.rs expression: app.app_hash.as_bytes() --- [ - 171, - 98, - 3, - 172, - 104, - 125, - 77, - 86, - 230, - 236, - 153, - 226, - 227, - 158, - 59, - 19, - 108, - 136, - 6, - 56, - 175, - 113, - 196, - 108, - 171, - 66, - 211, - 241, - 217, - 82, - 191, - 152 + 109, + 155, + 33, + 202, + 146, + 131, + 180, + 52, + 26, + 145, + 67, + 231, + 44, + 32, + 133, + 23, + 8, + 44, + 142, + 102, + 149, + 214, + 152, + 200, + 221, + 129, + 85, + 255, + 254, + 135, + 123, + 148 ] diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_genesis_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_genesis_snapshot.snap index da9e648bf0..50e733cecc 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_genesis_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_genesis_snapshot.snap @@ -3,36 +3,36 @@ source: crates/astria-sequencer/src/app/tests_breaking_changes.rs expression: app.app_hash.as_bytes() --- [ - 93, - 117, - 49, + 9, + 0, + 255, + 231, + 184, + 194, + 27, + 158, + 152, + 223, + 174, + 96, + 156, + 162, + 246, + 226, + 186, + 211, + 188, + 166, + 27, + 221, + 218, 7, - 244, - 114, - 95, - 76, - 26, - 53, - 191, - 78, - 217, - 100, - 131, - 48, - 137, - 180, - 89, - 93, - 56, - 249, - 56, - 247, - 253, - 10, - 113, - 108, - 87, - 250, - 68, - 145 + 109, + 104, + 172, + 49, + 20, + 96, + 204, + 88 ] diff --git a/crates/astria-sequencer/src/app/test_utils.rs b/crates/astria-sequencer/src/app/test_utils.rs index 09c42c2f01..b148de4c2b 100644 --- a/crates/astria-sequencer/src/app/test_utils.rs +++ b/crates/astria-sequencer/src/app/test_utils.rs @@ -86,6 +86,7 @@ pub(crate) fn default_fees() -> genesis::Fees { sequence_byte_cost_multiplier: 1, init_bridge_account_base_fee: 48, bridge_lock_byte_cost_multiplier: 1, + bridge_sudo_change_fee: 24, ics20_withdrawal_base_fee: 24, } } diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 77573cb2ee..a0cdbab1a0 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -23,6 +23,7 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::{ BridgeLockAction, + BridgeSudoChangeAction, BridgeUnlockAction, IbcRelayerChangeAction, SequenceAction, @@ -162,13 +163,20 @@ async fn app_execute_transaction_with_every_action_snapshot() { }, }; + use crate::genesis::Account; + let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); let (bridge_signing_key, bridge_address) = get_bridge_signing_key_and_address(); let bob_address = address_from_hex_string(BOB_ADDRESS); let carol_address = address_from_hex_string(CAROL_ADDRESS); + let mut accounts = default_genesis_accounts(); + accounts.push(Account { + address: bridge_address, + balance: 1_000_000_000, + }); let genesis_state = GenesisState { - accounts: default_genesis_accounts(), + accounts, authority_sudo_address: alice_address, ibc_sudo_address: alice_address, ibc_relayer_addresses: vec![], @@ -186,15 +194,8 @@ async fn app_execute_transaction_with_every_action_snapshot() { power: 100u32.into(), }; - // setup for BridgeLockAction let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let asset_id = get_native_asset().id(); - let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state_tx - .put_bridge_account_asset_id(&bridge_address, &asset_id) - .unwrap(); - app.apply(state_tx); let tx = UnsignedTransaction { params: TransactionParams { @@ -205,14 +206,14 @@ async fn app_execute_transaction_with_every_action_snapshot() { TransferAction { to: bob_address, amount: 333_333, - asset_id: get_native_asset().id(), - fee_asset_id: get_native_asset().id(), + asset_id, + fee_asset_id: asset_id, } .into(), SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data: b"hello world".to_vec(), - fee_asset_id: get_native_asset().id(), + fee_asset_id: asset_id, } .into(), Action::ValidatorUpdate(update.clone()), @@ -223,22 +224,6 @@ async fn app_execute_transaction_with_every_action_snapshot() { FeeAssetChangeAction::Addition(asset::Id::from("test-0".to_string())).into(), FeeAssetChangeAction::Addition(asset::Id::from("test-1".to_string())).into(), FeeAssetChangeAction::Removal(asset::Id::from("test-0".to_string())).into(), - InitBridgeAccountAction { - rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), - asset_id: get_native_asset().id(), - fee_asset_id: get_native_asset().id(), - sudo_address: None, - withdrawer_address: None, - } - .into(), - BridgeLockAction { - to: bridge_address, - amount: 100, - asset_id: get_native_asset().id(), - fee_asset_id: get_native_asset().id(), - destination_chain_address: "nootwashere".to_string(), - } - .into(), SudoAddressChangeAction { new_address: bob_address, } @@ -249,21 +234,54 @@ async fn app_execute_transaction_with_every_action_snapshot() { let signed_tx = Arc::new(tx.into_signed(&alice_signing_key)); app.execute_transaction(signed_tx).await.unwrap(); - // execute BridgeUnlock action let tx = UnsignedTransaction { params: TransactionParams { nonce: 0, chain_id: "test".to_string(), }, actions: vec![ + InitBridgeAccountAction { + rollup_id, + asset_id, + fee_asset_id: asset_id, + sudo_address: None, + withdrawer_address: None, + } + .into(), + ], + }; + let signed_tx = Arc::new(tx.into_signed(&bridge_signing_key)); + app.execute_transaction(signed_tx).await.unwrap(); + + let tx = UnsignedTransaction { + params: TransactionParams { + nonce: 1, + chain_id: "test".to_string(), + }, + actions: vec![ + BridgeLockAction { + to: bridge_address, + amount: 100, + asset_id, + fee_asset_id: asset_id, + destination_chain_address: "nootwashere".to_string(), + } + .into(), BridgeUnlockAction { to: bob_address, amount: 10, - fee_asset_id: get_native_asset().id(), + fee_asset_id: asset_id, memo: vec![0u8; 32], from: None, } .into(), + BridgeSudoChangeAction { + bridge_address, + new_sudo_address: Some(bob_address), + new_withdrawer_address: Some(bob_address), + fee_asset_id: asset_id, + } + .into(), ], }; diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs new file mode 100644 index 0000000000..2e857e35a5 --- /dev/null +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -0,0 +1,181 @@ +use anyhow::{ + ensure, + Context as _, + Result, +}; +use astria_core::{ + primitive::v1::Address, + protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, +}; +use tracing::instrument; + +use crate::{ + accounts::state_ext::StateWriteExt as _, + bridge::state_ext::{ + StateReadExt as _, + StateWriteExt as _, + }, + state_ext::{ + StateReadExt, + StateWriteExt, + }, + transaction::action_handler::ActionHandler, +}; + +#[async_trait::async_trait] +impl ActionHandler for BridgeSudoChangeAction { + async fn check_stateful( + &self, + state: &S, + from: Address, + ) -> Result<()> { + ensure!( + state + .is_allowed_fee_asset(self.fee_asset_id) + .await + .context("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + // check that the sender of this tx is the authorized sudo address for the bridge account + let Some(sudo_address) = state + .get_bridge_account_sudo_address(&self.bridge_address) + .await + .context("failed to get bridge account sudo address")? + else { + // TODO: if the sudo address is unset, should we still allow this action + // if the sender if the bridge address itself? + anyhow::bail!("bridge account does not have an associated sudo address"); + }; + + ensure!( + sudo_address == from, + "unauthorized for bridge sudo change action", + ); + + Ok(()) + } + + #[instrument(skip_all)] + async fn execute(&self, state: &mut S, _: Address) -> Result<()> { + let fee = state + .get_bridge_sudo_change_fee() + .await + .context("failed to get bridge sudo change fee")?; + state + .decrease_balance(self.bridge_address, self.fee_asset_id, fee) + .await + .context("failed to decrease balance for bridge sudo change fee")?; + + if let Some(sudo_address) = self.new_sudo_address { + state.put_bridge_account_sudo_address(&self.bridge_address, &sudo_address); + } + + if let Some(withdrawer_address) = self.new_withdrawer_address { + state.put_bridge_account_withdrawer_address(&self.bridge_address, &withdrawer_address); + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use astria_core::primitive::v1::asset::Id; + use cnidarium::StateDelta; + + use super::*; + + #[tokio::test] + async fn bridge_sudo_change_check_stateless_ok() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = Id::from_denom("test"); + state.put_allowed_fee_asset(asset_id); + + let bridge_address = Address::from([99; 20]); + let sudo_address = Address::from([98; 20]); + state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); + + let action = BridgeSudoChangeAction { + bridge_address, + new_sudo_address: None, + new_withdrawer_address: None, + fee_asset_id: asset_id, + }; + + action.check_stateful(&state, sudo_address).await.unwrap(); + } + + #[tokio::test] + async fn bridge_sudo_change_check_stateless_unauthorized() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = Id::from_denom("test"); + state.put_allowed_fee_asset(asset_id); + + let bridge_address = Address::from([99; 20]); + let sudo_address = Address::from([98; 20]); + state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); + + let action = BridgeSudoChangeAction { + bridge_address, + new_sudo_address: None, + new_withdrawer_address: None, + fee_asset_id: asset_id, + }; + + assert!( + action + .check_stateful(&state, bridge_address) + .await + .unwrap_err() + .to_string() + .contains("unauthorized for bridge sudo change action") + ); + } + + #[tokio::test] + async fn bridge_sudo_change_execute_ok() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + state.put_bridge_sudo_change_fee(10); + + let fee_asset_id = Id::from_denom("test"); + let bridge_address = Address::from([99; 20]); + let new_sudo_address = Address::from([97; 20]); + let new_withdrawer_address = Address::from([96; 20]); + state + .put_account_balance(bridge_address, fee_asset_id, 10) + .unwrap(); + + let action = BridgeSudoChangeAction { + bridge_address, + new_sudo_address: Some(new_sudo_address), + new_withdrawer_address: Some(new_withdrawer_address), + fee_asset_id, + }; + + action.execute(&mut state, bridge_address).await.unwrap(); + + assert_eq!( + state + .get_bridge_account_sudo_address(&bridge_address) + .await + .unwrap(), + Some(new_sudo_address), + ); + assert_eq!( + state + .get_bridge_account_withdrawer_address(&bridge_address) + .await + .unwrap(), + Some(new_withdrawer_address), + ); + } +} diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 4cfacd1437..0b03790dc5 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -1,4 +1,5 @@ use anyhow::{ + ensure, Context as _, Result, }; @@ -44,12 +45,13 @@ impl ActionHandler for BridgeUnlockAction { let withdrawer_address = state .get_bridge_account_withdrawer_address(&bridge_address) .await - .context("failed to get bridge account sudo address")? + .context("failed to get bridge account withdrawer address")? .unwrap_or(bridge_address); - if withdrawer_address != from { - anyhow::bail!("unauthorized to unlock bridge account"); - } + ensure!( + withdrawer_address == from, + "unauthorized to unlock bridge account", + ); let transfer_action = TransferAction { to: self.to, @@ -134,6 +136,80 @@ mod test { ); } + #[tokio::test] + async fn bridge_unlock_fail_withdrawer_unset_invalid_withdrawer() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_amount = 100; + + let sender_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + + let bridge_address = Address::from([3; 20]); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + from: Some(bridge_address), + }; + + // invalid sender, doesn't match action's `from`, should fail + assert!( + bridge_unlock + .check_stateful(&state, sender_address) + .await + .unwrap_err() + .to_string() + .contains("unauthorized to unlock bridge account") + ); + } + + #[tokio::test] + async fn bridge_unlock_fail_withdrawer_set_invalid_withdrawer() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_amount = 100; + + let sender_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + + let bridge_address = Address::from([3; 20]); + let withdrawer_address = Address::from([4; 20]); + state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + from: Some(bridge_address), + }; + + // invalid sender, doesn't match action's bridge account's withdrawer, should fail + assert!( + bridge_unlock + .check_stateful(&state, sender_address) + .await + .unwrap_err() + .to_string() + .contains("unauthorized to unlock bridge account") + ); + } + #[tokio::test] async fn bridge_unlock_fee_check_stateful_from_none() { let storage = cnidarium::TempStorage::new().await.unwrap(); @@ -186,6 +262,61 @@ mod test { .unwrap(); } + #[tokio::test] + async fn bridge_unlock_fee_check_stateful_from_some() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_fee = 10; + let transfer_amount = 100; + state.put_transfer_base_fee(transfer_fee).unwrap(); + + let bridge_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let withdrawer_address = Address::from([4; 20]); + state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + from: Some(bridge_address), + }; + + // not enough balance to transfer asset; should fail + state + .put_account_balance(bridge_address, asset_id, transfer_amount) + .unwrap(); + assert!( + bridge_unlock + .check_stateful(&state, withdrawer_address) + .await + .unwrap_err() + .to_string() + .contains("insufficient funds for transfer and fee payment") + ); + + // enough balance; should pass + state + .put_account_balance(bridge_address, asset_id, transfer_amount + transfer_fee) + .unwrap(); + bridge_unlock + .check_stateful(&state, withdrawer_address) + .await + .unwrap(); + } + #[tokio::test] async fn bridge_unlock_execute_from_none() { let storage = cnidarium::TempStorage::new().await.unwrap(); @@ -237,4 +368,56 @@ mod test { .await .unwrap(); } + + #[tokio::test] + async fn bridge_unlock_execute_from_some() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_fee = 10; + let transfer_amount = 100; + state.put_transfer_base_fee(transfer_fee).unwrap(); + + let bridge_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + from: Some(bridge_address), + }; + + // not enough balance; should fail + state + .put_account_balance(bridge_address, asset_id, transfer_amount) + .unwrap(); + assert!( + bridge_unlock + .execute(&mut state, bridge_address) + .await + .unwrap_err() + .to_string() + .eq("failed to execute bridge unlock action as transfer action") + ); + + // enough balance; should pass + state + .put_account_balance(bridge_address, asset_id, transfer_amount + transfer_fee) + .unwrap(); + bridge_unlock + .execute(&mut state, bridge_address) + .await + .unwrap(); + } } diff --git a/crates/astria-sequencer/src/bridge/component.rs b/crates/astria-sequencer/src/bridge/component.rs index b9e27ba32a..509d109566 100644 --- a/crates/astria-sequencer/src/bridge/component.rs +++ b/crates/astria-sequencer/src/bridge/component.rs @@ -24,6 +24,7 @@ impl Component for BridgeComponent { async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { state.put_init_bridge_account_base_fee(app_state.fees.init_bridge_account_base_fee); state.put_bridge_lock_byte_cost_multiplier(app_state.fees.bridge_lock_byte_cost_multiplier); + state.put_bridge_sudo_change_fee(app_state.fees.bridge_sudo_change_fee); Ok(()) } diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index 6d2570f725..1809c62424 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -1,4 +1,5 @@ mod bridge_lock_action; +mod bridge_sudo_change_action; mod bridge_unlock_action; pub(crate) mod component; pub(crate) mod init_bridge_account_action; diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index dd79d9012f..9f4b67986b 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -62,6 +62,7 @@ const BRIDGE_ACCOUNT_WITHDRAWER_PREFIX: &str = "bwithdrawer"; const DEPOSIT_PREFIX: &str = "deposit"; const INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY: &str = "initbridgeaccfee"; const BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "bridgelockmultiplier"; +const BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY: &str = "bridgesudofee"; fn storage_key(address: &str) -> String { format!("{BRIDGE_ACCOUNT_PREFIX}/{address}") @@ -118,9 +119,9 @@ pub(crate) trait StateReadExt: StateRead { let bytes = self .get_raw(&asset_id_storage_key(address)) .await - .context("failed reading raw asset IDs from state")? - .ok_or_else(|| anyhow!("asset IDs not found"))?; - let asset_id = asset::Id::try_from_slice(&bytes).context("invalid asset IDs bytes")?; + .context("failed reading raw asset ID from state")? + .ok_or_else(|| anyhow!("asset ID not found"))?; + let asset_id = asset::Id::try_from_slice(&bytes).context("invalid asset ID bytes")?; Ok(asset_id) } @@ -255,6 +256,17 @@ pub(crate) trait StateReadExt: StateRead { let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; Ok(fee) } + + #[instrument(skip(self))] + async fn get_bridge_sudo_change_fee(&self) -> Result { + let bytes = self + .get_raw(BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY) + .await + .context("failed reading raw bridge sudo change fee from state")? + .ok_or_else(|| anyhow!("bridge sudo change fee not found"))?; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } } impl StateReadExt for T {} @@ -366,6 +378,14 @@ pub(crate) trait StateWriteExt: StateWrite { borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), ); } + + #[instrument(skip(self))] + fn put_bridge_sudo_change_fee(&mut self, fee: u128) { + self.put_raw( + BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), + ); + } } impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/genesis.rs b/crates/astria-sequencer/src/genesis.rs index f6972f8412..1a6348474c 100644 --- a/crates/astria-sequencer/src/genesis.rs +++ b/crates/astria-sequencer/src/genesis.rs @@ -32,6 +32,7 @@ pub(crate) struct Fees { pub(crate) sequence_byte_cost_multiplier: u128, pub(crate) init_bridge_account_base_fee: u128, pub(crate) bridge_lock_byte_cost_multiplier: u128, + pub(crate) bridge_sudo_change_fee: u128, pub(crate) ics20_withdrawal_base_fee: u128, } @@ -118,6 +119,7 @@ mod test { "sequence_byte_cost_multiplier": 1, "init_bridge_account_base_fee": 48, "bridge_lock_byte_cost_multiplier": 1, + "bridge_sudo_change_fee": 24, "ics20_withdrawal_base_fee": 24 }, "native_asset_base_denomination": "nria", diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 55377de807..9eceeee8bf 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -69,6 +69,8 @@ impl ActionHandler for action::Ics20Withdrawal { state: &S, from: Address, ) -> Result<()> { + // TODO: bridge withdrawer address check + let fee = state .get_ics20_withdrawal_base_fee() .await diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index a2074e0e2b..e7877472d2 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -90,6 +90,10 @@ pub(crate) async fn check_balance_for_total_fees( .get_bridge_lock_byte_cost_multiplier() .await .context("failed to get bridge lock byte cost multiplier")?; + let bridge_sudo_change_fee = state + .get_bridge_sudo_change_fee() + .await + .context("failed to get bridge sudo change fee")?; let mut fees_by_asset = HashMap::new(); for action in &tx.actions { @@ -127,7 +131,7 @@ pub(crate) async fn check_balance_for_total_fees( Action::BridgeUnlock(act) => { bridge_unlock_update_fees( state, - from, + act.from.unwrap_or(from), act.amount, act.fee_asset_id, &mut fees_by_asset, @@ -135,6 +139,12 @@ pub(crate) async fn check_balance_for_total_fees( ) .await?; } + Action::BridgeSudoChange(act) => { + fees_by_asset + .entry(act.fee_asset_id) + .and_modify(|amt| *amt = amt.saturating_add(bridge_sudo_change_fee)) + .or_insert(bridge_sudo_change_fee); + } Action::ValidatorUpdate(_) | Action::SudoAddressChange(_) | Action::Ibc(_) @@ -242,14 +252,14 @@ fn bridge_lock_update_fees( async fn bridge_unlock_update_fees( state: &S, - from: Address, + bridge_address: Address, amount: u128, fee_asset_id: asset::Id, fees_by_asset: &mut HashMap, transfer_fee: u128, ) -> anyhow::Result<()> { let asset_id = state - .get_bridge_account_asset_id(&from) + .get_bridge_account_asset_id(&bridge_address) .await .context("must be a bridge account for BridgeUnlock action")?; fees_by_asset @@ -305,6 +315,7 @@ mod test { state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); state_tx.put_init_bridge_account_base_fee(12); state_tx.put_bridge_lock_byte_cost_multiplier(1); + state_tx.put_bridge_sudo_change_fee(24); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); @@ -371,6 +382,7 @@ mod test { state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); state_tx.put_init_bridge_account_base_fee(12); state_tx.put_bridge_lock_byte_cost_multiplier(1); + state_tx.put_bridge_sudo_change_fee(24); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 1eaa5f2a30..b6020b77cc 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -116,6 +116,10 @@ impl ActionHandler for UnsignedTransaction { .check_stateless() .await .context("stateless check failed for SudoAddressChangeAction")?, + Action::FeeChange(act) => act + .check_stateless() + .await + .context("stateless check failed for FeeChangeAction")?, Action::Ibc(act) => { let action = act .clone() @@ -145,14 +149,14 @@ impl ActionHandler for UnsignedTransaction { .check_stateless() .await .context("stateless check failed for BridgeLockAction")?, - Action::FeeChange(act) => act - .check_stateless() - .await - .context("stateless check failed for FeeChangeAction")?, Action::BridgeUnlock(act) => act .check_stateless() .await .context("stateless check failed for BridgeLockAction")?, + Action::BridgeSudoChange(act) => act + .check_stateless() + .await + .context("stateless check failed for BridgeSudoChangeAction")?, } } Ok(()) @@ -199,6 +203,10 @@ impl ActionHandler for UnsignedTransaction { .check_stateful(state, from) .await .context("stateful check failed for SudoAddressChangeAction")?, + Action::FeeChange(act) => act + .check_stateful(state, from) + .await + .context("stateful check failed for FeeChangeAction")?, Action::Ibc(_) => { ensure!( state @@ -228,14 +236,14 @@ impl ActionHandler for UnsignedTransaction { .check_stateful(state, from) .await .context("stateful check failed for BridgeLockAction")?, - Action::FeeChange(act) => act - .check_stateful(state, from) - .await - .context("stateful check failed for FeeChangeAction")?, Action::BridgeUnlock(act) => act .check_stateful(state, from) .await .context("stateful check failed for BridgeUnlockAction")?, + Action::BridgeSudoChange(act) => act + .check_stateful(state, from) + .await + .context("stateful check failed for BridgeSudoChangeAction")?, } } @@ -283,6 +291,11 @@ impl ActionHandler for UnsignedTransaction { .await .context("execution failed for SudoAddressChangeAction")?; } + Action::FeeChange(act) => { + act.execute(state, from) + .await + .context("execution failed for FeeChangeAction")?; + } Action::Ibc(act) => { let action = act .clone() @@ -317,15 +330,15 @@ impl ActionHandler for UnsignedTransaction { .await .context("execution failed for BridgeLockAction")?; } - Action::FeeChange(act) => { + Action::BridgeUnlock(act) => { act.execute(state, from) .await - .context("execution failed for FeeChangeAction")?; + .context("execution failed for BridgeUnlockAction")?; } - Action::BridgeUnlock(act) => { + Action::BridgeSudoChange(act) => { act.execute(state, from) .await - .context("execution failed for BridgeUnlockAction")?; + .context("execution failed for BridgeSudoChangeAction")?; } } } diff --git a/crates/astria-sequencer/test-genesis-app-state.json b/crates/astria-sequencer/test-genesis-app-state.json index 5a7bc86189..87f6879ecb 100644 --- a/crates/astria-sequencer/test-genesis-app-state.json +++ b/crates/astria-sequencer/test-genesis-app-state.json @@ -27,6 +27,7 @@ "sequence_byte_cost_multiplier": 1, "init_bridge_account_base_fee": 48, "bridge_lock_byte_cost_multiplier": 1, + "bridge_sudo_change_fee": 24, "ics20_withdrawal_base_fee": 24 }, "native_asset_base_denomination": "nria", diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index f351baefe5..470335e1be 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -42,6 +42,7 @@ message Action { InitBridgeAccountAction init_bridge_account_action = 11; BridgeLockAction bridge_lock_action = 12; BridgeUnlockAction bridge_unlock_action = 13; + BridgeSudoChangeAction bridge_sudo_change_action = 14; // IBC user actions are defined on 21-30 astria_vendored.penumbra.core.component.ibc.v1.IbcRelay ibc_action = 21; @@ -55,7 +56,7 @@ message Action { FeeChangeAction fee_change_action = 55; } reserved 3 to 10; - reserved 14 to 20; + reserved 15 to 20; reserved 23 to 30; reserved 56 to 60; @@ -206,6 +207,17 @@ message BridgeUnlockAction { astria.primitive.v1.Address from = 5; } +message BridgeSudoChangeAction { + // the address of the bridge account to change the sudo or withdrawer addresses for + astria.primitive.v1.Address bridge_address = 1; + // the new sudo address; unchanged if unset + astria.primitive.v1.Address new_sudo_address = 2; + // the new withdrawer address; unchanged if unset + astria.primitive.v1.Address new_withdrawer_address = 3; + // the asset used to pay the transaction fee + bytes fee_asset_id = 4; +} + message FeeChangeAction { // note that the proto number ranges are doubled from that of `Action`. // this to accomodate both `base_fee` and `byte_cost_multiplier` for each action. From b8ae517bbd73f0d9fb9ca4ce0a0314175709a392 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 21:42:36 -0400 Subject: [PATCH 04/16] improve bridge unlock logic --- .../src/app/tests_execute_transaction.rs | 1 + .../src/bridge/bridge_unlock_action.rs | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 13915d7417..aa0c177055 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1022,6 +1022,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { state_tx .put_bridge_account_asset_id(&bridge_address, &asset_id) .unwrap(); + state_tx.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); app.apply(state_tx); let amount = 100; diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 0b03790dc5..11ac6a359f 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -1,4 +1,5 @@ use anyhow::{ + bail, ensure, Context as _, Result, @@ -40,13 +41,13 @@ impl ActionHandler for BridgeUnlockAction { .context("failed to get bridge's asset id, must be a bridge account")?; // check that the sender of this tx is the authorized withdrawer for the bridge account - // NOTE: this is made backwards-compatible by allowing the - // sudo address to be the bridge address if unset - let withdrawer_address = state + let Some(withdrawer_address) = state .get_bridge_account_withdrawer_address(&bridge_address) .await .context("failed to get bridge account withdrawer address")? - .unwrap_or(bridge_address); + else { + bail!("bridge account does not have an associated withdrawer address"); + }; ensure!( withdrawer_address == from, @@ -152,6 +153,7 @@ mod test { state .put_bridge_account_asset_id(&bridge_address, &asset_id) .unwrap(); + state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); let bridge_unlock = BridgeUnlockAction { to: to_address, @@ -230,6 +232,7 @@ mod test { .put_bridge_account_asset_id(&bridge_address, &asset_id) .unwrap(); state.put_allowed_fee_asset(asset_id); + state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); let bridge_unlock = BridgeUnlockAction { to: to_address, From 00459f322a22b6cf94aa8af19f0ab2c23c8c7a69 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 21:49:10 -0400 Subject: [PATCH 05/16] fix other crates --- crates/astria-bridge-withdrawer/priv.key | 1 + .../src/withdrawer/ethereum/convert.rs | 2 ++ crates/astria-cli/src/commands/sequencer.rs | 2 ++ .../astria.protocol.transactions.v1alpha1.rs | 4 +++- .../protocol/transaction/v1alpha1/action.rs | 22 +++++++++---------- .../src/app/tests_breaking_changes.rs | 2 +- .../src/app/tests_execute_transaction.rs | 2 +- .../src/bridge/bridge_unlock_action.rs | 18 +++++++-------- .../src/transaction/checks.rs | 2 +- .../transactions/v1alpha1/types.proto | 2 +- 10 files changed, 32 insertions(+), 25 deletions(-) create mode 100644 crates/astria-bridge-withdrawer/priv.key diff --git a/crates/astria-bridge-withdrawer/priv.key b/crates/astria-bridge-withdrawer/priv.key new file mode 100644 index 0000000000..e0cd2b4245 --- /dev/null +++ b/crates/astria-bridge-withdrawer/priv.key @@ -0,0 +1 @@ +2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90 diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs index 90857f853d..d374ba725b 100644 --- a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs +++ b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs @@ -105,6 +105,7 @@ fn event_to_bridge_unlock( ))?, memo: serde_json::to_vec(&memo).wrap_err("failed to serialize memo to json")?, fee_asset_id, + bridge_address: None, }; Ok(Action::BridgeUnlock(action)) } @@ -215,6 +216,7 @@ mod tests { }) .unwrap(), fee_asset_id: denom.id(), + bridge_address: None, }; assert_eq!(action, expected_action); diff --git a/crates/astria-cli/src/commands/sequencer.rs b/crates/astria-cli/src/commands/sequencer.rs index 9b9460c08d..0cdc7a9d21 100644 --- a/crates/astria-cli/src/commands/sequencer.rs +++ b/crates/astria-cli/src/commands/sequencer.rs @@ -269,6 +269,8 @@ pub(crate) async fn init_bridge_account(args: &InitBridgeAccountArgs) -> eyre::R rollup_id, asset_id: default_native_asset_id(), fee_asset_id: default_native_asset_id(), + sudo_address: None, + withdrawer_address: None, }), ) .await diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index dde98e036f..97b2547921 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -376,7 +376,9 @@ pub struct BridgeUnlockAction { /// if the bridge account's withdrawer address is not the same as the bridge address. /// if unset, the signer of the transaction is used. #[prost(message, optional, tag = "5")] - pub from: ::core::option::Option, + pub bridge_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, } impl ::prost::Name for BridgeUnlockAction { const NAME: &'static str = "BridgeUnlockAction"; diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 1c0f342364..1ce9154ed9 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -1284,7 +1284,7 @@ pub struct BridgeUnlockAction { // the address of the bridge account to transfer from, // if the bridge account's withdrawer address is not the same as the bridge address. // if unset, the signer of the transaction is used. - pub from: Option
, + pub bridge_address: Option
, } impl BridgeUnlockAction { @@ -1295,7 +1295,7 @@ impl BridgeUnlockAction { amount: Some(self.amount.into()), fee_asset_id: self.fee_asset_id.as_ref().to_vec(), memo: self.memo, - from: self.from.map(Address::into_raw), + bridge_address: self.bridge_address.map(Address::into_raw), } } @@ -1306,7 +1306,7 @@ impl BridgeUnlockAction { amount: Some(self.amount.into()), fee_asset_id: self.fee_asset_id.as_ref().to_vec(), memo: self.memo.clone(), - from: self.from.as_ref().map(Address::to_raw), + bridge_address: self.bridge_address.as_ref().map(Address::to_raw), } } @@ -1329,18 +1329,18 @@ impl BridgeUnlockAction { .ok_or(BridgeUnlockActionError::missing_amount())?; let fee_asset_id = asset::Id::try_from_slice(&proto.fee_asset_id) .map_err(BridgeUnlockActionError::invalid_fee_asset_id)?; - let from = proto - .from + let bridge_address = proto + .bridge_address .as_ref() .map(Address::try_from_raw) .transpose() - .map_err(BridgeUnlockActionError::invalid_from_address)?; + .map_err(BridgeUnlockActionError::invalid_bridge_address)?; Ok(Self { to, amount: amount.into(), fee_asset_id, memo: proto.memo, - from, + bridge_address, }) } } @@ -1371,8 +1371,8 @@ impl BridgeUnlockActionError { } #[must_use] - fn invalid_from_address(err: IncorrectAddressLength) -> Self { - Self(BridgeUnlockActionErrorKind::InvalidFromAddress(err)) + fn invalid_bridge_address(err: IncorrectAddressLength) -> Self { + Self(BridgeUnlockActionErrorKind::InvalidBridgeAddress(err)) } } @@ -1386,8 +1386,8 @@ enum BridgeUnlockActionErrorKind { MissingAmount, #[error("the `fee_asset_id` field was invalid")] InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), - #[error("the `from` field was invalid")] - InvalidFromAddress(#[source] IncorrectAddressLength), + #[error("the `bridge_address` field was invalid")] + InvalidBridgeAddress(#[source] IncorrectAddressLength), } #[allow(clippy::module_name_repetitions)] diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index a0cdbab1a0..875b35f9c0 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -272,7 +272,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { amount: 10, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: None, + bridge_address: None, } .into(), BridgeSudoChangeAction { diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index aa0c177055..274f9b2c01 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1052,7 +1052,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { amount, fee_asset_id: asset_id, memo: b"lilywashere".to_vec(), - from: None, + bridge_address: None, }; let tx = UnsignedTransaction { diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 11ac6a359f..ca208388d8 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -32,7 +32,7 @@ impl ActionHandler for BridgeUnlockAction { ) -> Result<()> { // the bridge address to withdraw funds from // if unset, use the tx sender's address - let bridge_address = self.from.unwrap_or(from); + let bridge_address = self.bridge_address.unwrap_or(from); // grab the bridge account's asset let asset_id = state @@ -68,7 +68,7 @@ impl ActionHandler for BridgeUnlockAction { #[instrument(skip_all)] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { // the bridge address to withdraw funds from - let bridge_address = self.from.unwrap_or(from); + let bridge_address = self.bridge_address.unwrap_or(from); let asset_id = state .get_bridge_account_asset_id(&bridge_address) @@ -123,7 +123,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: None, + bridge_address: None, }; // not a bridge account, should fail @@ -160,7 +160,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: Some(bridge_address), + bridge_address: Some(bridge_address), }; // invalid sender, doesn't match action's `from`, should fail @@ -198,7 +198,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: Some(bridge_address), + bridge_address: Some(bridge_address), }; // invalid sender, doesn't match action's bridge account's withdrawer, should fail @@ -239,7 +239,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: None, + bridge_address: None, }; // not enough balance to transfer asset; should fail @@ -294,7 +294,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: Some(bridge_address), + bridge_address: Some(bridge_address), }; // not enough balance to transfer asset; should fail @@ -346,7 +346,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: None, + bridge_address: None, }; // not enough balance; should fail @@ -398,7 +398,7 @@ mod test { amount: transfer_amount, fee_asset_id: asset_id, memo: vec![0u8; 32], - from: Some(bridge_address), + bridge_address: Some(bridge_address), }; // not enough balance; should fail diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index e7877472d2..67a6567f74 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -131,7 +131,7 @@ pub(crate) async fn check_balance_for_total_fees( Action::BridgeUnlock(act) => { bridge_unlock_update_fees( state, - act.from.unwrap_or(from), + act.bridge_address.unwrap_or(from), act.amount, act.fee_asset_id, &mut fees_by_asset, diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 470335e1be..e840717e2b 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -204,7 +204,7 @@ message BridgeUnlockAction { // the address of the bridge account to transfer from, // if the bridge account's withdrawer address is not the same as the bridge address. // if unset, the signer of the transaction is used. - astria.primitive.v1.Address from = 5; + astria.primitive.v1.Address bridge_address = 5; } message BridgeSudoChangeAction { From 9beb76804329e43a470732183691b925a6eafcda Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 3 Jun 2024 21:51:17 -0400 Subject: [PATCH 06/16] fix --- crates/astria-bridge-withdrawer/priv.key | 1 - .../astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 crates/astria-bridge-withdrawer/priv.key diff --git a/crates/astria-bridge-withdrawer/priv.key b/crates/astria-bridge-withdrawer/priv.key deleted file mode 100644 index e0cd2b4245..0000000000 --- a/crates/astria-bridge-withdrawer/priv.key +++ /dev/null @@ -1 +0,0 @@ -2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90 diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs index d374ba725b..a00af9726e 100644 --- a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs +++ b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs @@ -249,6 +249,7 @@ mod tests { }) .unwrap(), fee_asset_id: denom.id(), + bridge_address: None, }; assert_eq!(action, expected_action); From 1687ae8969e7178be5e22023fa6bcdb2d5fd8177 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 4 Jun 2024 14:49:58 -0400 Subject: [PATCH 07/16] Ics20Withdrawal updates to handle bridge accounts --- .../astria.protocol.transactions.v1alpha1.rs | 7 + .../protocol/transaction/v1alpha1/action.rs | 26 ++ .../astria-sequencer/src/accounts/action.rs | 10 + .../src/ibc/ics20_withdrawal.rs | 261 +++++++++++++++++- .../transactions/v1alpha1/types.proto | 10 + 5 files changed, 313 insertions(+), 1 deletion(-) diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 97b2547921..b831c9f84c 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -205,6 +205,13 @@ pub struct Ics20Withdrawal { /// a memo to include with the transfer #[prost(string, tag = "9")] pub memo: ::prost::alloc::string::String, + /// the address of the bridge account to transfer from, if this is a withdrawal + /// from a bridge account. + /// if unset, the withdrawal is presumed to be a user (non-bridge) withdrawal. + #[prost(message, optional, tag = "10")] + pub bridge_address: ::core::option::Option< + super::super::super::primitive::v1::Address, + >, } impl ::prost::Name for Ics20Withdrawal { const NAME: &'static str = "Ics20Withdrawal"; diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 1ce9154ed9..5c7c890ca9 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -637,6 +637,16 @@ pub struct Ics20Withdrawal { pub fee_asset_id: asset::Id, // a memo to include with the transfer pub memo: String, + // the address of the bridge account to transfer from, if this is a withdrawal + // from a bridge account and the sender of the tx is the bridge's withdrawer, + // which differs from the bridge account's address. + // + // if unset, and the transaction sender is not a bridge account, the withdrawal + // is treated as a user (non-bridge) withdrawal. + // + // if unset, and the transaction sender is a bridge account, the withdrawal is + // treated as a bridge withdrawal (ie. the bridge account's withdrawer address is checked). + pub bridge_address: Option
, } impl Ics20Withdrawal { @@ -708,6 +718,7 @@ impl Ics20Withdrawal { source_channel: self.source_channel.to_string(), fee_asset_id: self.fee_asset_id.get().to_vec(), memo: self.memo.clone(), + bridge_address: self.bridge_address.as_ref().map(Address::to_raw), } } @@ -723,6 +734,7 @@ impl Ics20Withdrawal { source_channel: self.source_channel.to_string(), fee_asset_id: self.fee_asset_id.get().to_vec(), memo: self.memo, + bridge_address: self.bridge_address.map(Address::into_raw), } } @@ -743,6 +755,12 @@ impl Ics20Withdrawal { .timeout_height .ok_or(Ics20WithdrawalError::missing_timeout_height())? .into(); + let bridge_address = proto + .bridge_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(Ics20WithdrawalError::invalid_bridge_address)?; Ok(Self { amount: amount.into(), @@ -758,6 +776,7 @@ impl Ics20Withdrawal { fee_asset_id: asset::Id::try_from_slice(&proto.fee_asset_id) .map_err(Ics20WithdrawalError::invalid_fee_asset_id)?, memo: proto.memo, + bridge_address, }) } } @@ -819,6 +838,11 @@ impl Ics20WithdrawalError { fn invalid_fee_asset_id(err: asset::IncorrectAssetIdLength) -> Self { Self(Ics20WithdrawalErrorKind::InvalidFeeAssetId(err)) } + + #[must_use] + fn invalid_bridge_address(err: IncorrectAddressLength) -> Self { + Self(Ics20WithdrawalErrorKind::InvalidBridgeAddress(err)) + } } #[derive(Debug, thiserror::Error)] @@ -833,6 +857,8 @@ enum Ics20WithdrawalErrorKind { InvalidSourceChannel(#[source] IdentifierError), #[error("`fee_asset_id` field was invalid")] InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), + #[error("`bridge_address` field was invalid")] + InvalidBridgeAddress(#[source] IncorrectAddressLength), } #[allow(clippy::module_name_repetitions)] diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 1546add77b..585e6cb2bc 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -14,6 +14,7 @@ use crate::{ StateReadExt, StateWriteExt, }, + bridge::state_ext::StateReadExt as _, state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -85,6 +86,15 @@ impl ActionHandler for TransferAction { state: &S, from: Address, ) -> Result<()> { + ensure!( + state + .get_bridge_account_rollup_id(&from) + .await + .context("failed to get bridge account rollup id")? + .is_none(), + "cannot transfer out of bridge account; BridgeUnlock must be used", + ); + transfer_check_stateful(self, state, from) .await .context("stateful transfer check failed") diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 9eceeee8bf..6cc19a1032 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -1,5 +1,6 @@ use anyhow::{ anyhow, + bail, ensure, Context as _, Result, @@ -28,6 +29,7 @@ use crate::{ StateReadExt, StateWriteExt, }, + bridge::state_ext::StateReadExt as _, ibc::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -51,6 +53,49 @@ fn withdrawal_to_unchecked_ibc_packet( ) } +async fn ics20_withdrawal_check_stateful_bridge_account( + action: &action::Ics20Withdrawal, + state: &S, + from: Address, +) -> Result<()> { + // bridge address checks: + // - if the sender of this transaction is not a bridge account, and the tx `bridge_address` + // field is None, don't need to do any bridge related checks as it's a normal user withdrawal. + // - if the sender of this transaction is a bridge account, and the tx `bridge_address` field is + // None, check that the withdrawer address is the same as the transaction sender. + // - if the tx `bridge_address` field is Some, check that the `bridge_address` is a valid + // bridge, and check that the withdrawer address is the same as the transaction sender. + + let is_sender_bridge = state + .get_bridge_account_rollup_id(&from) + .await + .context("failed to get bridge account rollup id")? + .is_some(); + + if !is_sender_bridge && action.bridge_address.is_none() { + return Ok(()); + } + + // if `action.bridge_address` is Some, but it's not a valid bridge account, + // the `get_bridge_account_withdrawer_address` step will fail. + let bridge_address = action.bridge_address.unwrap_or(from); + + let Some(withdrawer) = state + .get_bridge_account_withdrawer_address(&bridge_address) + .await + .context("failed to get bridge withdrawer")? + else { + bail!("bridge address must have a withdrawer address set"); + }; + + ensure!( + withdrawer == from, + "sender does not match bridge withdrawer address; unauthorized" + ); + + Ok(()) +} + #[async_trait::async_trait] impl ActionHandler for action::Ics20Withdrawal { #[instrument(skip(self))] @@ -69,7 +114,7 @@ impl ActionHandler for action::Ics20Withdrawal { state: &S, from: Address, ) -> Result<()> { - // TODO: bridge withdrawer address check + ics20_withdrawal_check_stateful_bridge_account(self, state, from).await?; let fee = state .get_ics20_withdrawal_base_fee() @@ -172,3 +217,217 @@ fn is_source(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> let prefix = format!("{source_port}/{source_channel}/"); !asset.prefix_is(&prefix) } + +#[cfg(test)] +mod tests { + use astria_core::primitive::v1::RollupId; + use cnidarium::StateDelta; + use ibc_types::core::client::Height; + + use super::*; + use crate::bridge::state_ext::StateWriteExt as _; + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_not_bridge() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let state = StateDelta::new(snapshot); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: None, + destination_chain_address: "test".to_string(), + return_address: Address::from([1u8; 20]), + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + let from = Address::from([1u8; 20]); + ics20_withdrawal_check_stateful_bridge_account(&action, &state, from) + .await + .unwrap(); + } + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_sender_is_bridge_bridge_address_none_ok() + { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // sender is a bridge address, which is also the withdrawer, so it's ok + let bridge_address = Address::from([1u8; 20]); + state.put_bridge_account_rollup_id( + &bridge_address, + &RollupId::from_unhashed_bytes("testrollupid"), + ); + state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: None, + destination_chain_address: "test".to_string(), + return_address: bridge_address, + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) + .await + .unwrap(); + } + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_sender_is_bridge_bridge_address_none_invalid() + { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer + let bridge_address = Address::from([1u8; 20]); + state.put_bridge_account_rollup_id( + &bridge_address, + &RollupId::from_unhashed_bytes("testrollupid"), + ); + state.put_bridge_account_withdrawer_address(&bridge_address, &Address::from([2u8; 20])); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: None, + destination_chain_address: "test".to_string(), + return_address: bridge_address, + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) + .await + .unwrap_err(); + assert!( + err.to_string() + .contains("sender does not match bridge withdrawer address; unauthorized") + ); + } + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_bridge_address_some_ok() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // sender the withdrawer address, so it's ok + let bridge_address = Address::from([1u8; 20]); + let withdrawer_address = Address::from([2u8; 20]); + state.put_bridge_account_rollup_id( + &bridge_address, + &RollupId::from_unhashed_bytes("testrollupid"), + ); + state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: Some(bridge_address), + destination_chain_address: "test".to_string(), + return_address: bridge_address, + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + ics20_withdrawal_check_stateful_bridge_account(&action, &state, withdrawer_address) + .await + .unwrap(); + } + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_bridge_address_some_invalid_sender() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // sender is not the withdrawer address, so must fail + let bridge_address = Address::from([1u8; 20]); + let withdrawer_address = Address::from([2u8; 20]); + state.put_bridge_account_rollup_id( + &bridge_address, + &RollupId::from_unhashed_bytes("testrollupid"), + ); + state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: Some(bridge_address), + destination_chain_address: "test".to_string(), + return_address: bridge_address, + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) + .await + .unwrap_err(); + assert!( + err.to_string() + .contains("sender does not match bridge withdrawer address; unauthorized") + ); + } + + #[tokio::test] + async fn ics20_withdrawal_check_stateful_bridge_account_bridge_address_some_invalid_bridge_account() + { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let state = StateDelta::new(snapshot); + + // sender is not the withdrawer address, so must fail + let not_bridge_address = Address::from([1u8; 20]); + + let denom = Denom::from("test".to_string()); + let action = action::Ics20Withdrawal { + amount: 1, + denom: denom.clone(), + bridge_address: Some(not_bridge_address), + destination_chain_address: "test".to_string(), + return_address: not_bridge_address, + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset_id: denom.id(), + memo: "".to_string(), + }; + + let err = + ics20_withdrawal_check_stateful_bridge_account(&action, &state, not_bridge_address) + .await + .unwrap_err(); + assert!( + err.to_string() + .contains("bridge address must have a withdrawer address set") + ); + } +} diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index e840717e2b..231385d96a 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -120,6 +120,16 @@ message Ics20Withdrawal { bytes fee_asset_id = 8; // a memo to include with the transfer string memo = 9; + // the address of the bridge account to transfer from, if this is a withdrawal + // from a bridge account and the sender of the tx is the bridge's withdrawer, + // which differs from the bridge account's address. + // + // if unset, and the transaction sender is not a bridge account, the withdrawal + // is treated as a user (non-bridge) withdrawal. + // + // if unset, and the transaction sender is a bridge account, the withdrawal is + // treated as a bridge withdrawal (ie. the bridge account's withdrawer address is checked). + astria.primitive.v1.Address bridge_address = 10; } message IbcHeight { From 97473aa3ea05cc8e73d4b3eb865ca2c5cf3cb5d3 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 4 Jun 2024 14:51:40 -0400 Subject: [PATCH 08/16] fmt proto --- .../astria/protocol/transactions/v1alpha1/types.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 231385d96a..1a44b51b3f 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -124,7 +124,7 @@ message Ics20Withdrawal { // from a bridge account and the sender of the tx is the bridge's withdrawer, // which differs from the bridge account's address. // - // if unset, and the transaction sender is not a bridge account, the withdrawal + // if unset, and the transaction sender is not a bridge account, the withdrawal // is treated as a user (non-bridge) withdrawal. // // if unset, and the transaction sender is a bridge account, the withdrawal is From 5df401de9f388b0100f1d5740c0a973d07d41def Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 4 Jun 2024 14:54:47 -0400 Subject: [PATCH 09/16] protos --- .../generated/astria.protocol.transactions.v1alpha1.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index b831c9f84c..fa69e560be 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -206,8 +206,14 @@ pub struct Ics20Withdrawal { #[prost(string, tag = "9")] pub memo: ::prost::alloc::string::String, /// the address of the bridge account to transfer from, if this is a withdrawal - /// from a bridge account. - /// if unset, the withdrawal is presumed to be a user (non-bridge) withdrawal. + /// from a bridge account and the sender of the tx is the bridge's withdrawer, + /// which differs from the bridge account's address. + /// + /// if unset, and the transaction sender is not a bridge account, the withdrawal + /// is treated as a user (non-bridge) withdrawal. + /// + /// if unset, and the transaction sender is a bridge account, the withdrawal is + /// treated as a bridge withdrawal (ie. the bridge account's withdrawer address is checked). #[prost(message, optional, tag = "10")] pub bridge_address: ::core::option::Option< super::super::super::primitive::v1::Address, From 4fd9e5a24624eec7f2dd6836aab3fbc39452511d Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 4 Jun 2024 14:57:07 -0400 Subject: [PATCH 10/16] clippy --- crates/astria-bridge-withdrawer/priv.key | 1 + .../src/withdrawer/ethereum/convert.rs | 1 + crates/astria-sequencer/src/ibc/ics20_withdrawal.rs | 12 ++++++------ 3 files changed, 8 insertions(+), 6 deletions(-) create mode 100644 crates/astria-bridge-withdrawer/priv.key diff --git a/crates/astria-bridge-withdrawer/priv.key b/crates/astria-bridge-withdrawer/priv.key new file mode 100644 index 0000000000..e0cd2b4245 --- /dev/null +++ b/crates/astria-bridge-withdrawer/priv.key @@ -0,0 +1 @@ +2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90 diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs index a00af9726e..ba1831a7e6 100644 --- a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs +++ b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs @@ -172,6 +172,7 @@ fn event_to_ics20_withdrawal( source_channel: channel .parse() .wrap_err("failed to parse channel from denom")?, + bridge_address: None, }; Ok(Action::Ics20Withdrawal(action)) } diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 6cc19a1032..8e4345f538 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -244,7 +244,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; let from = Address::from([1u8; 20]); @@ -279,7 +279,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) @@ -313,7 +313,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) @@ -351,7 +351,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; ics20_withdrawal_check_stateful_bridge_account(&action, &state, withdrawer_address) @@ -385,7 +385,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) @@ -418,7 +418,7 @@ mod tests { timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), fee_asset_id: denom.id(), - memo: "".to_string(), + memo: String::new(), }; let err = From a483ace95d8d2370d9bafffbdf394398d2dc6df4 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 4 Jun 2024 15:56:12 -0400 Subject: [PATCH 11/16] clippy --- .../astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs index ba1831a7e6..3b07f88e68 100644 --- a/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs +++ b/crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs @@ -295,6 +295,7 @@ mod tests { timeout_height: IbcHeight::new(u64::MAX, u64::MAX).unwrap(), timeout_time: 0, // zero this for testing source_channel: "channel-0".parse().unwrap(), + bridge_address: None, }; assert_eq!(action, expected_action); } From 034b329987da3a90fdafc414529fee8dcf00ac85 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Thu, 6 Jun 2024 14:46:26 -0400 Subject: [PATCH 12/16] fix tests --- .../src/withdrawer/submitter/tests.rs | 2 ++ crates/astria-sequencer/src/bridge/state_ext.rs | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs b/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs index 9f1f1fe053..d0a61e84e8 100644 --- a/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs +++ b/crates/astria-bridge-withdrawer/src/withdrawer/submitter/tests.rs @@ -179,6 +179,7 @@ fn make_ics20_withdrawal_action() -> Action { timeout_height: IbcHeight::new(u64::MAX, u64::MAX).unwrap(), timeout_time: 0, // zero this for testing source_channel: "channel-0".parse().unwrap(), + bridge_address: None, }; Action::Ics20Withdrawal(inner) @@ -195,6 +196,7 @@ fn make_bridge_unlock_action() -> Action { }) .unwrap(), fee_asset_id: denom.id(), + bridge_address: None, }; Action::BridgeUnlock(inner) } diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 814d7145b3..483dceb314 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -106,7 +106,9 @@ fn last_transaction_hash_for_bridge_account_storage_key(address: &Address) -> Ve format!( "{}/lasttx", bridge_account_storage_key(&address.encode_hex::()) - ).as_bytes().to_vec() + ) + .as_bytes() + .to_vec() } #[async_trait] From eb57b26ae21035df84392d9838c818794fde620a Mon Sep 17 00:00:00 2001 From: elizabeth Date: Fri, 7 Jun 2024 14:35:53 -0400 Subject: [PATCH 13/16] address comments --- crates/astria-bridge-withdrawer/priv.key | 1 - .../generated/astria.protocol.transactions.v1alpha1.rs | 4 +++- .../src/protocol/transaction/v1alpha1/action.rs | 8 ++++++++ .../astria/protocol/transactions/v1alpha1/types.proto | 1 + 4 files changed, 12 insertions(+), 2 deletions(-) delete mode 100644 crates/astria-bridge-withdrawer/priv.key diff --git a/crates/astria-bridge-withdrawer/priv.key b/crates/astria-bridge-withdrawer/priv.key deleted file mode 100644 index e0cd2b4245..0000000000 --- a/crates/astria-bridge-withdrawer/priv.key +++ /dev/null @@ -1 +0,0 @@ -2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90 diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index fa69e560be..4f6d89cfc8 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -434,7 +434,7 @@ impl ::prost::Name for BridgeSudoChangeAction { pub struct FeeChangeAction { /// note that the proto number ranges are doubled from that of `Action`. /// this to accomodate both `base_fee` and `byte_cost_multiplier` for each action. - #[prost(oneof = "fee_change_action::Value", tags = "1, 2, 3, 20, 21, 40")] + #[prost(oneof = "fee_change_action::Value", tags = "1, 2, 3, 20, 21, 22, 40")] pub value: ::core::option::Option, } /// Nested message and enum types in `FeeChangeAction`. @@ -456,6 +456,8 @@ pub mod fee_change_action { InitBridgeAccountBaseFee(super::super::super::super::primitive::v1::Uint128), #[prost(message, tag = "21")] BridgeLockByteCostMultiplier(super::super::super::super::primitive::v1::Uint128), + #[prost(message, tag = "22")] + BridgeSudoChangeBaseFee(super::super::super::super::primitive::v1::Uint128), /// ibc fees are defined on 40-59 #[prost(message, tag = "40")] Ics20WithdrawalBaseFee(super::super::super::super::primitive::v1::Uint128), diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 5c7c890ca9..5634bf7ac6 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -1454,6 +1454,7 @@ impl BridgeSudoChangeAction { /// - if the `bridge_address` field is invalid /// - if the `new_sudo_address` field is invalid /// - if the `new_withdrawer_address` field is invalid + /// - if the `fee_asset_id` field is invalid pub fn try_from_raw( proto: raw::BridgeSudoChangeAction, ) -> Result { @@ -1538,6 +1539,7 @@ pub enum FeeChange { SequenceByteCostMultiplier, InitBridgeAccountBaseFee, BridgeLockByteCostMultiplier, + BridgeSudoChangeBaseFee, Ics20WithdrawalBaseFee, } @@ -1575,6 +1577,9 @@ impl FeeChangeAction { self.new_value.into(), ) } + FeeChange::BridgeSudoChangeBaseFee => { + raw::fee_change_action::Value::BridgeSudoChangeBaseFee(self.new_value.into()) + } FeeChange::Ics20WithdrawalBaseFee => { raw::fee_change_action::Value::Ics20WithdrawalBaseFee(self.new_value.into()) } @@ -1605,6 +1610,9 @@ impl FeeChangeAction { Some(raw::fee_change_action::Value::BridgeLockByteCostMultiplier(new_value)) => { (FeeChange::BridgeLockByteCostMultiplier, new_value) } + Some(raw::fee_change_action::Value::BridgeSudoChangeBaseFee(new_value)) => { + (FeeChange::BridgeSudoChangeBaseFee, new_value) + } Some(raw::fee_change_action::Value::Ics20WithdrawalBaseFee(new_value)) => { (FeeChange::Ics20WithdrawalBaseFee, new_value) } diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 1a44b51b3f..635369769c 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -240,6 +240,7 @@ message FeeChangeAction { // bridge fees are defined on 20-39 astria.primitive.v1.Uint128 init_bridge_account_base_fee = 20; astria.primitive.v1.Uint128 bridge_lock_byte_cost_multiplier = 21; + astria.primitive.v1.Uint128 bridge_sudo_change_base_fee = 22; // ibc fees are defined on 40-59 astria.primitive.v1.Uint128 ics20_withdrawal_base_fee = 40; From 18afe4c148d9ad154b0ca4096a6be69f679c8e5d Mon Sep 17 00:00:00 2001 From: elizabeth Date: Fri, 7 Jun 2024 14:39:22 -0400 Subject: [PATCH 14/16] add comment to protos --- .../src/generated/astria.protocol.transactions.v1alpha1.rs | 2 ++ .../astria/protocol/transactions/v1alpha1/types.proto | 2 ++ 2 files changed, 4 insertions(+) diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 4f6d89cfc8..3921eb85a3 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -316,11 +316,13 @@ pub struct InitBridgeAccountAction { pub fee_asset_id: ::prost::alloc::vec::Vec, /// the address corresponding to the key which has sudo capabilities; /// ie. can change the sudo and withdrawer addresses for this bridge account. + /// if this is empty, the sender of the transaction is used. #[prost(message, optional, tag = "4")] pub sudo_address: ::core::option::Option< super::super::super::primitive::v1::Address, >, /// the address corresponding to the key which can withdraw funds from this bridge account. + /// if this is empty, the sender of the transaction is used. #[prost(message, optional, tag = "5")] pub withdrawer_address: ::core::option::Option< super::super::super::primitive::v1::Address, diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 635369769c..6c068afcfe 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -173,8 +173,10 @@ message InitBridgeAccountAction { bytes fee_asset_id = 3; // the address corresponding to the key which has sudo capabilities; // ie. can change the sudo and withdrawer addresses for this bridge account. + // if this is empty, the sender of the transaction is used. astria.primitive.v1.Address sudo_address = 4; // the address corresponding to the key which can withdraw funds from this bridge account. + // if this is empty, the sender of the transaction is used. astria.primitive.v1.Address withdrawer_address = 5; } From e82f649bddafac2cad781065ee730f64485e851b Mon Sep 17 00:00:00 2001 From: elizabeth Date: Fri, 7 Jun 2024 14:42:51 -0400 Subject: [PATCH 15/16] fix sequencer --- crates/astria-sequencer/src/authority/action.rs | 3 +++ .../src/bridge/bridge_sudo_change_action.rs | 4 ++-- crates/astria-sequencer/src/bridge/component.rs | 2 +- crates/astria-sequencer/src/bridge/state_ext.rs | 4 ++-- crates/astria-sequencer/src/transaction/checks.rs | 6 +++--- 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 6c5386e0b4..e2420c1d30 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -142,6 +142,9 @@ impl ActionHandler for FeeChangeAction { FeeChange::BridgeLockByteCostMultiplier => { state.put_bridge_lock_byte_cost_multiplier(self.new_value); } + FeeChange::BridgeSudoChangeBaseFee => { + state.put_bridge_sudo_change_base_fee(self.new_value); + } FeeChange::Ics20WithdrawalBaseFee => { state .put_ics20_withdrawal_base_fee(self.new_value) diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 2e857e35a5..e7142bd65d 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -59,7 +59,7 @@ impl ActionHandler for BridgeSudoChangeAction { #[instrument(skip_all)] async fn execute(&self, state: &mut S, _: Address) -> Result<()> { let fee = state - .get_bridge_sudo_change_fee() + .get_bridge_sudo_change_base_fee() .await .context("failed to get bridge sudo change fee")?; state @@ -144,7 +144,7 @@ mod tests { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); - state.put_bridge_sudo_change_fee(10); + state.put_bridge_sudo_change_base_fee(10); let fee_asset_id = Id::from_denom("test"); let bridge_address = Address::from([99; 20]); diff --git a/crates/astria-sequencer/src/bridge/component.rs b/crates/astria-sequencer/src/bridge/component.rs index 509d109566..bc180e4e24 100644 --- a/crates/astria-sequencer/src/bridge/component.rs +++ b/crates/astria-sequencer/src/bridge/component.rs @@ -24,7 +24,7 @@ impl Component for BridgeComponent { async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { state.put_init_bridge_account_base_fee(app_state.fees.init_bridge_account_base_fee); state.put_bridge_lock_byte_cost_multiplier(app_state.fees.bridge_lock_byte_cost_multiplier); - state.put_bridge_sudo_change_fee(app_state.fees.bridge_sudo_change_fee); + state.put_bridge_sudo_change_base_fee(app_state.fees.bridge_sudo_change_fee); Ok(()) } diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 483dceb314..876e6d5f8f 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -273,7 +273,7 @@ pub(crate) trait StateReadExt: StateRead { } #[instrument(skip(self))] - async fn get_bridge_sudo_change_fee(&self) -> Result { + async fn get_bridge_sudo_change_base_fee(&self) -> Result { let bytes = self .get_raw(BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY) .await @@ -416,7 +416,7 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip(self))] - fn put_bridge_sudo_change_fee(&mut self, fee: u128) { + fn put_bridge_sudo_change_base_fee(&mut self, fee: u128) { self.put_raw( BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY.to_string(), borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index 67a6567f74..1c86b89b80 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -91,7 +91,7 @@ pub(crate) async fn check_balance_for_total_fees( .await .context("failed to get bridge lock byte cost multiplier")?; let bridge_sudo_change_fee = state - .get_bridge_sudo_change_fee() + .get_bridge_sudo_change_base_fee() .await .context("failed to get bridge sudo change fee")?; @@ -315,7 +315,7 @@ mod test { state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); state_tx.put_init_bridge_account_base_fee(12); state_tx.put_bridge_lock_byte_cost_multiplier(1); - state_tx.put_bridge_sudo_change_fee(24); + state_tx.put_bridge_sudo_change_base_fee(24); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); @@ -382,7 +382,7 @@ mod test { state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); state_tx.put_init_bridge_account_base_fee(12); state_tx.put_bridge_lock_byte_cost_multiplier(1); - state_tx.put_bridge_sudo_change_fee(24); + state_tx.put_bridge_sudo_change_base_fee(24); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); From 974ab0c0bf04782f30d6a600995dd9d18245cd29 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Sat, 8 Jun 2024 18:59:46 -0400 Subject: [PATCH 16/16] update chart for genesis change --- charts/sequencer/Chart.yaml | 2 +- charts/sequencer/files/cometbft/config/genesis.json | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/charts/sequencer/Chart.yaml b/charts/sequencer/Chart.yaml index cf0d784873..15a30d16bc 100644 --- a/charts/sequencer/Chart.yaml +++ b/charts/sequencer/Chart.yaml @@ -15,7 +15,7 @@ type: application # This is the chart version. This version number should be incremented each time you make changes # to the chart and its templates, including the app version. # Versions are expected to follow Semantic Versioning (https://semver.org/) -version: 0.15.4 +version: 0.15.5 # This is the version number of the application being deployed. This version number should be # incremented each time you make changes to the application. Versions are not expected to diff --git a/charts/sequencer/files/cometbft/config/genesis.json b/charts/sequencer/files/cometbft/config/genesis.json index 1e0e5566c9..4474934237 100644 --- a/charts/sequencer/files/cometbft/config/genesis.json +++ b/charts/sequencer/files/cometbft/config/genesis.json @@ -18,6 +18,7 @@ "sequence_byte_cost_multiplier": 1, "init_bridge_account_base_fee": 48, "bridge_lock_byte_cost_multiplier": 1, + "bridge_sudo_change_fee": 24, "ics20_withdrawal_base_fee": 24 }, "allowed_fee_assets": [