refactor(i): Lint casts & move panics under must funcs

[shahzadlone]

Relevant issue(s)

Resolves #3103

Description

• Fix the BadRequest in the acp handlers to now be panics
• Make the panics documented under must functions
• Add the linter to avoid increasing of undocumented panics
• Ignore linter in existing files outside cli and http (this should either be resolved gradually over time or in a single PR, regardless would prefer to not block this PR because of them)

[codecov]

Codecov Report

Attention: Patch coverage is 75.82418% with 22 lines in your changes missing coverage. Please review.

Project coverage is 77.79%. Comparing base (47d38a8) to head (ec7e43d).

Files with missing lines	Patch %	Lines
http/handler_tx.go	56.52%	8 Missing and 2 partials ⚠️
http/handler.go	33.33%	3 Missing and 1 partial ⚠️
http/handler_lens.go	0.00%	4 Missing ⚠️
http/handler_collection.go	80.00%	2 Missing ⚠️
http/handler_store.go	85.71%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3134      +/-   ##
===========================================
- Coverage    78.10%   77.79%   -0.31%     
===========================================
  Files          354      354              
  Lines        34660    34678      +18     
===========================================
- Hits         27071    26977      -94     
- Misses        5972     6064      +92     
- Partials      1617     1637      +20     

Flag	Coverage Δ
all-tests	77.79% <75.82%> (-0.31%)	⬇️

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

Files with missing lines	Coverage Δ
cli/p2p_info.go	100.00% <100.00%> (ø)
cli/purge.go	93.33% <100.00%> (ø)
cli/utils.go	70.27% <100.00%> (+0.55%)	⬆️
http/errors.go	28.00% <ø> (ø)
http/handler_acp.go	91.18% <100.00%> (+7.73%)	⬆️
http/handler_ccip.go	95.12% <100.00%> (ø)
http/handler_extras.go	100.00% <100.00%> (ø)
http/handler_p2p.go	73.21% <100.00%> (ø)
http/middleware.go	70.00% <100.00%> (ø)
http/utils.go	88.00% <100.00%> (+9.43%)	⬆️
... and 5 more

... and 15 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47d38a8...ec7e43d. Read the comment docs.

[AndrewSisley]

todo: This appears to be entirely untested, given the lack of any changes made to test files, and no note of any manual testing in the PR description. I prefer that you add tests, but if you really think manual testing is okay in the short term, please note why.

I also have a question RE the linter excluded file list.

[shahzadlone]

todo: This appears to be entirely untested, given the lack of any changes made to test files, and no note of any manual testing in the PR description. I prefer that you add tests, but if you really think manual testing is okay in the short term, please note why.

Happy to add tests if you have any specific code blocks that I should test, here are all the changes that have been done:

• Implicit casts that would panic .() moved to mustXYZ functions -> Won't change or need tests as it's the same behavior.
• HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.
• Added extra linter (testing not applicable)
• Some instances of double casts just split into multiple must/``try` checks.

[AndrewSisley]

HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.

It was this I was mostly after - might not be worth the hassle of writing tests for though as I guess it is mostly just the http status code that changed, and that's not something the integration test framework can handle atm I think. Have you manually tested this?

[fredcarle]

question: Why is this turned into a must instead of a try like what you did is some other handles?

[shahzadlone]

Only one that remained a try (not panic) is the P2P one because I think in that case it makes sense because P2P might be turned off and I see that specific error being returned by original author of the p2p code as useful for that situation as useful.

Rest all context casts are converted to a must.

[fredcarle]

todo: This isn't a BadRequest but rather an InternalServerError (500).

Out of scope of this PR but a lot of our returned statuses are wrong like this. I create #3139 to track this. If a panic within the handlers automatically returns a 500 status then it might be better to just do a must typecast and let the server panic for cases where a 500 is appropriate.

[shahzadlone]

Actually #3139 seems similar / duplicate of what I wanted #3103 to be originally (if i understand correctly) . So I would say that is very much in the scope of this PR. I can change all the required BadRequests that need changing if you can guide me to which additional ones should be changed (@nasdf suggested only for the context gets+casts originally), I can take care of them in this PR.

[shahzadlone]

HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.

It was this I was mostly after - might not be worth the hassle of writing tests for though as I guess it is mostly just the http status code that changed, and that's not something the integration test framework can handle atm I think. Have you manually tested this?

Manually some yes, but not all. I am a bit unsure how to test certain situations.