chore:(PRO-259) Add additional Validation Checks

[amilz]

All Tokens Check

Adds validation warning when allowed_spl_paid_tokens is configured with "All" instead of an explicit allowlist

⚠️  Using 'All' for allowed_spl_paid_tokens - this accepts ANY SPL token for payment.
Consider using an explicit allowlist to reduce volatility risk and protect against
potentially malicious or worthless tokens being used for fees.".to_string()

Also minor tweak to crates/lib/src/signer/utils.rs to pass pre-commit checks

Signer.toml Check

Adds an optional check on signers.toml to ensure signers are valid

Other

Nit: updating TS test timeout timing to ensure more reliable testing

---

Important

Adds validation checks for SPL tokens and signer configurations, with updates to configuration validation logic and test improvements.

• Validation Checks:
  • Adds warning for allowed_spl_paid_tokens set to "All" in config_validator.rs.
  • Adds optional validation for signers.toml in config_validator.rs and signer_validator.rs.
• Code Changes:
  • Updates ConfigCommands in main.rs to include signers_config option.
  • Implements validate_with_result_and_signers() in config_validator.rs.
  • Refactors SignerPoolConfig validation methods in config.rs.
• Testing:
  • Increases timeout for TS tests in integration.test.ts to 90000ms.
  • Adds tests for new validation logic in config_validator.rs and signer_validator.rs.

^{This description was created by for d76922c. You can customize this summary. It will automatically update as commits are pushed.}

---

📊 Test Coverage

Coverage: 86.3%

View Detailed Coverage Report

[linear]

PRO-259 Warning if all spl tokens are allowed

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to c1450ab in 58 seconds. Click for details.

• Reviewed 59 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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/signer/utils.rs:6

• Draft comment:
  Using is_multiple_of(2) is more idiomatic, but ensure your toolchain supports it as it requires a recent Rust version.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. crates/lib/src/validator/config_validator.rs:122

• Draft comment:
  The new warning for using 'All' for allowed_spl_paid_tokens is useful. Consider reformatting the multi-line string to avoid unintended extra whitespace in the output.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_paQwwFngagHD0s3o

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 06c0dc6 in 31 seconds. Click for details.

• Reviewed 7 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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. .github/badges/coverage.json:1

• Draft comment:
  Coverage value in JSON (86.2%) does not match PR description (86.3%). Confirm the intended value.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. .github/badges/coverage.json:1

• Draft comment:
  Consider adding a trailing newline for better POSIX compliance.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_CN6CNPGkJWL859IJ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4ac366c in 1 minute and 53 seconds. Click for details.

• Reviewed 440 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft 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/signer/config.rs:133

• Draft comment:
  In validate_strategy_weights, consider handling the case when a signer's weight is None for a Weighted strategy. Currently, only a weight of 0 is flagged, but missing weights may lead to unintended behavior.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment raises a valid point about None weights not being validated. However, looking at the struct definition, weight is documented as "optional, defaults to 1". This implies that None is a valid state and is handled elsewhere in the code by defaulting to 1. Therefore, there's no need to validate None weights as they have a sensible default. I could be wrong about where the default weight of 1 is actually implemented. Without seeing the weight selection code, I can't be 100% sure that None weights are properly handled. Even with that uncertainty, the struct's documentation clearly states the intended behavior. If there was a mismatch between documentation and implementation, that would be a different issue than what this validation function should handle. The comment should be deleted because None weights are documented to have a default value of 1, making them valid by design. The validation function correctly focuses only on explicitly set weights that are zero.

2. crates/lib/src/validator/config_validator.rs:273

• Draft comment:
  Instead of using println to notify that signers configuration is not validated, consider using a logging framework. This approach would better control log levels and avoid potential exposure of sensitive data.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While using a logging framework is generally good practice, this specific println is just an informational message that doesn't contain sensitive data. It's part of a validation utility that already uses println for all its other output (see lines 278-297). The comment suggests a larger architectural change that would need to be applied consistently across the whole file, not just this one line. The comment has a valid point about logging best practices, but it's suggesting an inconsistent change since the rest of the file uses println. Also, there's no evidence of sensitive data exposure risk here. While using a logging framework would be nice, changing just this one line would create inconsistency. Any logging improvements should be done as a separate, comprehensive refactor. The comment should be deleted. While technically correct, it suggests an inconsistent change and the current code follows the established pattern in the file.

