-
Notifications
You must be signed in to change notification settings - Fork 31
fix: use correct signer locator in approval submissions #1543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Previously, approveTransactionInternal and approveSignatureInternal would always use this.signer.locator() when building approval payloads, even when a different signer from additionalSigners was used to sign. This fix ensures the correct signer's locator is used in the approval payload, making the additionalSigners option work as intended. Co-Authored-By: Brendan Weinstein <brendan@basebeta.com>
Co-Authored-By: Brendan Weinstein <brendan@basebeta.com>
🦋 Changeset detectedLatest commit: 5a79365 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 |
Original prompt from Brendan |
🤖 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:
|
albertoelias-crossmint
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain more about the bug? but the new logic makes sense too
|
@albertoelias-crossmint I noticed this issue when trying to implement The issue was that when approving with a delegated signer, the code would:
So the cryptographic signature was valid (signed by the correct delegated signer), but the approval payload sent to the backend claimed it was signed by the wallet's primary signer. The backend uses that signer field to look up which public key to verify against, so the signature verification would fail - making delegated signer approvals broken. |
Description
Fixes a bug where
approveSignatureInternalandapproveTransactionInternalwould always usethis.signer.locator()when building approval payloads, even when a different signer fromadditionalSignerswas used to sign. This made theadditionalSignersoption effectively broken for delegated signers.The fix ensures the correct signer's locator is used in the approval payload - the one that actually performed the signing operation.
Before: Approval submitted with
this.signer.locator()regardless of which signer signedAfter: Approval submitted with the actual signer's locator that was matched and used for signing
Test plan
signers.find()is correctly used in the approval payloadHuman review checklist:
signervariable captured in the map callback is the correct one to use forsigner.locator()additionalSignersapproval flow should be addedPackage updates
@crossmint/wallets-sdk: patch bump - changeset added viapnpm change:addLink to Devin run: https://crossmint.devinenterprise.com/sessions/2265a4dc8e5745b6916e43bd3acff919
Requested by: Brendan Weinstein (@brendanw)