Make isSignerApproved throw on errors - #2012
Conversation
🦋 Changeset detectedLatest commit: 7e78d25 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 AI### Issue 1
packages/wallets/src/wallets/services/signer-manager.ts:226-227
**Validate required signer fields**
The new validation rejects every response containing an `error` property before checking whether valid signer data is also present. Validate the fields required by `mapApiSignerToSigner` instead, so error metadata accompanying usable signer data does not make `isSignerApproved()` throw.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Make isSignerApproved throw when the sig..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (signerResponse == null || typeof signerResponse !== "object" || "error" in signerResponse) { | ||
| throw new Error(`Failed to fetch the approval state of signer ${signerLocator}`); |
There was a problem hiding this comment.
The new validation rejects every response containing an error property before checking whether valid signer data is also present. Validate the fields required by mapApiSignerToSigner instead, so error metadata accompanying usable signer data does not make isSignerApproved() throw.
Rule Used: When validating API response structures, avoid che... (source)
Learned From
Paella-Labs/crossbit-main#21001
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/services/signer-manager.ts
Line: 226-227
Comment:
**Validate required signer fields**
The new validation rejects every response containing an `error` property before checking whether valid signer data is also present. Validate the fields required by `mapApiSignerToSigner` instead, so error metadata accompanying usable signer data does not make `isSignerApproved()` throw.
**Rule Used:** When validating API response structures, avoid che... ([source](https://app.greptile.com/crossmint/-/custom-context?memory=11da8f5d-8c7c-4a66-8d65-236276bfb2e5))
**Learned From**
[Paella-Labs/crossbit-main#21001](https://github.com/Paella-Labs/crossbit-main/pull/21001)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
the success DTO for this endpoint is a union of signer shapes with no error field, so a body carrying error metadata next to usable signer data isn't something the API can return. Error bodies only come with non-2xx statuses, and those now throw in getSigner before this check runs. Keeping the "error" in response narrowing also matches how the rest of the package guards responses (getSignerState, send, etc)
There was a problem hiding this comment.
That's a fair point — since getSigner now throws on non-2xx responses before #fetchSigner ever sees the body, an error payload can't coexist with valid signer data by the time this check runs. And the consistency with getSignerState, send, and the rest of the package is a good reason to keep the pattern uniform. I'll withdraw the concern.
Shall I remember this as a rule: "trust "error" in response narrowing as a valid response guard in this package, consistent with getSignerState, send, and other callers, since non-2xx bodies are thrown before reaching these checks"?
771b036 to
7e78d25
Compare
|
Reviews (2): Last reviewed commit: "Make isSignerApproved throw when the sig..." | Re-trigger Greptile |
1 participant
wallet.isSignerApprovedswallowed every error and resolved tofalse, so callers couldn't tell a signer that isn't approved from a request that failed. It now throws on real failures and keeps resolvingfalsefor a signer that isn't registered, matching Swift (#128) and Kotlin (#131).Changes
ApiClient.getSignernow throws anApiClientErrorcarrying the HTTP status on non-ok responses, instead of returning the error body as a regular resultSignerManager.isSignerApprovedmaps a 404 tofalse(the signer isn't registered) and lets every other error bubble up to the callergetSignerStatekeeps its lenient null-state fallback, sinceassembleand the recovery flow rely on it to degrade gracefullygetSignertests for the ok and non-ok paths, andisSignerApprovedtests for the approved, not registered, and failure cases