feat: (PRO-203) Implement TurnkeyError handling and enhance PrivySign…

[dev-jodee]

…er error logging

• Added TurnkeyError enum for comprehensive error handling in TurnkeySigner.
• Updated sign and create_stamp methods to return TurnkeyError variants.
• Enhanced error logging in PrivySigner for API call failures, including status and response details.
• Refactored TurnkeySigner initialization to remove unnecessary error handling, improving clarity.
• Added tests for TurnkeyError display and conversion to KoraError.

---

Important

Introduces TurnkeyError for TurnkeySigner error handling, enhances PrivySigner error logging, and refactors TurnkeySigner initialization.

• Behavior:
  • Added TurnkeyError enum for error handling in TurnkeySigner.
  • Updated sign and create_stamp methods in TurnkeySigner to return TurnkeyError variants.
  • Enhanced error logging in PrivySigner for API call failures, including status and response details.
• Refactoring:
  • Refactored TurnkeySigner initialization to remove unnecessary error handling.
• Testing:
  • Added tests for TurnkeyError display and conversion to KoraError.
• Misc:
  • Updated test coverage to 85.8%.

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

---

📊 Test Coverage

Coverage: 85.8%

View Detailed Coverage Report

[linear]

PRO-203 Turnkey/privy: graceful error handling

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 9237356 in 1 minute and 56 seconds. Click for details.

• Reviewed 435 lines of code in 6 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/error.rs:202

• Draft comment:
  Consider preserving underlying error context (e.g. via source or richer types) in the TurnkeyError-to-KoraError conversion rather than relying solely on to_string().
• 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 comment raises a valid point about error handling best practices - preserving error context can be valuable for debugging. However, looking at the codebase, this matches the established pattern used throughout the file. All other error conversions use to_string(). Changing just this one would be inconsistent. The comment suggests a larger architectural change that would need to be applied consistently. The comment identifies a real software engineering best practice. Error context preservation is valuable. Maybe I'm being too quick to dismiss it just because other code does it the same way. While error context preservation is good practice, suggesting it only here creates inconsistency. This would need to be a larger refactoring effort across all error handling. Delete the comment. While it suggests a valid improvement, it would require a larger architectural change. The current implementation follows the established pattern in the codebase.

2. crates/lib/src/signer/privy/signer.rs:60

• Draft comment:
  Enhanced logging here is useful, but verify that logging the full error response (status and text) does not inadvertently leak sensitive details.
• 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% This is a third-party API (Privy) that we're calling. The error responses are controlled by Privy, not by this code. If Privy includes sensitive data in their error responses, that would be a problem with their API design. This code is just logging whatever error response Privy sends. The comment is speculative and asks for verification rather than pointing out a clear issue. The comment raises a valid security concern - logging of API responses could potentially expose sensitive data. There could be cases where Privy includes sensitive details in errors. While security is important, this is a third-party API and we have to trust their error handling. If they include sensitive data in errors, that's an API design issue. The comment doesn't identify a clear problem to fix. Delete this comment. It's speculative, asks for verification rather than pointing out a clear issue, and relates to the security of a third-party API rather than a problem with this code.

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

• Draft comment:
  The use of unwrap() on try_into() to convert the signature vector into a [u8;64] may panic if the signature length is unexpected. Consider handling this conversion error gracefully.
• Reason this comment was not posted:
  Comment was on unchanged code.

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

• Draft comment:
  In create_stamp, mapping errors (e.g. from hex_to_bytes and SigningKey creation) to generic TurnkeyError variants may lose context. Consider preserving more error details.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

5. crates/lib/src/signer/turnkey/types.rs:100

• Draft comment:
  TurnkeyError’s Display and conversion tests are well covered. Ensure consistency of error messages across different modules.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_BTj0CcnCJPjsuy85

^{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 0fa0ed4 in 1 minute and 12 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:
  Ensure consistency between the reported coverage percentages. The description notes 85.8% but the badge now shows 85.7%.
• 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 references a description that shows 85.8%, but I can't verify this claim without seeing the description. The actual change in the diff is from 85.5% to 85.7%. Without being able to verify the 85.8% number, I can't be confident this comment is correct. Additionally, small coverage fluctuations are common and not necessarily worth commenting on. I might be missing important context from the PR description that would validate this comment. The difference between 85.7% and 85.8% could be meaningful in some contexts. Without clear evidence of the 85.8% number and given that this is a minor difference, it's better to err on the side of removing potentially incorrect or nitpicky comments. Delete this comment as we cannot verify its accuracy and the difference is minor enough that it may not warrant a comment.

Workflow ID: wflow_hBDhoKTu5UNcnSw0

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