fix(ethrpc): log client rejections below ERROR and surface whitelist detail#322
fix(ethrpc): log client rejections below ERROR and surface whitelist detail#322dhyaniarun1993 wants to merge 1 commit into
Conversation
…detail The eth-rpc facade logged every failed method at ERROR, so routine wallet probing — MetaMask polling stale imported tokens, calling unsupported methods (permit selectors), sending contract-creation-shaped eth_calls — produced a stream of ERROR lines that buried genuine faults. It also made a non-whitelisted sender's eth_sendRawTransaction rejection log only the generic "request forbidden", hiding which address was rejected. - Add logErr helper that picks severity by apperr category: server/internal errors stay ERROR; client-category errors (bad request, not-supported, forbidden) log at Debug for read-path probes (Call) and Warn for transfer attempts (SendRawTransaction), which are operator-relevant but not faults. - checkWhitelist now wraps the detail as the error too, so ServiceError.Error() (and thus the log line) shows "sender not whitelisted: 0x..." instead of the generic message. - Add log_test.go asserting the Debug/Warn/Error classification and that the rejected address reaches the log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Ethereum RPC service logging decorator to use a new logErr helper, which prevents routine client-side errors from being logged as server-side errors. It also adds comprehensive unit tests to verify the logging severity levels and ensures that whitelist rejection details are preserved in the logs. The feedback suggests avoiding in-place slice mutation of the fields parameter in logErr using 3-index slicing to prevent potential side effects on the caller's slice, along with adding a defensive nil check for the error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (l *logService) logErr(failedMsg string, warnOnClientErr bool, err error, fields []zap.Field) { | ||
| fields = append(fields, zap.Error(err)) | ||
| switch { | ||
| case apperr.IsInternalError(err): | ||
| l.logger.Error(failedMsg, fields...) | ||
| case warnOnClientErr: | ||
| l.logger.Warn(failedMsg, fields...) | ||
| default: | ||
| l.logger.Debug(failedMsg, fields...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Mutating a slice parameter in-place via append is a Go anti-pattern that can lead to subtle bugs if the caller's slice has extra capacity or is reused in the future. Additionally, adding a defensive nil check for err prevents potential issues if logErr is ever called with a nil error.
Using 3-index slicing (fields[:len(fields):len(fields)]) forces a copy of the slice when appending, ensuring the caller's underlying array is never mutated.
func (l *logService) logErr(failedMsg string, warnOnClientErr bool, err error, fields []zap.Field) {
if err == nil {
return
}
fields = append(fields[:len(fields):len(fields)], zap.Error(err))
switch {
case apperr.IsInternalError(err):
l.logger.Error(failedMsg, fields...)
case warnOnClientErr:
l.logger.Warn(failedMsg, fields...)
default:
l.logger.Debug(failedMsg, fields...)
}
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
=======================================
Coverage ? 31.46%
=======================================
Files ? 153
Lines ? 11492
Branches ? 0
=======================================
Hits ? 3616
Misses ? 7592
Partials ? 284
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
salindne
left a comment
salindne
left a comment
There was a problem hiding this comment.
Maybe check gemini's suggestions, otherwise looks good to me
3 participants
No description provided.