fix, refactor(sequencer): refactor ics20 logic by SuperFluffy · Pull Request #1495 · astriaorg/astria

SuperFluffy · 2024-09-12T12:22:30Z

Summary

Fixes incorrect addresses in ics20 refunds, and fixes/adds source vs sink zone detection of received/refunded asets.

Background

This PR started with the goal of using Ics20WithdrawalFromRollup.rollup_return_address when emitting ics20 deposit events instead of the rollup's bridge address (which for the rollup was quite meaningless).

However, because the original logic was overly convoluted, this patch evolved into a refactor which subsequently revealed 2 more bugs.

Changes

Refactor ics20 logic: split up execute_ics20_transfer into two functions receive_tokens and refund_tokens
Fix deposit events being emitted with the packet sender/bridge address as the receiving address on the rollup: instead use Ics20WithdrawalFromRollup.rollup_return_address which is the address originally supplied and understood by the rollup.
Fix bridge lock events of source tokens being emitted with an extra (port, channel) pair added (this resulting token is likely completely meaningless and not understood by the rollup): instead strip the leading (port, channel) pair to get the original token on sequencer.
Fix refunds to a rollup of failed ics20 transfers not checking if sequencer is their source or sink zone: instead perform the check and decrease the ibc escrow account is necessary.

Testing

Renamed and expanded tests to also check for sink vs source zone assets being received, the correct asset being emitted in bridge lock events, and the correct rollup return address being emitted in refunds. Specifically these tests were added or renamed:

receive_source_zone_asset_on_sequencer_account
receive_sink_zone_asset_on_sequencer_account
receive_source_zone_asset_on_bridge_account_and_emit_to_rollup
receive_sink_zone_asset_on_bridge_account_and_emit_to_rollup
transfer_to_bridge_is_rejected_due_to_invalid_memo
transfer_to_bridge_account_is_rejected_due_to_not_permitted_token
refund_sequencer_account_with_source_zone_asset
refund_sequencer_account_with_sink_zone_asset
refund_rollup_with_sink_zone_asset
refund_rollup_with_source_zone_asset
refund_rollup_with_source_zone_asset_compat_prefix

Related Issues

Closes #1439
Closes #1496
Closes #1514

