Fix wallet approval failures from unbounded signers#1979
Open
tomas-martins-crossmint wants to merge 10 commits into
Open
Fix wallet approval failures from unbounded signers#1979tomas-martins-crossmint wants to merge 10 commits into
tomas-martins-crossmint wants to merge 10 commits into
Conversation
…oncurrency, simplify signer resolution signature
…DeviceRecoveryService Wallet#resolveMissingDeviceSigner and DeviceRecoveryService#findLocalDeviceSigner each independently read deviceSignerKeyStorage and called tryAssembleLocalDeviceSigner with their own try/catch. Both now delegate to a single tryResolveLocallyAvailableSigner method, so device is the only signer type this decision has to be reimplemented for if another gains it later.
…reInternal/approveTransactionInternal Both methods ran the same mapWithConcurrency + resolveApprovalSigner wrapper around a different sign call. Extracted collectApprovals so each call site only supplies the differing signMessage/signTransaction logic.
🦋 Changeset detectedLatest commit: 29945cf 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 |
…allet-approval-failures-from-unbounded-signers # Conflicts: # packages/wallets/src/utils/constants.ts # packages/wallets/src/wallets/wallet.ts
tomas-martins-crossmint
marked this pull request as ready for review
July 16, 2026 22:15
Contributor
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
.changeset/fix-wallet-approval-unbounded-signers.md:1-3
The changeset declares `minor` for `@crossmint/wallets-sdk`, but all changes in this PR are bug fixes with no new public API surface — the correct semver bump is `patch`.
```suggestion
---
"@crossmint/wallets-sdk": patch
---
```
### Issue 2 of 2
packages/wallets/src/wallets/wallet.ts:1162
`collectApprovals` uses the `private` keyword while the two new helpers directly above it (`#resolveMissingDeviceSigner`, `#resolveApprovalSigner`) use `#`-syntax private fields. The rest of the class consistently uses `#` for private members. Note: applying this suggestion also requires updating the `this.collectApprovals(...)` call sites in `approveSignatureInternal` and `approveTransactionInternal` to `this.#collectApprovals(...)`.
```suggestion
async #collectApprovals<P extends { signer: { locator: string }; message: string }>(
```
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ith the rest of the class
Contributor
|
Reviews (2): Last reviewed commit: "fix(wallets): use # private syntax for c..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wallet.approve()was throwingSigner not found in pending approvalsfor signers that were valid and locally available, just not present in the wallet's currently-assembled signers list. This hit wallets with large signer counts, wherewallet.signers()was silently truncating under backend rate limits, and separately when device-signer recovery minted a new signer mid-approval after the transaction's pending-approvals snapshot had already been fetchedChanges
wallet.approve()now falls back totryAssembleLocalDeviceSignerbefore rejecting a signer locator it doesn't recognize, instead of throwing immediatelytryAssembleLocalDeviceSigner(signers/index.ts) is a shared helper that checksdeviceSignerKeyStorage.hasKey()and assembles a signer if found; both the new approval fallback and the existingDeviceRecoveryServicerecovery path use it now, they had near-identical logic inline before thismapWithConcurrency, a small worker-pool helper, replaces unboundedPromise.allinwallet.signers()and in approval signer resolution, capped atRATE_LIMIT_BATCH_SIZE; unbounded parallel requests were what tripped the rate limit and caused failed lookups to be silently droppedsdk-device-signer) to1.1.0, which includes the fix for key duplication and alias mapping loss on Android Keystore storage