-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
op-proposer: service lifecycle cleanup #8040
Conversation
Thank you, this refactor looks great. I'm catching up with some more PR review after having been OOO for a while, but will take a look at this soon! |
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.
Structure looks good, but this needs several golint fixes. run make lint-go
from the top level of the repo to verify that they are fixed. Best practice is also to split up moving code from modifying code - it's not easy to quickly check that the function for l2_output_submitter.go
got correctly moved over to driver.go
Adding the Admin RPC is also very nice, but again, a little harder to review when including it with the other refactor.
op-e2e/faultproof_test.go:535:55: Error return value of `(github.com/ethereum-optimism/optimism/op-proposer/proposer/rpc.ProposerDriver).StopL2OutputSubmitting` is not checked (errcheck)
sys.L2OutputSubmitter.Driver().StopL2OutputSubmitting()
^
op-e2e/system_fpp_test.go:262:55: Error return value of `(github.com/ethereum-optimism/optimism/op-proposer/proposer/rpc.ProposerDriver).StopL2OutputSubmitting` is not checked (errcheck)
sys.L2OutputSubmitter.Driver().StopL2OutputSubmitting()
^
op-proposer/metrics/noop.go:30: File is not `goimports`-ed (goimports)
}
op-proposer/proposer/config.go:9: File is not `goimports`-ed (goimports)
op-proposer/proposer/driver.go:12: File is not `goimports`-ed (goimports)
Thank you @trianglesphere for the advice. Next time, I will open two separate pull requests to make the review process smoother :) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #8040 +/- ##
===========================================
- Coverage 53.78% 52.71% -1.07%
===========================================
Files 162 162
Lines 6171 6171
Branches 966 966
===========================================
- Hits 3319 3253 -66
- Misses 2728 2763 +35
- Partials 124 155 +31
Flags with carried forward coverage won't be shown. Click here to find out more. |
@joohhnnn there's a couple more lints & one merge conflict to fix. |
@trianglesphere Are you ok to do a final review on this or do you want me to dive in and review (I'd have to come up to speed on the general context of the proposer)? |
afea36e
In reference to pull/7682, I've addressed portions of issue 7671 / 7684, within the proposer code. I've strived to maintain a coding style consistent with pull/7682, though I'm not certain if anything has been overlooked.