fix(cli, bridge-withdrawer): dont fail entire block due to bad withdraw event by itamarreif · Pull Request #1409 · astriaorg/astria

itamarreif · 2024-08-27T00:37:34Z

Summary

Brief summary of the changes made, ie "what?"

Background

Brief background on why these changes were made, ie "why?"

Changes

Separate the errors for getting logs from the conversion errors
Conversion returns a Result<Vec<Result<Action, WithdrawalConversionError>>, GetWithdrawalLogsError> instead of Result<Vec<Action>, GetWithdrawalActionsError>
Handle failed conversions in the cli and withdrawer crates

Testing

Unit tests for the conversions will be added in a subsequent PR:

Breaking Changelist

This fixes the bug described in Bridge withdrawer shouldnt panic on bad single withdrawal address #1251, where a single withdrawal which fails to be converted to an action will cause the entire rollup block's batch of withdrawals to fail, causing the withdrawer to be stuck.

Related Issues

closes #1251

joroshiba

LGTM

SuperFluffy

This PR contains a lot of renames that I don't think are necessary (and partially confusing, talking about "logs" where "actions" are promised).

SuperFluffy · 2024-08-29T18:47:27Z

crates/astria-bridge-contracts/src/lib.rs

+    /// Converts a rollup-side smart contract event log to a sequencer-side ics20 withdrawal action.
+    ///
+    /// # Errors
+    /// - If the log does not contain a block number.
+    /// - If the log does not contain a transaction hash.
+    /// - If the log cannot be decoded as a sequencer withdrawal event.
+    /// - If the log does not contain a `recipient` field.
+    /// - If the memo cannot be encoded to json.
+    /// - If calculating the amount using the asset withdrawal divisor overflows.
+    ///
+    /// # Panics
+    /// - If getting a withdrawal event type that it was not configured for. For example, if the
+    ///   configuration was specified to only get sequencer withdrawal events but an ICS20
+    ///   withdrawal is received.
+    pub fn log_to_ics20_withdrawal_action(


I don't see why this needs to be a pub method? I don't think it's used anywhere outside this lib? You can then also remove the doc comment.

SuperFluffy · 2024-08-29T18:49:49Z

crates/astria-bridge-contracts/src/lib.rs

+}
+
+#[derive(Debug, thiserror::Error)]
+enum GetWithdrawalLogsErrorKind {


Making this an enum seems unnecessary?

SuperFluffy · 2024-08-29T18:49:57Z

crates/astria-bridge-contracts/src/lib.rs

 #[derive(Debug, thiserror::Error)]
 #[error(transparent)]
-pub struct GetWithdrawalActionsError(GetWithdrawalActionsErrorKind);
+pub struct GetWithdrawalLogsError(GetWithdrawalLogsErrorKind);


I think you are leaking concepts that don't appear at the high level. Please change this back.

SuperFluffy · 2024-08-29T18:52:55Z

crates/astria-bridge-contracts/src/lib.rs

+    /// Converts a rollup-side smart contract event log to a sequencer-side ics20 withdrawal action.
+    ///
+    /// # Errors
+    /// - If the log does not contain a block number.
+    /// - If the log does not contain a transaction hash.
+    /// - If the log cannot be decoded as a sequencer withdrawal event.
+    /// - If the log does not contain a `recipient` field.
+    /// - If the memo cannot be encoded to json.
+    /// - If calculating the amount using the asset withdrawal divisor overflows.
+    ///
+    /// # Panics
+    /// - If getting a withdrawal event type that it was not configured for. For example, if the
+    ///   configuration was specified to only get sequencer withdrawal events but an ICS20
+    ///   withdrawal is received.


Remove this doc comment.

SuperFluffy · 2024-08-29T18:54:36Z

crates/astria-cli/src/commands/bridge/collect.rs

        .wrap_err_with(|| {
            format!(
-                "failed getting actions for block; block hash: `{block_hash}`, block height: \
+                "failed getting logs for block; block hash: `{block_hash}`, block height: \


This is not correct. The actions_fetcher is promising to give you actions. Why is this talking about logs? That's confusing.

SuperFluffy · 2024-08-29T18:56:04Z

crates/astria-cli/src/commands/bridge/collect.rs

-        })?;
+        })?
+        .into_iter()
+        .filter_map(|r| {


I am ambivalent about changing this logic in the CLI at this point. I am fine with adding a flag that filters out bad actions, but I would leave this to a followup.

SuperFluffy

Nice, this is a much neater change. Thank you!

The message in the error event looks a bit wonky. See my suggestion.

crates/astria-bridge-withdrawer/src/bridge_withdrawer/ethereum/watcher.rs

SuperFluffy · 2024-08-29T21:18:02Z

crates/astria-cli/src/commands/bridge/collect.rs

-        })?;
+        })?
+        .into_iter()
+        .collect::<Result<_, _>>()?;


