-
Notifications
You must be signed in to change notification settings - Fork 83
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
SMA-392: Session Keys V2 #180
Changes from 35 commits
5cdea88
f4b3772
ced1fc2
b6574b8
30da23c
5cc20b1
6acc584
2e71194
6a84e57
089e7cb
d6451d1
edf5d3e
9166014
a75fa43
57ceed6
0575f17
8f5ea61
6ca3e6a
2dca0d4
a5f0d22
3e2eb30
08e5aff
eb6f21a
8d9db63
878d38e
5f3cfd5
c5fd4ad
058f8dd
ed20f48
b936ef4
e41d075
7453008
22bc7fc
1c524f9
c5ca9de
11cf3ad
ac206f4
3041d8f
f365630
c3fe846
50ccc11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,3 +24,4 @@ deployments/localhost | |
typings | ||
forge-cache | ||
out | ||
.env.bak | ||
ankurdubey521 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"solidity.compileUsingRemoteVersion": "v0.8.23+commit.f704f362" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.20; | ||
|
||
/* solhint-disable no-empty-blocks*/ | ||
ankurdubey521 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* @title Session Key Manager module for Biconomy Modular Smart Accounts. | ||
* @dev Stores the Session Information explicity in the storage, instead of maintainting | ||
* a merkle tree. | ||
* This reduces the amount of calldata required to validate a session key, making it cheaper on | ||
* L2s. | ||
* Allows for a session to be enabled explicity, or being batched with the first usage of said session | ||
* @author Ankur Dubey - <[email protected]> | ||
*/ | ||
interface ISessionKeyManagerModuleHybrid { | ||
struct SessionData { | ||
uint48 validUntil; | ||
uint48 validAfter; | ||
address sessionValidationModule; | ||
bytes sessionKeyData; | ||
} | ||
|
||
event SessionCreated( | ||
address indexed sa, | ||
bytes32 indexed sessionDataDigest, | ||
SessionData data | ||
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. would there be any benefit of this? 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. skm address + valid until + valid after separate 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. It's for cheaply storing the session data corresponding to a sessionId on-chain, and primarily for analytics which Data Squad has been requesting.
why would we want to emit SKM address? 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. just like paymaster address is key component of AA tx. with every session key tx a skm will be involved and makes sense to emit it's address. My concern was emitting entire session data and then decoding it may not be straight forward 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. I think decoding should be easy since it can be done directly with ABI, no? |
||
); | ||
|
||
event SessionDisabled( | ||
address indexed sa, | ||
bytes32 indexed sessionDataDigest | ||
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. similar thought process here 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. replied above. |
||
); | ||
|
||
/** | ||
* @dev creates a session for a smart account | ||
* @param sessionData session data | ||
*/ | ||
function enableSession(SessionData calldata sessionData) external; | ||
|
||
/** | ||
* @notice Explicitly disable a session. Can be useful is situations where a session | ||
ankurdubey521 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* needs to be disabled before it expires. | ||
* @param _sessionDigest digest of session key data | ||
*/ | ||
function disableSession(bytes32 _sessionDigest) external; | ||
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. from sdk pov would we / devs needs to store exact session information pieces to generate calldata for this? 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. Yes the Session Data (validUntil, validAfter, svm, data) needs to be maintained in the user's local storage. 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. ok 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. Do you think adding a batch disableSession would be helpful. The users can definitely call execute batch multiple times to disableSession but having a method might be useful. 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. added |
||
|
||
/** | ||
* @notice Returns session data for a given session digest and smart account | ||
* @param _sessionDataDigest digest of session key data | ||
* @param _sa smart account for which session key is being disabled | ||
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. As per the description.. "smart account for which session key is being disabled" 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. The description was incorrect, fixed it |
||
* @return data SessionData struct | ||
*/ | ||
function enabledSessionsData( | ||
bytes32 _sessionDataDigest, | ||
address _sa | ||
) external view returns (SessionData memory data); | ||
|
||
/** | ||
* @dev Returns session data digest | ||
* @param _data session data | ||
* @return digest of session data | ||
*/ | ||
function sessionDataDigest( | ||
SessionData calldata _data | ||
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. I am assuming we would need to store data for a session id(offchain identifier) then call this view method to get digest when needed 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. That is correct, but since this is a pure function it can be done off-chain for performance reasons. This is mainly to provide a reference implementation. |
||
) external pure returns (bytes32); | ||
} |
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.
.env.*
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.
would also cover
.env.example
, which should not be ignored