-
Notifications
You must be signed in to change notification settings - Fork 619
[SDK] feat: Add session key update handling and expose utility #6217
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "thirdweb": patch | ||
| --- | ||
|
|
||
| Handle updating session keys with new params and expose `shouldUpdateSessionKey` from `extensions/erc4337` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,10 @@ | ||
| import { ZERO_ADDRESS } from "../../../constants/addresses.js"; | ||
| import type { ThirdwebContract } from "../../../contract/contract.js"; | ||
| import type { BaseTransactionOptions } from "../../../transaction/types.js"; | ||
| import { isContractDeployed } from "../../../utils/bytecode/is-contract-deployed.js"; | ||
| import { toWei } from "../../../utils/units.js"; | ||
| import type { Account } from "../../../wallets/interfaces/wallet.js"; | ||
| import { getPermissionsForSigner } from "../__generated__/IAccountPermissions/read/getPermissionsForSigner.js"; | ||
| import { | ||
| isSetPermissionsForSignerSupported, | ||
| setPermissionsForSigner, | ||
|
|
@@ -86,3 +91,78 @@ | |
| export function isAddSessionKeySupported(availableSelectors: string[]) { | ||
| return isSetPermissionsForSignerSupported(availableSelectors); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the session key should be updated. | ||
| * @param currentPermissions - The current permissions of the session key. | ||
| * @param newPermissions - The new permissions to set for the session key. | ||
| * @returns A boolean indicating if the session key should be updated. | ||
| */ | ||
| export async function shouldUpdateSessionKey(args: { | ||
| accountContract: ThirdwebContract; | ||
| sessionKeyAddress: string; | ||
| newPermissions: AccountPermissions; | ||
| }): Promise<boolean> { | ||
| const { accountContract, sessionKeyAddress, newPermissions } = args; | ||
|
|
||
| // check if account is deployed | ||
| const accountDeployed = await isContractDeployed(accountContract); | ||
| if (!accountDeployed) { | ||
| return true; | ||
| } | ||
|
|
||
| // get current permissions | ||
| const currentPermissions = await getPermissionsForSigner({ | ||
| contract: accountContract, | ||
| signer: sessionKeyAddress, | ||
| }); | ||
| // check end time validity | ||
| if ( | ||
| currentPermissions.endTimestamp && | ||
| currentPermissions.endTimestamp < Math.floor(new Date().getTime() / 1000) | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| // check targets | ||
| if ( | ||
| !areSessionKeyContractTargetsEqual( | ||
| currentPermissions.approvedTargets, | ||
| newPermissions.approvedTargets, | ||
| ) | ||
|
Comment on lines
+128
to
+132
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of checking for equality, it could check for subests to minimize the need for creating sessions (i.e.,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! Looks great |
||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| // check if the new native token limit is greater than the current one | ||
| if ( | ||
| toWei(newPermissions.nativeTokenLimitPerTransaction?.toString() ?? "0") > | ||
| currentPermissions.nativeTokenLimitPerTransaction | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| function areSessionKeyContractTargetsEqual( | ||
| currentTargets: readonly string[], | ||
| newTargets: string[] | "*", | ||
| ): boolean { | ||
| // Handle the case where approvedTargets is "*" | ||
| if ( | ||
| newTargets === "*" && | ||
| currentTargets.length === 1 && | ||
| currentTargets[0] === ZERO_ADDRESS | ||
| ) { | ||
| return true; | ||
| } | ||
| if (newTargets !== "*") { | ||
| return newTargets | ||
| .map((target) => target.toLowerCase()) | ||
| .every((target) => | ||
| currentTargets.map((t) => t.toLowerCase()).includes(target), | ||
| ); | ||
| } | ||
| return false; | ||
| } | ||
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.
We also check on
startDateandisAdminvalues in our current version of this logic, but not sure if that's important here.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.
start date is a bit weird, decided to leave it alone for this. And yeah admin should be separate i think?