feat(sequencer)!: implement bridge unlock action and derestrict transfers #1034

Lilyjjo · 2024-05-02T13:20:01Z

Summary

Implements bridge accounts' BridgeUnlock action and allows bridge accounts to transfer assets like normal.

Background

We need a BridgeUnlock action to help with tracking/proving that withdraws for rollup specific events happened. This PR implements the ground work for that by adding a new action with a memo:bytes field that is currently unused.

We also decided that it's unnecessary to have restrictions on the bridge account in terms of normal transfers in/out of the account. The bridge account is already a privileged actor and having the restrictions on transfer would make normal account maintenance (like gas funding) messy.

Changes

implement BridgeUnlockAction for transferring out a bridge's bridged asset
derestricted the transfer function to let bridge account also act like normal accounts

Testing

unit tests

Breaking Changelist

one new action is added: BridgeUnlock
bridge account now can transfer assets in/out like normal sequencer accounts

Related Issues

closes #983

Fraser999

Just a few minor comments/questions.

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

Fraser999 · 2024-05-03T17:47:41Z

crates/astria-sequencer/src/accounts/action.rs

+        if state
+            .get_bridge_account_rollup_id(&from)
+            .await
+            .context("failed to get bridge account rollup ID from state")?


This is identical to the context being applied just above (at line 95) and a third in ics20_transfer.rs. It might be useful to change both strings in this file to hold a little more info to disambiguate the three, e.g. "failed to get bridge account rollup ID for transfer [sender|recipient] from state"

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

Fraser999 · 2024-05-04T11:48:31Z

crates/astria-sequencer/src/accounts/action.rs

+            let bridged_asset_id = state
+                .get_bridge_account_asset_ids(&from)
+                .await
+                .context("failed to get bridge account asset ID")?;


This context appears in four different places. Maybe there should be a separate PR where we deduplicate error contexts across the crate?

noot · 2024-05-08T01:22:37Z

crates/astria-sequencer/src/accounts/action.rs

+        // ensure that if the sender is a bridge account, that the asset being transferred is not
+        // the same asset as the bridge account's asset. The bridge account should be using
+        // the `BridgeUnlockAction` for that.


i'm not sure if we need this logic - i think we should either not allow Transfers out of bridge accounts at all, or allow them but not place any restrictions on them. this seems unnecessary, as if the bridge account owner wanted to transfer out the same asset but was restricted from using Transfer, they could still just use BridgeUnlock and leave the memo empty (or put something random)

I think we should remove this part for now and have a bigger discussion on what actions/accounting should look like

noot · 2024-05-08T01:23:28Z

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

+
+        // ensure the bridge account's tracked asset is being transferred.
+        let allowed_asset_id = state
+            .get_bridge_account_asset_ids(&from)


since this returns just 1 asset id now, can you rename the fn to get_bridge_account_asset_id?

noot · 2024-05-08T01:24:43Z

proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto

+  // the asset to be transferred
+  bytes asset_id = 3;


don't think we need this field - since bridge accounts only allow one asset to be bridged, we know using BridgeUnlockAction must also withdraw that same asset

noot · 2024-05-08T01:25:08Z

proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto

