fix: throw DeviceSignerRotatedError when device key rotates between tx creation and approval#1957
Conversation
…tes between tx creation and approval When the device signer key is lost (e.g. unreliable Android Keystore on budget devices), the SDK's DeviceRecoveryService generates a new key. If a transaction was already created for the previous device key, the approve() flow would throw a generic InvalidSignerError with no guidance. Now throws DeviceSignerRotatedError (code: wallet:signer-rotated) with both the expected and actual signer locators, guiding the developer to re-create the transaction so it uses the current device signer. Applies to both approveTransactionInternal and approveSignatureInternal. Refs: WAL-10931, Pylon #8810 Co-Authored-By: Emilio <emilio@paella.dev>
Original prompt from Emilio
|
🤖 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: a35fe50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/wallets/src/utils/errors.ts:95-100
The error message refers to "this transaction" even when this error is thrown from the signature-approval path (`approveSignatureInternal`). A developer approving a `signatureId` will see a confusing message telling them to "re-create the transaction" when they should re-create the signature request instead.
```suggestion
super(
`The device signer was rotated since this request was created. ` +
`The pending approval requires signer '${expectedLocator}', but the current device signer is '${actualLocator}'. ` +
`Please re-create the transaction or signature request so it uses the current device signer.`,
WalletErrorCode.SIGNER_ROTATED
);
```
### Issue 2 of 2
packages/wallets/src/wallets/wallet.ts:1227-1234
Duplicated device-signer rotation detection block — the same 8-line guard appears identically in both `approveSignatureInternal` (line ~1168) and `approveTransactionInternal` (here). Extracting it into a small helper (e.g. `throwIfDeviceSignerRotated(signers, pendingApproval)`) would keep the two call-sites in sync and make future changes (different condition, different fields on the error) a single-place edit.
Reviews (1): Last reviewed commit: "fix(wallets-sdk): throw DeviceSignerRota..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| super( | ||
| `The device signer was rotated since this transaction was created. ` + | ||
| `The transaction requires signer '${expectedLocator}', but the current device signer is '${actualLocator}'. ` + | ||
| `Please re-create the transaction so it uses the current device signer.`, | ||
| WalletErrorCode.SIGNER_ROTATED | ||
| ); |
There was a problem hiding this comment.
approveSignatureInternal). A developer approving a signatureId will see a confusing message telling them to "re-create the transaction" when they should re-create the signature request instead.
| super( | |
| `The device signer was rotated since this transaction was created. ` + | |
| `The transaction requires signer '${expectedLocator}', but the current device signer is '${actualLocator}'. ` + | |
| `Please re-create the transaction so it uses the current device signer.`, | |
| WalletErrorCode.SIGNER_ROTATED | |
| ); | |
| super( | |
| `The device signer was rotated since this request was created. ` + | |
| `The pending approval requires signer '${expectedLocator}', but the current device signer is '${actualLocator}'. ` + | |
| `Please re-create the transaction or signature request so it uses the current device signer.`, | |
| WalletErrorCode.SIGNER_ROTATED | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/errors.ts
Line: 95-100
Comment:
The error message refers to "this transaction" even when this error is thrown from the signature-approval path (`approveSignatureInternal`). A developer approving a `signatureId` will see a confusing message telling them to "re-create the transaction" when they should re-create the signature request instead.
```suggestion
super(
`The device signer was rotated since this request was created. ` +
`The pending approval requires signer '${expectedLocator}', but the current device signer is '${actualLocator}'. ` +
`Please re-create the transaction or signature request so it uses the current device signer.`,
WalletErrorCode.SIGNER_ROTATED
);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good catch from the cold-blooded code crawler 🐍 — fixed in a35fe50. The message now says "request" instead of "transaction" so it makes sense from both approval paths.
| const deviceSigner = signers.find((s) => s.type === "device"); | ||
| if ( | ||
| pendingApproval.signer.locator.startsWith("device:") && | ||
| deviceSigner != null && | ||
| deviceSigner.locator() !== pendingApproval.signer.locator | ||
| ) { | ||
| throw new DeviceSignerRotatedError(pendingApproval.signer.locator, deviceSigner.locator()); | ||
| } |
There was a problem hiding this comment.
approveSignatureInternal (line ~1168) and approveTransactionInternal (here). Extracting it into a small helper (e.g. throwIfDeviceSignerRotated(signers, pendingApproval)) would keep the two call-sites in sync and make future changes (different condition, different fields on the error) a single-place edit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1227-1234
Comment:
Duplicated device-signer rotation detection block — the same 8-line guard appears identically in both `approveSignatureInternal` (line ~1168) and `approveTransactionInternal` (here). Extracting it into a small helper (e.g. `throwIfDeviceSignerRotated(signers, pendingApproval)`) would keep the two call-sites in sync and make future changes (different condition, different fields on the error) a single-place edit.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fair point from the slithering style inspector 🦎, but this is a 6-line guard in two places — extracting a helper here would be over-engineering for the scope of this fix. If this pattern grows or gains more callers, happy to refactor then.
The error is thrown from both transaction and signature approval paths, so the message now says 'request' instead of 'transaction'. Co-Authored-By: Emilio <emilio@paella.dev>
|
Reviews (2): Last reviewed commit: "fix: use generic wording in DeviceSigner..." | Re-trigger Greptile |
Description
When a mobile device loses its device signer key (e.g. unreliable Android Keystore on budget devices like HOTWAV Cyber 13), the SDK's
DeviceRecoveryServicegenerates a new device key. If a transaction/signature was already created for the previous device key,approve()throws a genericInvalidSignerErrorwith no guidance on what happened or how to recover.This PR introduces a specific
DeviceSignerRotatedError(code:wallet:signer-rotated) that is thrown instead when the SDK detects that:device:*signerThe new error includes both the expected (old) and actual (new) signer locators and a message guiding the developer to re-create the transaction.
Changes:
@crossmint/common-sdk-base: AddSIGNER_ROTATEDtoWalletErrorCode@crossmint/wallets-sdk:DeviceSignerRotatedErrorclass withexpectedLocatorandactualLocatorfieldsapproveTransactionInternal(): detect device signer mismatch → throwDeviceSignerRotatedErrorapproveSignatureInternal(): same detection logicDeviceSignerRotatedErrorfrom package indexRefs: WAL-10931, Pylon #8810
Test plan
wallet.test.ts:DeviceSignerRotatedErrorDeviceSignerRotatedErrorInvalidSignerError(no regression)pnpm lint:fixPackage updates
@crossmint/common-sdk-base: patch (newSIGNER_ROTATEDerror code)@crossmint/wallets-sdk: patch (newDeviceSignerRotatedError+ detection logic).changeset/device-signer-rotated-error.mdLink to Devin session: https://crossmint.devinenterprise.com/sessions/be8e8d0513e146219fd9f723d83641eb
Requested by: @0xEmilio