feat: unit testing coverage

[dev-jodee]

Important

Enhance unit testing coverage for token, transaction, and validator modules, adding tests for various functions and scenarios.

• Token Module:
  • Add tests for TokenType methods in token.rs to verify correct token program retrieval.
  • Add tests for TokenUtil methods to validate token price calculations and extension validations.
• Transaction Module:
  • Add tests for VersionedMessageExt in versioned_message.rs to ensure base64 encoding correctness.
  • Add tests for VersionedTransactionResolved in versioned_transaction.rs to verify transaction resolution and signing.
• Validator Module:
  • Add tests for AccountType validation in account_validator.rs to ensure correct account type handling.
  • Add tests for validate_account function to check account validation logic.

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

---

📊 Test Coverage

Coverage: 85.8%

View Detailed Coverage Report

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 05d7227 in 2 minutes and 37 seconds. Click for details.

• Reviewed 6286 lines of code in 42 files
• Skipped 0 files when reviewing.
• Skipped posting 9 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/tests/toml_mock.rs:234

• Draft comment:
  Consider providing a more descriptive error message instead of using unwrap when writing the temp file in build_config.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. crates/lib/src/token/spl_token_2022.rs:92

• Draft comment:
  Document that validate_and_adjust_amount returns the unchanged amount when no fee conditions apply.
• 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/token/token.rs:69

• Draft comment:
  Avoid using unwrap when converting token addresses; consider handling conversion errors 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/transaction/versioned_transaction.rs:164

• Draft comment:
  Instead of using unwrap_or_default() when decoding inner instruction data with bs58::decode, consider propagating the error to avoid silently ignoring potential decoding issues.
• 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:92

• Draft comment:
  Ensure that treating an empty allowed_tokens list as a fatal error (instead of a warning) is the intended behavior, as it differs from the behavior in validate_with_result.
• 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/transaction_validator.rs:69

• Draft comment:
  Avoid using unwrap when converting allowed_tokens to Pubkeys; handle any potential conversion errors instead of panicking.
• 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/validator/transaction_validator.rs:176

• Draft comment:
  Include the fee payer's pubkey in the error message within the check_if_allowed closure for better debugging.
• 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.

8. crates/lib/src/validator/transaction_validator.rs:102

• Draft comment:
  This validate_transaction function chains several validation steps; consider modularizing these steps into smaller helper functions to improve readability and ease debugging.
• 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.

9. crates/lib/src/admin/token_util.rs:30

• Draft comment:
  Typo: 'funciton' should be corrected to 'function'.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_EWJwfgFmzm1xna4Q

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

[github-actions]

📊 Rust Coverage Report

Coverage: 4.9%

View detailed report

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 120a1f0 in 1 minute and 27 seconds. Click for details.

• Reviewed 223 lines of code in 2 files
• Skipped 1 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/rpc_server/rpc.rs:217

• Draft comment:
  The test 'test_method_delegation_with_mocks' calls update_config and setup_or_get_test_signer, which modify global state. Consider isolating test state (or using mocks with isolated state) to prevent interference between tests.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. crates/lib/src/rpc_server/rpc.rs:227

• Draft comment:
  Tests here only assert that responses are OK. Consider verifying the actual content of responses to improve test robustness, especially for critical endpoints.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. crates/lib/src/rpc_server/server.rs:103

• Draft comment:
  The macro 'register_method_if_enabled' ignores potential errors from method registration (using 'let _ ='). Consider handling or logging these errors to aid debugging during module setup.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. crates/lib/src/rpc_server/server.rs:255

• Draft comment:
  The test 'test_build_rpc_module_all_methods_enabled' asserts a hard-coded method count (10), which might be brittle if new methods are added. Consider verifying the presence of expected method names rather than only the count.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. crates/lib/src/rpc_server/server.rs:213

• Draft comment:
  Tests for get_value_by_priority use environment variables and manually remove them. Wrapping such setup/cleanup in a guard could ensure cleanup even if a panic occurs, enhancing test isolation.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_ExCjZz0kFHA7DS9i