3. crates/lib/src/validator/config_validator.rs:247

• Draft comment:
  The unwrap() call when parsing the payment address assumes earlier validation guarantees its success. For improved robustness, consider handling potential parse failures gracefully.
• 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:135

• Draft comment:
  The multi‐line warning message for using 'All' for allowed_spl_paid_tokens may include unintended whitespace. Consider reformatting it (e.g., using a single-line string or careful newline handling) to ensure consistent output.
• 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/signer_validator.rs:40

• Draft comment:
  For converting the selection strategy enum to a string in warnings, consider implementing the Display trait for SelectionStrategy. This would avoid hardcoded string conversions and improve maintainability.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The suggestion is reasonable - implementing Display would be more idiomatic Rust and could reduce duplication if the enum needs to be converted to strings elsewhere. However, this is only used in one place for warning messages, and the current match statement is clear and maintainable. The benefit seems minor compared to the effort. The suggestion could improve code quality and follow Rust best practices. The Display trait is the standard way to convert types to strings in Rust. While technically correct, the current code is simple and the change would be primarily cosmetic with little practical benefit since the enum-to-string conversion is only used once in warning messages. The comment should be removed as it suggests a change that would provide minimal practical benefit for the effort required.

Workflow ID: wflow_wKrUfzTC5GirRNzL

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 7f76b49 in 40 seconds. Click for details.

• Reviewed 7 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft 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. .github/badges/coverage.json:1

• Draft comment:
  Badge update: changed coverage from 86.3% to 85.7%. Confirm that this reflects the current test coverage and update docs if needed.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm the test coverage change and update documentation if needed. This falls under asking the author to confirm their intention and ensure the behavior is intended, which is against the rules.

Workflow ID: wflow_F9VSdzYmdIyfFVed

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed c73b609 in 1 minute and 1 seconds. Click for details.

• Reviewed 227 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft 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/signer/config.rs:117

• Draft comment:
  Exposing helper methods (validate_signer_not_empty, validate_signer_names, validate_strategy_weights, validate_individual_signer_config) as public improves reusability. Consider adding API docs if these are intended for external use.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. crates/lib/src/signer/config.rs:127

• Draft comment:
  The duplicate name check returns on the first duplicate. It might be more user‐friendly to accumulate all duplicate name errors for comprehensive feedback.
• 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/signer_validator.rs:59

• Draft comment:
  Consider implementing the Display trait for SelectionStrategy to avoid hardcoding the string conversion in warning messages (avoid the 'unknown' fallback).
• 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/signer/config.rs:139

• Draft comment:
  In weighted strategy, a missing weight triggers a warning while an explicit 0 value errors out. Clarify the intended behavior (e.g. default to 1) in the documentation for weight handling.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. crates/lib/src/signer/config.rs:95

• Draft comment:
  The commit message mentions additional validation for allowed_spl_paid_tokens ('All' warning), but no related changes are visible in this diff. Ensure that these validation checks are included as intended.
• 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.

Workflow ID: wflow_J7PLqnI7vBDVyaGi

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 2eeba02 in 31 seconds. Click for details.

• Reviewed 8 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft 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. .github/badges/coverage.json:1

• Draft comment:
  Coverage value updated to 86.3% - ensure the file ends with a trailing newline to follow best practices.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_4K33RWuA578Ppthk

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[github-actions]

📊 TypeScript Coverage Report

Coverage: 82.0%

View detailed report

Coverage artifacts have been uploaded to this workflow run.
View Artifacts

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed d76922c in 40 seconds. Click for details.

• Reviewed 48 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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/signer/config.rs:41

• Draft comment:
  Nice use of the Display trait for SelectionStrategy. Consider adding brief documentation for clarity.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. crates/lib/src/validator/signer_validator.rs:59

• Draft comment:
  Refactored warning message to use SelectionStrategy's Display implementation, which improves maintainability.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_qH0a0Vdh3IG3FiOJ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[dev-jodee]

ahha yeah good idea