SuperFluffy · 2024-09-12T13:03:19Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+    /// assert!(!denom.has_leading_port(""));
+    /// ```
+    #[must_use]
+    pub fn has_leading_port<T: AsRef<str>>(&self, port: T) -> bool {


Added has_leading_port and has_leading_channel to have a stricter prefix check instead of starts_with_str.

SuperFluffy · 2024-09-12T13:03:25Z

crates/astria-core/src/primitive/v1/asset/denom.rs

-            return false;
-        }
-        parts.next().is_none()
+    pub fn has_leading_channel<T: AsRef<str>>(&self, channel: T) -> bool {


Added has_leading_port and has_leading_channel to have a stricter prefix check instead of starts_with_str.

SuperFluffy · 2024-09-12T13:04:43Z

crates/astria-core/src/sequencerblock/v1alpha1/block.rs

@@ -1291,86 +1291,23 @@ impl FilteredSequencerBlockError {
 )]
 pub struct Deposit {


This is just a stupid container without any invariants.

Made all fields public and removed the getters.

SuperFluffy · 2024-09-12T13:05:24Z

crates/astria-core/src/sequencerblock/v1alpha1/block.rs


 impl Deposit {
-    #[must_use]
-    pub fn new(


Terrible evergrowing constructor. Since there are no invariants to be upheld I removed it in favor of directly constructing Deposit from public fields.

SuperFluffy · 2024-09-12T13:05:58Z

crates/astria-core/src/sequencerblock/v1alpha1/block.rs

    }
 }

+impl From<Deposit> for crate::generated::sequencerblock::v1alpha1::Deposit {


Not new, just moved down from above; this is an artifact of the refactor, but the trait impl fits better down here.

SuperFluffy · 2024-09-12T13:07:13Z

crates/astria-sequencer/src/api_state_ext.rs

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:07:18Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:07:30Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:07:34Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:07:39Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:07:56Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:08:19Z

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

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:11:41Z

crates/astria-sequencer/src/ibc/state_ext.rs

 #[async_trait]
 pub(crate) trait StateWriteExt: StateWrite {
+    #[instrument(skip_all, fields(%channel))]
+    async fn decrease_ibc_channel_balance<TAsset>(


New utility to atomically read-update-write the balance on the escrow account.

SuperFluffy · 2024-09-12T13:11:53Z

crates/astria-sequencer/src/transaction/checks.rs

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

SuperFluffy · 2024-09-12T13:12:10Z

crates/astria-sequencer/src/utils.rs

contains only changes due to deposit losing its getters

SuperFluffy · 2024-09-12T13:14:25Z

crates/astria-sequencer/src/ibc/ics20_transfer.rs

+        let ack: TokenTransferAcknowledgement =
+            serde_json::from_slice(msg.acknowledgement.as_slice())
+                .context("failed to deserialize token transfer acknowledgement")?;
+        if !ack.is_successful() {


This just flips the logic: instead of returning early on success, success is now the fall-through case while failure triggers a refund.

SuperFluffy · 2024-09-12T13:16:53Z

crates/astria-sequencer/src/ibc/ics20_transfer.rs

This file should probably not be reviewed using its diff, but instead read from source.

As a high level overview:

execute_ics20_transfer (that branched on an argument is_refund) is split up into receive_tokens and refund_tokens.

Both functions were cleaned up and should be much easier to read.

SuperFluffy · 2024-09-12T13:21:02Z

crates/astria-sequencer/src/ibc/ics20_transfer.rs

-
-    // check if this is a transfer to a bridge account and
-    // execute relevant state changes if it is
-    execute_ics20_transfer_bridge_lock(


I believe this function was incorrect: its argument trace_with_dest = dest_port/dest_channel/source_port/source_channel/asset was always constructed irrespective of whether sequencer was a source or a sink zone, but the construction is only correct if sequencer is a sink!

If sequencer is a source, then dest_port/dest_channel must not be prepended, and instead source_port/soure_channel removed, leaving just asset.

SuperFluffy · 2024-09-12T13:22:34Z

crates/astria-sequencer/src/ibc/ics20_transfer.rs

+        &mut state,
        bridge_address,
-        &denom,
+        memo.rollup_return_address,


This is the actual fix for the incorrect deposit: instead of bridge_address being used twice, the memo.rollup_return_address is now put into destination_chain_address field.

SuperFluffy · 2024-09-12T13:25:03Z

crates/astria-sequencer-utils/src/blob_parser.rs

contains only changes due to Deposit::new being replaced by direction declaration Deposit {}.

crates/astria-sequencer/src/ibc/ics20_transfer.rs

SuperFluffy · 2024-09-18T12:35:27Z

Rebased on top of recent main.

In addition I instrumented all code that's called during ics20 execution, injecting address information, assets, amounts, etc into the generated spans. Function calls using eyre Reports as errors also generates an event in error cases.

crates/astria-sequencer/src/ibc/ics20_transfer.rs

noot

looks good to me :D

Co-authored-by: noot <36753753+noot@users.noreply.github.com>

## Summary Fixes incorrect addresses in ics20 refunds, and fixes/adds source vs sink zone detection of received/refunded asets. ## Background This PR started with the goal of using `Ics20WithdrawalFromRollup.rollup_return_address` when emitting ics20 deposit events instead of the rollup's bridge address (which for the rollup was quite meaningless). However, because the original logic was overly convoluted, this patch evolved into a refactor which subsequently revealed 2 more bugs. ## Changes - Refactor ics20 logic: split up `execute_ics20_transfer` into two functions `receive_tokens` and `refund_tokens` - Fix deposit events being emitted with the packet sender/bridge address as the receiving address on the rollup: instead use `Ics20WithdrawalFromRollup.rollup_return_address` which is the address originally supplied and understood by the rollup. - Fix bridge lock events of source tokens being emitted with an extra (port, channel) pair added (this resulting token is likely completely meaningless and not understood by the rollup): instead strip the leading (port, channel) pair to get the original token on sequencer. - Fix refunds to a rollup of failed ics20 transfers not checking if sequencer is their source or sink zone: instead perform the check and decrease the ibc escrow account is necessary. ## Testing Renamed and expanded tests to also check for sink vs source zone assets being received, the correct asset being emitted in bridge lock events, and the correct rollup return address being emitted in refunds. Specifically these tests were added or renamed: - `receive_source_zone_asset_on_sequencer_account` - `receive_sink_zone_asset_on_sequencer_account` - `receive_source_zone_asset_on_bridge_account_and_emit_to_rollup` - `receive_sink_zone_asset_on_bridge_account_and_emit_to_rollup` - `transfer_to_bridge_is_rejected_due_to_invalid_memo` - `transfer_to_bridge_account_is_rejected_due_to_not_permitted_token` - `refund_sequencer_account_with_source_zone_asset` - `refund_sequencer_account_with_sink_zone_asset` - `refund_rollup_with_sink_zone_asset` - `refund_rollup_with_source_zone_asset` - `refund_rollup_with_source_zone_asset_compat_prefix` ## Related Issues Closes #1439 Closes #1496 Closes #1514 --------- Co-authored-by: noot <36753753+noot@users.noreply.github.com>

* main: feat(conductor): implement restart logic (#1463) fix: ignore `RUSTSEC-2024-0370` (#1483) fix, refactor(sequencer): refactor ics20 logic (#1495) fix(ci): use commit SHA instead of PR number preview-env images (#1501) chore(bridge-withdrawer): pass GRPC and CometBFT clients to consumers directly (#1510) fix(sequencer): Fix incorrect error message from BridgeUnlock actions (#1505) fix(bridge-contracts): fix memo transaction hash encoding (#1428) fix: build docker when workflow explicitly includes component (#1498) chore(sequencer): migrate from `anyhow::Result` to `eyre::Result` (#1387) fix(ci): typo for required field in sequencer preview-env (#1500) feat(ci): provide demo/preview environments (#1406)

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Sep 12, 2024

SuperFluffy force-pushed the ENG-711/use_rollup_address_for_refunds branch from 9ed086c to 7476516 Compare September 12, 2024 13:02

SuperFluffy commented Sep 12, 2024

View reviewed changes

crates/astria-sequencer/src/utils.rs

Copy link
Copy Markdown

Contributor Author

SuperFluffy Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contains only changes due to deposit losing its getters

SuperFluffy commented Sep 12, 2024

View reviewed changes

SuperFluffy force-pushed the ENG-711/use_rollup_address_for_refunds branch from 7476516 to 34e8132 Compare September 12, 2024 13:24

SuperFluffy commented Sep 12, 2024

View reviewed changes

SuperFluffy force-pushed the ENG-711/use_rollup_address_for_refunds branch from 34e8132 to b8d2f27 Compare September 12, 2024 13:56

SuperFluffy marked this pull request as ready for review September 12, 2024 13:56

SuperFluffy requested a review from a team as a code owner September 12, 2024 13:56

SuperFluffy requested review from Fraser999 and noot September 12, 2024 13:56

noot reviewed Sep 16, 2024

View reviewed changes

crates/astria-sequencer/src/ibc/ics20_transfer.rs Outdated Show resolved Hide resolved

noot reviewed Sep 16, 2024

View reviewed changes

crates/astria-sequencer/src/ibc/ics20_transfer.rs Outdated Show resolved Hide resolved

check source vs sink zone when refunding rollups

5673d04

SuperFluffy force-pushed the ENG-711/use_rollup_address_for_refunds branch from 0b09c08 to 5673d04 Compare September 18, 2024 10:24

allow clippy lints

b3058d9

SuperFluffy mentioned this pull request Sep 18, 2024

Refunding rollups on failed ics20 transfers does not check if sequencer is source or sink of an asset #1514

Closed

make names of tests clearer

9996204

SuperFluffy requested a review from noot September 19, 2024 12:40

SuperFluffy mentioned this pull request Sep 19, 2024

chore(sequencer): Add instrumentation #1368

Closed

noot reviewed Sep 19, 2024

View reviewed changes

crates/astria-sequencer/src/ibc/ics20_transfer.rs Show resolved Hide resolved

noot approved these changes Sep 19, 2024

View reviewed changes

SuperFluffy enabled auto-merge September 19, 2024 16:44

SuperFluffy disabled auto-merge September 19, 2024 16:45

SuperFluffy and others added 2 commits September 19, 2024 18:46

Update crates/astria-sequencer/src/ibc/ics20_transfer.rs

ac7121f

Co-authored-by: noot <36753753+noot@users.noreply.github.com>

more minor fixes

be88e99

SuperFluffy force-pushed the ENG-711/use_rollup_address_for_refunds branch from 15efaec to be88e99 Compare September 19, 2024 16:51

unused import

bf135c3

SuperFluffy enabled auto-merge September 19, 2024 16:55

SuperFluffy added this pull request to the merge queue Sep 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2024

SuperFluffy added this pull request to the merge queue Sep 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2024

SuperFluffy added this pull request to the merge queue Sep 19, 2024

Merged via the queue into main with commit c38ce8e Sep 19, 2024

SuperFluffy deleted the ENG-711/use_rollup_address_for_refunds branch September 19, 2024 19:54

This was referenced Sep 24, 2024

Clean up deposit constructor #1448

Closed

chore: refactor execute_ics20_transfer() #1331

Closed

noot mentioned this pull request Oct 1, 2024

Simplify ics20 bridge lock transfer by moving refund check one level up #1420

Closed

		@@ -1291,86 +1291,23 @@ impl FilteredSequencerBlockError {
		)]
		pub struct Deposit {

Conversation

SuperFluffy commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Related Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SuperFluffy commented Sep 18, 2024

Uh oh!

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SuperFluffy commented Sep 12, 2024 •

edited

Loading

SuperFluffy Sep 12, 2024 •

edited

Loading