-
Notifications
You must be signed in to change notification settings - Fork 68
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: sessions dx #486
chore: sessions dx #486
Conversation
size-limit report 📦
|
const { sessionKeyAddress, sessionStorageClient } = | ||
await createAndStoreNewSessionKey(smartAccount, chain); | ||
|
||
const { wait, session } = await createSession( |
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.
Is there a situation where passed smartAccount and sessionKeyAddress arguments would be different then the ones calculate above ? If not, we could improve this even more by removing the need to pass smartAccount and sessionKeyAddress in the "createSession" method. You could bind the method to the smartAccount like await smartAccount.createSession([{contractAddress: "0x...", functionSelector: "safeMint(address)", rules: ....}], sessionStorageClient, {paymasterServiceData ...})
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.
This was for a technical reason, at some point soon I want to move sessions into it's own repo because it imports merkletree. If I add the 'createSession' method to the smartAccount then the session code would need to live in the accounts package permanently, like paymaster and bundler etc. Keeping this setup will facilitate the future removal of the relatively large sessions package from our accounts package.
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.
this is specific to creating a session struct using ABI svm?
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.
and it will also send tx on-chain to set session using current active validation module(default ecdsa) ?
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.
Yes this is for ABI svm. Just using a more accessible term. Yes it encorporates the tx to change the validation module, if required
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.
the userop that gets built for this only supports single tx? setting one session struct on-chain on SKM.
or calldata batch is enabled where you set multiple sessions and also get session ids back in return along with tx hash.
using via multi-chain validation module goes one step further where you can send multiple userops (each op can have multiple session enabling as part of calldata batch on particular chain) that enables several same session on multiple chains
Co-authored-by: Vasile Gabriel Marian <56271768+VGabriel45@users.noreply.github.com>
Co-authored-by: Vasile Gabriel Marian <56271768+VGabriel45@users.noreply.github.com>
Co-authored-by: Vasile Gabriel Marian <56271768+VGabriel45@users.noreply.github.com>
Co-authored-by: Vasile Gabriel Marian <56271768+VGabriel45@users.noreply.github.com>
examples/CREATE_AND_USE_A_SESSION.md
Outdated
const smartAccountAddress = await smartAccount.getAccountAddress(); | ||
|
||
const { sessionKeyAddress, sessionStorageClient } = | ||
await createAndStoreNewSessionKey(smartAccount, chain); |
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.
this will just create a new key pair? store it where?
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.
createAndStoreNewSessionKey
has three arguments. The third is optional and isn't used here: ISessionStorage
. If the third argument isn't used then it will pick a storage client for you by default: localStorage if in the browser and FileStorage if not
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.
got it and this storage client stores leaf info and by default stores pk in there as well?
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.
current majorly used devx i have seen from dapps and demos is
i. client creates session keypair and passes session key eoa to us. keeps pk with them but pases ethers signer object
ii. during the op you pass which session key eoa and which SVM address (or session id) useful to find a leaf and corresponding proof - for dummy sig, original sig stuff
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.
This makes sense. Passing the whole store back to the sdk doesn't make sense. I thought it would be more convenient but it's less secure. I will simplify this a bit and make the store less engrained.
feat: transfer ownership (#484)
I like createSessionKeyEOA (later could become createSessionKeyEOAWithPermissions with mpc network capabilities) |
9c05c9d
to
81d8d39
Compare
This reverts commit c49a1b2. chore: lint chore: parseReferenceValue chore: fix helper chore: helper
81d8d39
to
5c47361
Compare
* feat/transfer_ownership(in progress) * cleanup * fix linting * added test for transferOwnership with session key manager module * Fix linting * refactor: refactor based on PR review * improve ts doc + refactor tests * added "moduleAddress" param to transferOwnership() * fix module tests * removed console.logs * fixed lint + removed unused import * remove unused import * added argument type for module address --------- Co-authored-by: GabiDev <gv@popoo.io>
This reverts commit c618f26.
* release v4.3.0 * refactor: increase timeout for transferOwnership tests --------- Co-authored-by: GabiDev <gv@popoo.io>
Improved dx around creating and using a session. See here for an example
PR-Codex overview
This PR focuses on improving session management and smart account functionalities.
Detailed summary