+// It's the same as a `TransferAction` but with the added
+// `memo` field.
+message BridgeUnlockAction {
+  // the address of the bridge account to transfer to


Suggested change

// the address of the bridge account to transfer to

// the address to withdraw funds to

noot · 2024-05-08T01:26:21Z

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

+
+    #[instrument(skip_all)]
+    async fn execute<S: StateWriteExt>(&self, state: &mut S, from: Address) -> Result<()> {
+        let transfer_action = TransferAction {


the memo is unused, which is fine, when we decide on the memo structure we should also store that (or data extracted from it). maybe add a comment here about that?

noot · 2024-05-13T20:19:27Z

crates/astria-sequencer/src/app/mod.rs

@@ -1142,7 +1142,7 @@ async fn update_mempool_after_finalization<S: StateReadExt>(

    for (tx, priority) in mempool.inner().await.iter_mut() {
        match TransactionPriority::new(
-            tx.nonce(),
+            (*tx).nonce(),


why the change here?

it was giving me linting errors, i think i messed up my vscode. i removed it!

noot · 2024-05-13T20:23:34Z

crates/astria-sequencer/src/app/tests_execute_transaction.rs

+    let signed_tx = tx.into_signed(&bridge_signing_key);
+    app.execute_transaction(signed_tx)
+        .await
+        .expect("failed while executing bridge unlock action");


Suggested change

.expect("failed while executing bridge unlock action");

.expect("executing bridge unlock action should succeed");

noot · 2024-05-13T20:23:44Z

crates/astria-sequencer/src/app/tests_execute_transaction.rs

+        app.state
+            .get_account_balance(bridge_address, asset_id)
+            .await
+            .expect("failed while getting account balance"),


Suggested change

.expect("failed while getting account balance"),

.expect("getting account balance should succeed"),

noot

nice, this looks good!

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels May 2, 2024

Lilyjjo had a problem deploying to BUF May 2, 2024 13:20 — with GitHub Actions Failure

Lilyjjo changed the title ~~Lilyjjo/bridge unlock action~~ feat(sequencer)!: implement bridge unlock action May 2, 2024

Lilyjjo temporarily deployed to BUF May 2, 2024 20:17 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 2, 2024 20:43 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 2, 2024 21:09 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 3, 2024 14:01 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 3, 2024 14:03 — with GitHub Actions Inactive

Lilyjjo force-pushed the lilyjjo/bridge-unlock-action branch from 1f18d0e to 6921aaf Compare May 3, 2024 14:04

Lilyjjo temporarily deployed to BUF May 3, 2024 14:05 — with GitHub Actions Inactive

Lilyjjo marked this pull request as ready for review May 3, 2024 14:09

Lilyjjo requested review from a team as code owners May 3, 2024 14:09

Lilyjjo requested review from Fraser999 and joroshiba May 3, 2024 14:09

Fraser999 reviewed May 6, 2024

View reviewed changes

Lilyjjo force-pushed the lilyjjo/bridge-unlock-action branch from 6921aaf to d8b3e1c Compare May 6, 2024 14:31

Lilyjjo temporarily deployed to BUF May 6, 2024 14:32 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 6, 2024 14:40 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 6, 2024 14:57 — with GitHub Actions Inactive

noot reviewed May 8, 2024

View reviewed changes

Lilyjjo force-pushed the lilyjjo/bridge-unlock-action branch from 866e53d to fd71308 Compare May 9, 2024 11:35

Lilyjjo temporarily deployed to BUF May 9, 2024 11:35 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 9, 2024 12:18 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 9, 2024 12:19 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 9, 2024 12:37 — with GitHub Actions Inactive

Lilyjjo temporarily deployed to BUF May 9, 2024 12:39 — with GitHub Actions Inactive

Lilyjjo requested a review from noot May 9, 2024 12:42

Lilyjjo force-pushed the lilyjjo/bridge-unlock-action branch from 409d494 to 44be7d3 Compare May 13, 2024 16:28

Lilyjjo temporarily deployed to BUF May 13, 2024 16:28 — with GitHub Actions Inactive

Lilyjjo requested a review from SuperFluffy as a code owner May 13, 2024 18:29

Lilyjjo temporarily deployed to BUF May 13, 2024 18:29 — with GitHub Actions Inactive

noot reviewed May 13, 2024

View reviewed changes

noot approved these changes May 13, 2024

View reviewed changes

Lilyjjo added 9 commits May 14, 2024 08:58

feat(sequencer)!: add bridge unlock action

3b1ddcc

nit(sequnecer): address comments from @Fraser999

f0df6d0

nit(sequnecer): address comments from @Fraser999

020ad87

feat(sequencer): remove gating logic and nits

b7d1a8f

feat(sequnecer): proto updates

70657ce

nit(sequencer): fix formatting

e32e148

nits

83e4788

nits

af3dee4

test(sequencer): add BridgeUnlock to breaking change list

75e3e59

Lilyjjo force-pushed the lilyjjo/bridge-unlock-action branch from 1c58448 to 75e3e59 Compare May 14, 2024 12:58

Lilyjjo temporarily deployed to BUF May 14, 2024 12:59 — with GitHub Actions Inactive

nits(sequencer): suggested nits from @noot

e39c8e9

Lilyjjo temporarily deployed to BUF May 14, 2024 13:03 — with GitHub Actions Inactive

Lilyjjo enabled auto-merge May 14, 2024 13:05

Lilyjjo added this pull request to the merge queue May 14, 2024

Merged via the queue into main with commit dbd3aec May 14, 2024
36 checks passed

Lilyjjo deleted the lilyjjo/bridge-unlock-action branch May 14, 2024 13:14

itamarreif added the bridging label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer)!: implement bridge unlock action and derestrict transfers #1034

feat(sequencer)!: implement bridge unlock action and derestrict transfers #1034

Lilyjjo commented May 2, 2024 •

edited

Loading

Fraser999 left a comment

Fraser999 May 3, 2024

Fraser999 May 4, 2024

noot May 8, 2024

noot May 8, 2024

noot May 8, 2024

noot May 8, 2024

noot May 8, 2024

noot May 13, 2024

Lilyjjo May 14, 2024 •

edited

Loading

noot May 13, 2024

noot May 13, 2024

noot left a comment

	// the address of the bridge account to transfer to
	// the address to withdraw funds to

	.expect("failed while executing bridge unlock action");
	.expect("executing bridge unlock action should succeed");

	.expect("failed while getting account balance"),
	.expect("getting account balance should succeed"),

feat(sequencer)!: implement bridge unlock action and derestrict transfers #1034

feat(sequencer)!: implement bridge unlock action and derestrict transfers #1034

Conversation

Lilyjjo commented May 2, 2024 • edited Loading

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Fraser999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lilyjjo May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

Lilyjjo commented May 2, 2024 •

edited

Loading

Lilyjjo May 14, 2024 •

edited

Loading