Skip to content

Commit

Permalink
fix(sequencer)!: ensure deposits use trace-prefixed assets (#1807)
Browse files Browse the repository at this point in the history
## Summary
Ensures deposits created within bridge lock actions use only
trace-prefixed assets.

## Background
Currently, deposits created within a bridge lock action include
whichever asset is used in the action, which can be either IBC- or
trace-prefixed. This poses a problem, since our EVM has no concept of
IBC-prefixed assets, and we shouldn't expect others using our network
to, either. One solution discussed was to enforce all `BridgeLock`
actions to use trace prefixed assets, but this would be a consensus
breaking change. This aims to remedy the bug whilst making no breaking
changes.

## Changes
- Map any IBC-prefixed assets included in a `BridgeLock` action to
trace-prefixed when constructing the `Deposit`.

## Testing
- Added unit test to ensure an IBC-prefixed asset included in a bridge
lock action is correctly mapped to a trace-prefixed asset in the
corresponding deposit.
- Tested that change would be non-breaking by syncing a full node to
mainnet

## Changelogs
Changelog updated.

## Breaking Changes
Technically, this change is breaking, since any sequencer version prior
to this which received a `BridgeLock` action containing an IBC-prefixed
asset would emit a `Deposit` with an IBC-prefixed asset, while any
sequencer version after would only emit deposits with trace-prefixed
assets. However, owing to the fact that no such cases have happened yet
on mainnet, this change would not be practically breaking so long as no
`BridgeLock` actions containing IBC-prefixed denoms have been submitted
by the time this patch is pushed out. This is actively being tested by a
full-node running this PR against mainnet.

## Steps to sync this PR to Mainnet on your local machine

Ensure you have docker installed + running

### Download and build Sequencer
```bash
git clone https://github.com/astriaorg/astria.git
cd astria
git checkout ENG-1004/trace_prefixed_deposits
cargo build -p astria-sequencer --release
just docker-build astria-sequencer
```

### Helm install
```bash
helm repo add astria https://astriaorg.github.io/charts/
```
Download this file and put in the `astria` repo:
https://github.com/astriaorg/networks/blob/main/astria/full-node-values.yaml

Make these edits to `full-node-values.yaml`, replacing "your_moniker"
with your chosen moniker:

```yaml
namespace: "astria-node"
moniker: `<your_moniker>`
images:
     sequencer:
          tag: local
cometbft:
     secrets:
          nodeKey:
               devContent:
                    priv_key:
                         value: 1yh4XrMHn75sSW5cOhGDTVgv5BbqXlhrLduxHcE2t1osbwKQzo7xlvSK1vh5CVDvHESPYK/56uTKXM/1ifqHbw==
     privValidatorKey:
          devContent:
               address: 8C17BBDC7C350C83C550163458FC9B7A5B54A64E
               pub_key:
                    value: 4v1RdMiKkWgBBTTP26iRSLOEkAY99gMVfZijm6OCzjs=
               priv_key:
                    value: WxsUPI2QASIwK+XV547R8xzL66a5oBR2G8h53t0Gcl7i/VF0yIqRaAEFNM/bqJFIs4SQBj32AxV9mKObo4LOOw==
sequencer:
     metrics:
          enabled: false
     otel:
          enabled: false
storage:
     local: true
     entities:
          sequencerSharedStorage:
               storageClassName: <your_moniker>-sequencer-shared-storage-local
```

From the `astria` repo:
```bash
just deploy cluster
just deploy ingress-controller
just wait-for-ingress-controller
just load-image sequencer
helm install astria-full-node astria/sequencer --version 1.0.0-rc.4 \
  --namespace astria-node --create-namespace -f full-node-values.yaml
```

After this you should be able to go into k9s and see both the sequencer
and CometBFT running

## Related Issues
closes #1806
  • Loading branch information
ethanoroshiba authored Nov 21, 2024
1 parent fb66cf8 commit e67fdb9
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 2 deletions.
3 changes: 2 additions & 1 deletion crates/astria-sequencer/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## Changed
### Changed

- Index all event attributes [#1786](https://github.com/astriaorg/astria/pull/1786).
- Ensure all deposit assets are trace prefixed [#1807](https://github.com/astriaorg/astria/pull/1807).

## [1.0.0] - 2024-10-25

Expand Down
102 changes: 101 additions & 1 deletion crates/astria-sequencer/src/bridge/bridge_lock_action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::{
},
address::StateReadExt as _,
app::ActionHandler,
assets::StateReadExt as _,
bridge::{
StateReadExt as _,
StateWriteExt as _,
Expand Down Expand Up @@ -68,11 +69,21 @@ impl ActionHandler for BridgeLock {
.expect("current source should be set before executing action")
.source_action_index;

// map asset to trace prefixed asset for deposit, if it is not already
let deposit_asset = match self.asset.as_trace_prefixed() {
Some(asset) => asset.clone(),
None => state
.map_ibc_to_trace_prefixed_asset(&allowed_asset)
.await
.wrap_err("failed to map IBC asset to trace prefixed asset")?
.ok_or_eyre("mapping from IBC prefixed bridge asset to trace prefixed not found")?,
};

let deposit = Deposit {
bridge_address: self.to,
rollup_id,
amount: self.amount,
asset: self.asset.clone(),
asset: deposit_asset.into(),
destination_chain_address: self.destination_chain_address.clone(),
source_transaction_id,
source_action_index,
Expand All @@ -94,3 +105,92 @@ impl ActionHandler for BridgeLock {
Ok(())
}
}

#[cfg(test)]
mod tests {
use astria_core::{
primitive::v1::{
asset,
TransactionId,
},
protocol::transaction::v1::action::BridgeLock,
};
use cnidarium::StateDelta;

use crate::{
accounts::{
AddressBytes,
StateWriteExt as _,
},
address::StateWriteExt as _,
app::ActionHandler as _,
assets::StateWriteExt as _,
benchmark_and_test_utils::{
astria_address,
nria,
ASTRIA_PREFIX,
},
bridge::{
StateReadExt,
StateWriteExt as _,
},
transaction::{
StateWriteExt as _,
TransactionContext,
},
};

#[tokio::test]
async fn bridge_lock_maps_ibc_to_trace_prefixed_for_deposit() {
let storage = cnidarium::TempStorage::new().await.unwrap();
let snapshot = storage.latest_snapshot();
let mut state = StateDelta::new(snapshot);

let trace_asset = "trace_asset"
.parse::<asset::denom::TracePrefixed>()
.unwrap();
let ibc_asset = trace_asset.to_ibc_prefixed();
let transfer_amount = 100;
let bridge_address = astria_address(&[3; 20]);
let from_address = astria_address(&[1; 20]);

state.put_transaction_context(TransactionContext {
address_bytes: *from_address.address_bytes(),
transaction_id: TransactionId::new([0; 32]),
source_action_index: 0,
});
state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap();
state
.put_bridge_account_rollup_id(&bridge_address, [0; 32].into())
.unwrap();
state
.put_bridge_account_ibc_asset(&bridge_address, ibc_asset)
.unwrap();
state.put_ibc_asset(trace_asset.clone()).unwrap();
state
.put_account_balance(&from_address, &trace_asset, transfer_amount)
.unwrap();

let bridge_lock_action = BridgeLock {
to: bridge_address,
amount: transfer_amount,
asset: ibc_asset.into(),
fee_asset: nria().into(),
destination_chain_address: "ethan_was_here".to_string(),
};

bridge_lock_action
.check_and_execute(&mut state)
.await
.unwrap();

let deposits = state
.get_cached_block_deposits()
.values()
.next()
.unwrap()
.clone();
assert_eq!(deposits.len(), 1);
assert!(deposits[0].asset.as_trace_prefixed().is_some());
}
}

0 comments on commit e67fdb9

Please sign in to comment.