fix(spend-permission): return the wallet-substituted account in requestSpendPermission#365
Open
Nexory wants to merge 1 commit into
Open
Conversation
…stSpendPermission
When capabilities are provided, requestSpendPermission sends
mutableData: { fields: ['message.account'] }, allowing the wallet to
substitute account with a smart-wallet address. permissionHash is already
derived from the wallet-returned (post-substitution) message, but the
returned permission object used the original pre-substitution
typedData.message, so result.permission.account and result.permissionHash
described different permissions.
Hoist the returned message into permissionMessage, set from
signResult.signedData.message in the wallet_sign branch and from
typedData.message in the eth_signTypedData_v4 branch, so permission and
permissionHash stay consistent. Adds a test where the wallet substitutes
account with a different address; it fails before this change and passes
after.
Collaborator
🟡 Heimdall Review Status
|
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
2 participants
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.
Summary
requestSpendPermissionsendsmutableData: { fields: ['message.account'] }whencapabilitiesare provided, so the wallet may substituteaccountwith a smart-wallet address.permissionHashis already derived from the wallet-returned (post-substitution) message, but the returnedpermissionobject used the original pre-substitutiontypedData.message. As a resultresult.permission.accountandresult.permissionHashdescribed different permissions, and any consumer readingresult.permission.account(display, storage,prepareSpendCallData,fetchPermissions) saw the wrong account.This hoists the returned message into a
permissionMessagevariable, set fromsignResult.signedData.messagein thewallet_signbranch and fromtypedData.messagein theeth_signTypedData_v4branch, sopermissionandpermissionHashstay consistent. Theeth_signTypedData_v4path is unchanged in behavior.Fixes #324.
How did you test your changes?
Added a unit test in
requestSpendPermission.test.tswhere the wallet returnssignedData.message.accountset to a different address than the request; it assertsresult.permissionmatches the post-substitution message. The test fails on the current code (returns the original account) and passes with this change.requestSpendPermission.test.ts: 17 passed (the new test fails without the fix)spend-permissionsuite: 9 files, 137 tests passingtsc --noEmit: no new type errors from this change