Skip to content
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

chore: distributed session keys #114

Merged
merged 3 commits into from
Nov 13, 2024
Merged

chore: distributed session keys #114

merged 3 commits into from
Nov 13, 2024

Conversation

joepegler
Copy link
Collaborator

@joepegler joepegler commented Nov 12, 2024

PR-Codex overview

This PR focuses on updating the @biconomy/sdk package to version 0.0.10, introducing distributed session keys and enhancing smart session functionalities. It includes various improvements in error handling, session management, and code structure.

Detailed summary

  • Updated version in package.json to 0.0.10.
  • Added KEY_GEN_DATA_NOT_FOUND and SIGNATURE_NOT_FOUND error messages in Constants.ts.
  • Changed permissionId to permissionIds in several files to support multiple permissions.
  • Updated createNexusSessionClient to directly return createNexusClient.
  • Refactored usePermission to use calls instead of actions.
  • Introduced useDistributedPermission for executing multiple actions in a session.
  • Added tests for creating and using smart sessions with DAN in toSmartSessionsValidator.dan.dx.test.ts.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

github-actions bot commented Nov 12, 2024

size-limit report 📦

Path Size
core (esm) 35.71 KB (+0.43% 🔺)
core (cjs) 51.25 KB (+0.13% 🔺)
bundler (tree-shaking) 547 B (0%)
paymaster (tree-shaking) 123 B (0%)

@joepegler joepegler marked this pull request as draft November 13, 2024 10:33
@@ -123,14 +123,14 @@ export const toSmartSessionsValidator = (
getStubSignature: async () =>
encodeSmartSessionSignature({
mode,
permissionId,
permissionId: permissionIds[permissionIdIndex],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when getting a sig we only need one permissionId at a time. which represents a session

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check if there is case where it's making use of multiple already-enabled sessions in a single userop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can confirm it then only make an array? in this PR can you revert it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this pr the functionality hasn't changed. I've only paved the way for accommodating multiple permissionIds later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better that the type changes to this sooner rather than later. Right now if you create multiple permissionIds then only the first gets added to the module.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd prefer I can revert, but I don't see any harm in doing this now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm the module keeps active permission id single because for signature only one can be used at a time.
reverting is better only because we first check feasibility, ask @Fil then make changes.

@@ -123,14 +123,14 @@ export const toSmartSessionsValidator = (
getStubSignature: async () =>
encodeSmartSessionSignature({
mode,
permissionId,
permissionId: permissionIds[permissionIdIndex],
enableSessionData,
signature: DUMMY_ECDSA_SIG
}),
signUserOpHash: async (userOpHash: Hex) =>
encodeSmartSessionSignature({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add on to above comments, in module-sdk also encodeSmartSessionSignature uses single permissionId

@@ -102,7 +102,7 @@ export const parseModule = <
chain extends Chain | undefined
>(
client: Client<Transport, chain, TModularSmartAccount>
) => {
): AnyData => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does parseModule do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a helper meant only for internal use. Not to be used externally really. It's a helper that pulls out the active module instance from the nexusClient.

Copy link
Contributor

@livingrockrises livingrockrises left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review i

@joepegler
Copy link
Collaborator Author

Good points re permissionIds. I've created a ticket for your suggestion: #116

@joepegler joepegler merged commit a1778c2 into develop Nov 13, 2024
5 checks passed
@joepegler joepegler deleted the feat/ss_dan branch November 13, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants