fix(wallets): preserve OTP validation failure reason instead of collapsing into generic error#1886
Conversation
…psing into generic error - Add OtpValidationError class with optional error code from TEE response - verifyOtp and sendMessageWithOtp now throw OtpValidationError instead of generic Error - recover() handles OtpValidationError: preserves local key for retry (like AuthRejectedError) - Export OtpValidationError so consumers can distinguish OTP failures from other errors
Original prompt from Guille
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 144f8fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/signers/non-custodial/ncs-signer.ts:332
The `sendMessageWithOtp` error path already guards against an absent `response.error` field with `response.error || "Failed to initiate OTP process."`, but `verifyOtp` does not apply the same guard. When the TEE returns `{status:"error"}` without an `error` field, `errorMessage` becomes `undefined`, and `OtpValidationError` ends up with the literal string `"undefined"` as its message — which would be a confusing surface to a consumer.
```suggestion
const errorMessage = response?.status === "error" ? (response.error || "Failed to validate encrypted OTP") : "Failed to validate encrypted OTP";
```
Reviews (1): Last reviewed commit: "chore: add changeset for OtpValidationEr..." | Re-trigger Greptile |
|
Reviews (2): Last reviewed commit: "fix: guard against undefined response.er..." | Re-trigger Greptile |
|
Reviews (3): Last reviewed commit: "fix: remove unnecessary parentheses to s..." | Re-trigger Greptile |
Description
During wallet recovery, when
complete-onboardingreturns an OTP validation failure, the error gets collapsed into a genericErrorthat surfaces as "Crossmint signer returned an internal error" in consumer apps. The SDK loses the underlying auth/OTP failure context at two points:verifyOtpcreates a plainError(response.error)— no typed error class, no error code from the TEE responserecover()treats OTP failures as unknown errors (falls to the genericelsebranch), which destructively deletes the device keyThis PR:
OtpValidationError(extendsError) with an optionalcodefield for the TEE error codeverifyOtpandsendMessageWithOtpnow throwOtpValidationErrorinstead of genericError, forwardingresponse.coderecover()handlesOtpValidationErrorlikeAuthRejectedError: preserves the local device key so the user can retry the OTP flow without re-registeringOtpValidationErrorfrom@crossmint/wallets-sdkso consumers caninstanceof-check and surface the real reasonError flow before:
Error flow after:
Test plan
preserves local key and rethrow when addSigner fails with OtpValidationError— verifiesdeleteKeyis NOT called andOtpValidationErrorpropagatesAuthRejectedErrortest still passes (no regression)@crossmint/wallets-sdktest suite passes (337 tests)Package updates
@crossmint/wallets-sdk— patch (changeset added)Link to Devin session: https://crossmint.devinenterprise.com/sessions/7bd8230602444a8a94498384e0aac02d
Requested by: @xmint-guille