Not the most efficient, but I guess it doesn't matter in this case.

crates/astria-bridge-withdrawer/src/bridge_withdrawer/ethereum/watcher.rs

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

* main: chore: ibc e2e smoke test (#1284) chore(metrics): restrict `metrics` crate usage to `astria-telemetry` (#1192) fix(charts)!: sequencer-relayer chart correct startup env var (#1437) chore(bridge-withdrawer): Add instrumentation (#1324) chore(conductor): Add instrumentation (#1330) fix(cli, bridge-withdrawer): dont fail entire block due to bad withdraw event (#1409) feat(sequencer, bridge-withdrawer)!: enforce withdrawals consumed (#1391)

…aw event (#1409) ## Summary Brief summary of the changes made, ie "what?" ## Background Brief background on why these changes were made, ie "why?" ## Changes - Separate the errors for getting logs from the conversion errors - Conversion returns a `Result<Vec<Result<Action, WithdrawalConversionError>>, GetWithdrawalLogsError> ` instead of `Result<Vec<Action>, GetWithdrawalActionsError>` - Handle failed conversions in the cli and withdrawer crates ## Testing - Unit tests for the conversions will be added in a subsequent PR: ## Breaking Changelist - This fixes the bug described in #1251, where a single withdrawal which fails to be converted to an action will cause the entire rollup block's batch of withdrawals to fail, causing the withdrawer to be stuck. ## Related Issues closes #1251 --------- Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

itamarreif self-assigned this Aug 27, 2024

itamarreif added bug Something isn't working bridging bridge-withdrawer code touching the bridge-withdrawer service labels Aug 27, 2024

itamarreif force-pushed the itamarreif/bridge-withdrawer/address-fix branch from 7e94407 to dd28ebb Compare August 28, 2024 20:27

itamarreif marked this pull request as ready for review August 28, 2024 20:39

itamarreif requested a review from a team as a code owner August 28, 2024 20:39

itamarreif requested a review from noot August 28, 2024 20:39

itamarreif force-pushed the itamarreif/bridge-withdrawer/address-fix branch from 842d582 to 55eb921 Compare August 28, 2024 22:26

joroshiba reviewed Aug 29, 2024

View reviewed changes

itamarreif added the ENG-674 Zelic's July 15 audit label Aug 29, 2024

SuperFluffy suggested changes Aug 29, 2024

View reviewed changes

itamarreif force-pushed the itamarreif/bridge-withdrawer/address-fix branch from 55eb921 to 2e6c958 Compare August 29, 2024 21:08

SuperFluffy approved these changes Aug 29, 2024

View reviewed changes

itamarreif force-pushed the itamarreif/bridge-withdrawer/address-fix branch from 56235ae to e2bb4cf Compare August 29, 2024 21:29

itamarreif and others added 3 commits August 29, 2024 17:30

smallest fix

29f2483

fix error encoding

daf97e4

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

droppped error

b2d4087

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

itamarreif force-pushed the itamarreif/bridge-withdrawer/address-fix branch from e2bb4cf to b2d4087 Compare August 29, 2024 21:30

itamarreif enabled auto-merge August 29, 2024 21:30

itamarreif added this pull request to the merge queue Aug 29, 2024

Merged via the queue into main with commit 3e24c50 Aug 29, 2024

itamarreif deleted the itamarreif/bridge-withdrawer/address-fix branch August 29, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli, bridge-withdrawer): dont fail entire block due to bad withdraw event#1409

fix(cli, bridge-withdrawer): dont fail entire block due to bad withdraw event#1409
itamarreif merged 3 commits intomainfrom
itamarreif/bridge-withdrawer/address-fix

itamarreif commented Aug 27, 2024 •

edited

Loading

Uh oh!

joroshiba left a comment

Uh oh!

SuperFluffy left a comment

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

SuperFluffy left a comment

Uh oh!

Uh oh!

SuperFluffy Aug 29, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

itamarreif commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SuperFluffy Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

itamarreif commented Aug 27, 2024 •

edited

Loading