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

alt-da: remove unused error handling #11828

Closed
wants to merge 3 commits into from

Conversation

qzhodl
Copy link

@qzhodl qzhodl commented Sep 10, 2024

Description

The GetInput function of s.fetcher never returns an ErrReorgRequired error, so we can remove the handling for this error type.

@qzhodl qzhodl requested review from a team as code owners September 10, 2024 09:20
@qzhodl qzhodl changed the title Remove unused error handling alt-da: remove unused error handling Sep 12, 2024
@tynes
Copy link
Contributor

tynes commented Sep 12, 2024

This needs a rebase for new CI jobs that were added. Also tagging @samlaf here for review

Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

Seems true to me. Tried quickly looking through previous PRs for where the ErrReorgRequired was removed from GetInput but couldn't find anything.

@qzhodl
Copy link
Author

qzhodl commented Sep 13, 2024

This needs a rebase for new CI jobs that were added. Also tagging @samlaf here for review

Sure, I’ve merged the latest changes

@qzhodl qzhodl requested a review from samlaf September 18, 2024 06:43
@samlaf
Copy link
Contributor

samlaf commented Sep 18, 2024

@qzhodl Im not a member of this repo, you should request review from someone on the team. Maybe @sebastianst ?

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

I've confirmed that the altda.ErrReorgRequired error only bubbles up in the callstack of DA.AdvanceL1Origin so removing it for the GetInput call looks fine to me.

Is it guaranteed that this error won't be returned by GetInput in the future?

cc @qzhodl @samlaf

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.14%. Comparing base (fb62380) to head (72faa22).
Report is 31 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #11828      +/-   ##
===========================================
- Coverage    64.32%   64.14%   -0.19%     
===========================================
  Files           52       52              
  Lines         4348     4348              
===========================================
- Hits          2797     2789       -8     
- Misses        1376     1385       +9     
+ Partials       175      174       -1     
Flag Coverage Δ
cannon-go-tests 64.14% <ø> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@qzhodl qzhodl closed this by deleting the head repository Oct 15, 2024
@samlaf
Copy link
Contributor

samlaf commented Oct 15, 2024

@qzhodl mistake?

@qzhodl
Copy link
Author

qzhodl commented Oct 15, 2024

@qzhodl mistake?

Oh, yes, I didn’t realize the repo still has unmerged PRs...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants