-
Notifications
You must be signed in to change notification settings - Fork 924
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(modstate): Remove IsStopped endpoint from StateModule #2912
Conversation
remove comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Chirag018, thank you for your contribution. Thank you for your contribution. Unfortunately, it will break the logic of handling POST requests in the gateway. The condition if r.Method == http.MethodPost && state.IsStopped(r.Context())
disables all POST requests only if the state module was stopped when a Bad Encoding Fraud Proof is received, and the network halts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good minus the comment left by @vgonkivs
Please also lint and let's run CI against it.
@Chirag018 , could you please fix lint errors? |
yeah, will have a look on it |
@vgonkivs, While addressing lint issues, I identified code duplication in the state file. To resolve this, should I refactor the API.Interface struct or make other related changes? cc: @Wondertan @renaynay |
@Chirag018, refactoring that struct should do the thing. cc @distractedm1nd for confirmation |
OK, will wait for @distractedm1nd 's confirmation |
Yes, it needs to be removed from the API struct as well as the Module interface - don't forget to remove the implementations as well :) That should do the trick |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, it still has the duplicate lint issue on the state file.
Maybe it needs a //nolint:dupl
not sure why it's coming up now and wasn't caught before.
@Chirag018 do you still want to finish up this PR? Just add |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2912 +/- ##
=======================================
Coverage 51.29% 51.29%
=======================================
Files 177 177
Lines 11151 11139 -12
=======================================
- Hits 5720 5714 -6
+ Misses 4935 4924 -11
- Partials 496 501 +5 ☔ View full report in Codecov by Sentry. |
@Chirag018 can you rebase main onto here? Or at least adjust the PR so we can update the PR with base branch, then this'll merge. Thanks! |
@Wondertan is this what you expected? Closes #2906
…org#2912) @Wondertan is this what you expected? Closes celestiaorg#2906
…org#2912) @Wondertan is this what you expected? Closes celestiaorg#2906
@Wondertan is this what you expected?
Closes #2906