Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(sequencer): migrate from anyhow::Result to eyre::Result #1387

Merged
merged 19 commits into from
Sep 13, 2024

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Aug 21, 2024

Summary

Migrate all instances of anyhow::Result to eyre::Result.

Background

Sequencer was using anyhow::Result, which provides an unhelpful Display impl and contrasts our error handling in the rest of the codebase. This change is to flush out our error handling in the sequencer, except for those parts which necessitate using anyhow.

Changes

  • Add eyre to sequencer's cargo.toml.
  • Migrate all instances of anyhow::Result to eyre::Result, except for those that touch cnidarium directly.
  • Create anyhow_to_eyre() and eyre_to_anyhow() helper functions for moving between the two without breaking the source chain.

Testing

Added unit tests to ensure eyre and anyhow source chains are maintained when converting between one another.

Related Issues

closes #1386

@github-actions github-actions bot added ci issues that are related to ci and github workflows sequencer pertaining to the astria-sequencer crate labels Aug 21, 2024
@ethanoroshiba ethanoroshiba changed the base branch from main to ENG-670/add_sequencer_instrumentation August 21, 2024 17:08
@ethanoroshiba ethanoroshiba marked this pull request as ready for review August 21, 2024 17:39
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner August 21, 2024 17:39
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

This approach breaks the error source chain, see proof below. (on a side note: please base the instrumentation fix in ENG-670 on this change from anyhow to eyre, not the other way round)

We need to find a way to turn anyhow:Error into a Box<dyn std::error::Error> and then wrap that in eyre (which sounds easier then it is in practice): https://docs.rs/anyhow/latest/anyhow/struct.Error.html#impl-From%3CError%3E-for-Box%3Cdyn+Error+%2B+Send+%2B+Sync%3E

Proof:

eyre-wrapped anyhow error:
Err(wrapped

Caused by:
    level 2

Location:
    src/main.rs:8:55)

eyre-wrapped anyhow error: broken source chain
0: wrapped
1: level 2
eyre error:
Err(wrapped

Caused by:
   0: level 2
   1: level 1
   2: level 0

Location:
    src/main.rs:6:33)
eyre-only error: functioning source chain
0: wrapped
1: level 2
2: level 1
3: level 0

Code used to generate this:

fn main() {
    let anyhow_err = Err::<(), _>(anyhow!("level 0").context("level 1").context("level 2"));
    let eyre_err = Err::<(), _>(eyre!("level 0").wrap_err("level 1").wrap_err("level 2"));

    let wrapped_anyhow_err = anyhow_err.map_err(|err| eyre!(err).wrap_err("wrapped"));
    println!("eyre-wrapped anyhow error:\n{wrapped_anyhow_err:?}\n");

    println!("eyre-wrapped anyhow error: broken source chain");
    for (i, source) in wrapped_anyhow_err.unwrap_err().chain().enumerate() {
        println!("{i}: {source}");
    }

    let wrapped_eyre_err = eyre_err.wrap_err("wrapped");
    println!("eyre error:\n{wrapped_eyre_err:?}");

    println!("eyre-only error: functioning source chain");
    for (i, source) in wrapped_eyre_err.unwrap_err().chain().enumerate() {
        println!("{i}: {source}");
    }

crates/astria-sequencer/Cargo.toml Outdated Show resolved Hide resolved
crates/astria-sequencer/src/bridge/state_ext.rs Outdated Show resolved Hide resolved
@ethanoroshiba ethanoroshiba requested review from a team, joroshiba and noot as code owners August 22, 2024 14:59
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec cd labels Aug 22, 2024
@ethanoroshiba ethanoroshiba changed the base branch from ENG-670/add_sequencer_instrumentation to main August 22, 2024 14:59
@ethanoroshiba ethanoroshiba added ci issues that are related to ci and github workflows and removed ci issues that are related to ci and github workflows conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec cd labels Aug 22, 2024
@ethanoroshiba ethanoroshiba removed request for a team, joroshiba and noot August 22, 2024 15:00
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Thank you for going through the entire codebase and making these changes! I think we should go ahead and merge this.

I have left a few comments. Please address them before merging.

@ethanoroshiba ethanoroshiba added this pull request to the merge queue Sep 13, 2024
@ethanoroshiba ethanoroshiba removed this pull request from the merge queue due to a manual request Sep 13, 2024
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Sep 13, 2024
Merged via the queue into main with commit ac7222e Sep 13, 2024
42 checks passed
@ethanoroshiba ethanoroshiba deleted the ENG-735/anyhow_to_eyre branch September 13, 2024 15:59
steezeburger added a commit that referenced this pull request Sep 23, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci issues that are related to ci and github workflows sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transition sequencer to eyre results
3 participants