^{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 b8e8a83 in 2 minutes and 53 seconds. Click for details.

• Reviewed 714 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 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/transaction/versioned_transaction.rs:107

• Draft comment:
  Typo in comment: 'accout' should be 'account'.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. crates/lib/src/transaction/versioned_transaction.rs:164

• Draft comment:
  Consider propagating the decode error from bs58 instead of using unwrap_or_default(), which may mask decoding issues.
• 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/transaction/versioned_transaction.rs:753

• Draft comment:
  Typographical error: The owner string "AddressLookupTab1e1111111111111111111111111" in the GetAccountInfo mock appears to have a typo. Should it be "AddressLookupTable1111111111111111111111111"?
• 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% This is a test file and the string in question is just mock data. The exact value of this string doesn't matter for the test functionality - it's just being used as a placeholder owner address in a mock response. Whether it has a "1" or an "l" makes no difference to the test behavior. The test is about resolving lookup table addresses, not validating program owners. Could the typo indicate a copy-paste error from real code that should be fixed? Could using the correct program ID string make the test more realistic? While using the real program ID would be slightly more realistic, this is clearly marked as test code and the exact value doesn't affect the test's validity. The test is focused on address resolution logic, not program ownership validation. Delete the comment. The suggested change is purely cosmetic in test mock data and doesn't affect test functionality or correctness.

Workflow ID: wflow_EdpioBFDcgHZAbeP

^{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 57c1891 in 3 minutes and 3 seconds. Click for details.

• Reviewed 528 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 4 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/privy/signer.rs:288

• Draft comment:
  The test for sign_solana expects the signature’s to_string() to yield 64 characters (hex), but solana_sdk::signature::Signature typically uses base58 encoding (usually ~88 chars). Confirm the intended encoding.
• 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% Looking at the code, I see that the signature comes from a mock response that we control. The test is just verifying we can parse the mock response correctly. The actual encoding used in production is not relevant to this test. The test is working as intended - it's testing our ability to parse the mock data, not testing real Solana signature behavior. The comment raises a valid point about Solana's typical signature encoding. Maybe there's something important about maintaining consistency with Solana conventions. The test is focused on parsing mock data, not on real Solana behavior. The encoding choice is an implementation detail of our test, not a functional requirement. The comment should be deleted. The test is correctly verifying our mock data parsing, and the actual encoding used is just an implementation detail of our test.

2. crates/lib/src/signer/turnkey/signer.rs:320

• Draft comment:
  The test for sign_solana in TurnkeySigner expects a base58 encoded signature length of 87, which differs from the PrivySigner test. Ensure that the signature encoding is intentionally different or standardize the format.
• 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% 1. The comment is speculative - it assumes there might be an inconsistency without evidence. 2. The base58 encoding length of 87 appears to be a valid expectation for Solana signatures. 3. The comment asks for verification without providing clear evidence of an issue. 4. Cross-file comparisons are out of scope per review rules. I might be overlooking a real inconsistency in signature formats that could cause issues. The length difference could indicate a real problem. While signature format inconsistency could be an issue, this comment is speculative and relies on cross-file comparison which is out of scope. If there was a real issue, it would manifest as test failures. Delete the comment as it is speculative, asks for verification without clear evidence of an issue, and relies on cross-file comparison which is out of scope for this review.

3. crates/lib/src/signer/turnkey/signer.rs:120

• Draft comment:
  Avoid using unwrap() on bytes_to_hex; prefer propagating the error with '?' to prevent potential 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.

4. crates/lib/src/signer/vault/vault_signer.rs:54

• Draft comment:
  The new() function maps over 'client' with map_err even though 'client' is not a Result. This appears to be a leftover and should be removed.
• 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_AZB9bKsuBGsEwVnd

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

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed bc188e0 in 7 minutes and 37 seconds. Click for details.

• Reviewed 147 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/workflows/rust.yml:55

• Draft comment:
  Multiple invocations of 'cargo llvm-cov test' (unit tests and integration tests) may risk not aggregating coverage data as expected. Verify that the coverage data from successive commands is merged correctly in the final report.
• 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 verify that the coverage data is merged correctly, which falls under the rule of not asking the author to ensure behavior is intended or to double-check things. It does not provide a specific suggestion or point out a specific issue with the code.

2. .github/workflows/rust.yml:190

• Draft comment:
  The regex used to remove the existing coverage section in the PR body relies on specific newline formatting. Consider making it more robust in case the section formatting changes.
• 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 regex is actually quite robust already - it handles multiple newline formats (\s\S), accounts for both section boundaries and end of text, and uses non-greedy matching. The current implementation seems well thought out and handles various edge cases. The comment is speculative about potential future issues rather than pointing out a concrete problem. I could be underestimating the variety of PR description formats that might be encountered. There could be edge cases I haven't considered. While edge cases are possible, the current regex is already quite flexible and there's no evidence of actual problems. The comment doesn't provide specific scenarios where it would fail. The comment should be deleted as it's speculative and doesn't point to a concrete issue with the implementation. The current regex appears sufficiently robust.

Workflow ID: wflow_kl9KofssRjrHbJUM

^{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 425c9a2 in 3 minutes and 20 seconds. Click for details.

• Reviewed 20 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/workflows/rust.yml:185

• Draft comment:
  Ensure 'context.runId' is defined for the report URL; verify this change works in all environments.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. .github/workflows/rust.yml:191

• Draft comment:
  The new regex with literal newlines is clearer; double-check it matches the PR body format as expected.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_JrcNKhuwDZZR3YJz

^{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 d29b175 in 1 minute and 59 seconds. Click for details.

• Reviewed 719 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/validator/account_validator.rs:92

• Draft comment:
  The error message for executable flag mismatch is misleading when the expected value is false. When an account is executable but should not be, the error still says "is not executable". Consider adjusting the message to reflect the expected state.
• 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.

2. crates/lib/src/validator/transaction_validator.rs:428

• Draft comment:
  The test 'test_transfer_amount_limits' only exercises transfers where the fee payer isn’t the sender. To fully validate the outflow logic (which uses the fee payer’s transfers), consider adding a test case where the fee payer is the sender and the transfer amount exceeds the max allowed lamports.
• 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_hRtHOlYFJS1rsxxO

^{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 bb3f5c3 in 3 minutes and 33 seconds. Click for details.

• Reviewed 1040 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 4 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/validator/account_validator.rs:92

• Draft comment:
  The error message in the executability check is hard-coded to refer to a Program ("cannot be a Program") even when validating Mint or TokenAccount (expected non‑executable). Consider adjusting the message to reflect the expected executable flag (e.g. 'expected non‑executable').
• 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.

2. crates/lib/src/validator/account_validator.rs:30

• Draft comment:
  Consider adding inline comments or using more specific error variants instead of always wrapping failures as 'InternalServerError'. This can improve debugging and error handling clarity, especially when validating different account types.
• 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/token/spl_token_2022.rs:842

• Draft comment:
  The test function name is still test_token2022_account_extension_methods which suggests it should be using a builder with "2022" in its name. The change from Token2022AccountMockBuilder to TokenAccountMockBuilder might be an unintentional inconsistency. Please verify if this is the intended behavior.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. crates/lib/src/token/spl_token_2022.rs:851

• Draft comment:
  There might be a typographical error in the method name: currently it's written as build_as_custom_token2022_token_account. Please double-check if it should be build_as_custom_token_2022_token_account (with an underscore between "token" and "2022") for consistency with naming conventions.
• 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% This is a very minor stylistic suggestion about a test helper method name. The current name is used consistently throughout the test file. The suggestion is purely about adding an underscore between "token" and "2022", but this doesn't affect functionality. The current name follows the pattern used in the actual SPL Token 2022 codebase where "token2022" is commonly used without an underscore. I could be wrong about the SPL Token 2022 codebase conventions. Maybe there is an established style guide that mandates underscores between "token" and "2022". Even if there was such a style guide, this is a minor naming issue in test code that doesn't affect functionality. The current name is clear and used consistently. This comment should be deleted as it's a minor stylistic suggestion about test code naming that doesn't affect functionality or clarity.

Workflow ID: wflow_az8S8SG9rjT5OjoT

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

[amilz]

Looks great. Left a couple of minor suggestions, also not a huge deal, but I if you want the unit tests to be deterministic I think you'd want to replace the use of Pubkey::new_unique() and Keypair::new() with hard-coded values. IMO fine as-is and good to merge.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 5a528f1 in 2 minutes and 0 seconds. Click for details.

• Reviewed 204 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 6 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/metrics/balance.rs:261

• Draft comment:
  Test now asserts that BalanceTracker::init() returns Ok. Verify if re-initialization should error (as previously allowed both Ok/Err) and consider adding a separate test for multiple initializations.
• 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.

2. crates/lib/src/rpc_server/method/get_config.rs:56

• Draft comment:
  The test 'test_get_config_success' now thoroughly asserts default configuration values. Ensure that any future changes to default config values are reflected here.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. crates/lib/src/rpc_server/method/get_supported_tokens.rs:29

• Draft comment:
  Using the #[serial] attribute on tests is a good practice here to ensure isolation when updating global config state.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. crates/lib/src/signer/memory_signer/solana_signer.rs:141

• Draft comment:
  Hardcoding the expected base58 public key in the test (test_from_private_key_string_base58) enforces determinism. Just ensure that future changes to key generation don't unexpectedly break this test.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

5. crates/lib/src/signer/privy/signer.rs:36

• Draft comment:
  Renaming get_auth_header to get_privy_auth_header improves clarity by specifying its context. The consistent updates across the file and tests are appreciated.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

6. crates/lib/src/token/token.rs:161

• Draft comment:
  In validate_token2022_extensions_for_payment, downcast_ref() is used with unwrap() for both mint and token accounts. Consider handling downcast failures gracefully (e.g. by returning an appropriate KoraError) instead of panicking.
• 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_NTXz4sPZ84l1AMy0

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