-
Notifications
You must be signed in to change notification settings - Fork 210
chore: (PRO-411) (PRO-415) Clean up and sanitize DEBUG macros, logger… #232
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: (PRO-411) (PRO-415) Clean up and sanitize DEBUG macros, logger… #232
Conversation
…s, etc. to prevent sensitive information from being displayed
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.
Important
Looks good to me! 👍
Reviewed everything up to 443e702 in 2 minutes and 6 seconds. Click for details.
- Reviewed
994lines of code in14files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/validator/config_validator.rs:288
- Draft comment:
Consider replacing these 'println!' calls with structured logging (e.g. log::info!) to ensure the output is appropriate for production environments. - Reason this comment was not posted:
Comment was on unchanged code.
2. crates/lib/src/validator/config_validator.rs:224
- Draft comment:
Consider handling failures of Pubkey::from_str explicitly in the RPC validation loops for allowed tokens and programs instead of silently skipping them. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. crates/lib/src/validator/config_validator.rs:202
- Draft comment:
Review the margin validation logic: margin > 1.0 currently produces a warning – verify if this should be an error instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. crates/lib/src/validator/config_validator.rs:215
- Draft comment:
Avoid using unwrap() on the result of CacheValidator::validate; consider propagating the error to prevent potential runtime panics. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. crates/lib/src/validator/config_validator.rs:235
- Draft comment:
Ensure consistent error reporting for Pubkey parsing failures in RPC validation—currently, if Pubkey::from_str fails it is ignored. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. crates/lib/src/validator/config_validator.rs:25
- Draft comment:
The 'validate_with_result_and_signers' function is quite lengthy; consider refactoring it into smaller helper functions to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. crates/lib/src/signer/config.rs:261
- Draft comment:
There's a potential interpolation issue on this line. The error message literal uses '{signer_name}' which will not interpolate the variable as intended. If you meant to include the value ofsigner_namein the formatted string, consider replacing '{signer_name}' with a proper placeholder (e.g.,{}) and passingsigner_nameas an additional argument. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_rjl4XPI1bHs2Z6y8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed e8663ed in 1 minute and 49 seconds. Click for details.
- Reviewed
178lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/sanitize.rs:62
- Draft comment:
Good addition in tests: the assertion now verifies that sanitized messages retain essential context (e.g., 'Failed to connect to') while redacting sensitive parts. This ensures that error messages remain useful without exposing credentials. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_y2JPsZLqBdz79yZD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| } | ||
| #[cfg(not(feature = "unsafe-debug"))] | ||
| { | ||
| KoraError::SigningError(sanitize_message(&_e.to_string())) |
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.
The changes consistently sanitize error messages across conversion implementations. Consider caching the result of _e.to_string() (or similar) in a local variable to avoid repeating the conversion inside sanitize_message(), which can improve clarity and reduce redundant allocations.
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.
Caution
Changes requested ❌
Reviewed 3c7b3de in 1 minute and 27 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/error.rs:80
- Draft comment:
Good refactoring: reusing the already computed sanitized_error_string avoids duplicate sanitization calls in the non-unsafe-debug branch. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_UzmnzSWARzaBXToj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
…s, etc. to prevent sensitive information from being displayed
Important
Sanitizes error messages across the codebase to prevent exposure of sensitive information, with a new
sanitizemodule andunsafe-debugfeature flag.sanitize.rsmodule for redacting sensitive information from error messages.sanitize_error!macro for conditional sanitization based onunsafe-debugfeature flag.cache.rs,config.rs,error.rs,oracle/jupiter.rs,signer/config.rs,signer/keypair_util.rs,usage_limit/usage_store.rs, andusage_limit/usage_tracker.rs.KoraErrorconversions to use sanitized messages.ConfigValidatorto include sanitization.unsafe-debugfeature flag inCargo.tomlto control error message verbosity.rpc_server/auth.rsto use sanitized error messages.sanitize.rsto verify sanitization of URLs and hex strings.This description was created by
for 3c7b3de. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 83.8%
View Detailed Coverage Report