Improvement: make session fields optional

[kopy-kat]

Describe the feature you would like

In the session object, fields salt, userOpPolices and erc7739Polices could be optional with defaults

Additional context

No response

[kopy-kat]

adding on here, if permitERC4337Paymaster is true, it should be enforced that userOpPolicies.length >= 1

[Dhruv-Mishra]

The comments in the relevant code suggest that we want to make the salt property optional with a default value of 0 and make either of the properties listed after the second comment to be optional:

export type Session = {
  sessionValidator: Address
  sessionValidatorInitData: Hex
  // todo: make this optional with default 0
  salt: Hex
  // todo: make the below optional but require one of them to be defined
  userOpPolicies: PolicyData[]
  erc7739Policies: ERC7739Data
  actions: ActionData[]
  permitERC4337Paymaster: boolean
  chainId: bigint
}

At the moment we are keeping the object as a type that the user can directly access and cast to, without an initializer or default getter.

We can try adding these constraints as custom rules so that they get flagged by the linter but this would only apply to someone using a TS development environment and may still not be full proof as it will not enforce the constraints at runtime

Since TS types do not exist at runtime, it is not possible to directly enforce conditions within the definition of the type. I believe that we might have to make a default getter function or make it into a class with a constructor to enforce our custom logic or we can add typeguards in execution of all the related functions to enforce these conditions at runtime.

[kopy-kat]

just to clarify so we want salt to use default 0, erc7739Policies to be optional (this is easy); the most complex one is that we want userOp policies to be optional if permitERC4337Paymaster is false but have length >= 1 if permitERC4337Paymaster is true. I was thinking about something like this:

type BaseSession = {
 ...
 actions: ActionData[];
};

type Session = 
 | (BaseSession & { permitERC4337Paymaster: false })
 | (BaseSession & { permitERC4337Paymaster: true; actions: [ActionData, ...ActionData[]] });

this should work for ts devs right?

however, I do see your point about devs not using ts so all of this isnt very useful anymore. what do you think would make the most sense here? the session object is used in a lot of places (in src/modules/smart-sessions/usage.ts especially)

[Dhruv-Mishra]

That's an excellent solution for enforcing the constraints on permitERC4337Paymaster. It looks correct and should work seamlessly for TypeScript developers.

Using a similar approach as you suggested, we can enforce that salt is either 0 or a defined Hex value. I don't think we can initialize salt with 0 without using a factory function or class constructor, the root cause again being that ts types don't exist at runtime in js.

[kopy-kat]

yep thats what I thought on the salt - what would the implications be for using a class/factory? I guess a different solution is making it optional in the type and then in all functions that the type uses we fill in 0 if its undefined - this feels a bit hacky but might be cleanest - wdyt?

[Dhruv-Mishra]

The approach for enforcing permitERC4337Paymaster-related conditions works well when Session objects are explicitly defined. However, issues arise when creating a Session object by spreading properties from another object. In such cases, ESLint flags an error. Below is an example demonstrating this issue:

[kopy-kat]

interesting - whats the error here? feels like it shouldnt flag this - but also I dont think ppl would really use spreading for the sessions so imo could be fine

[Dhruv-Mishra]

yep thats what I thought on the salt - what would the implications be for using a class/factory? I guess a different solution is making it optional in the type and then in all functions that the type uses we fill in 0 if its undefined - this feels a bit hacky but might be cleanest - wdyt?

Setting a value of 0 for salt, within function calls might not be a long term solution for us as we will have to keep a track of all the calls on the object and any future methods using Session object will have to adhere to the same practice too

Using a class makes it easy to handle defaults like in the constructor and ensures everything is initialized properly. It might not be necessary for a simple data type like Session and may instead introduce an additional overhead.

A factory function is just a way to return an instance of Session, allowing us to execute our custom logic when creating it, so it might be more useful here

[Dhruv-Mishra]

interesting - whats the error here? feels like it shouldnt flag this - but also I dont think ppl would really use spreading for the sessions so imo could be fine

The error is of the object not adhering to the Session type. Interestingly, when I go to the location of the object and use it's arguments to explicitly specify a new Session object, there are no errors.

[kopy-kat]

A factory function is just a way to return an instance of Session, allowing us to execute our custom logic when creating it, so it might be more useful here

could you quickly explain how the devx would change here for the sdk user/dev?

[Dhruv-Mishra]

A factory function is just a way to return an instance of Session, allowing us to execute our custom logic when creating it, so it might be more useful here

could you quickly explain how the devx would change here for the sdk user/dev?

If we use a factory function SDK users will call the function to create a Session object, e.g., createSession({ ... }) instead of directly creating a json object with session type.

[kopy-kat]

ok, from past feedback devs dont like this too much (eg for getAccount) so lets keep this open for now and just accept that devs need to provide a salt

[Dhruv-Mishra]

Sounds good, will just add the permitERC4337Paymaster based conditions for now

[Dhruv-Mishra]

What do we plan to do about the objects which relied on data from userOpPolicies and erc7739 policies? For instance, the permissions field below accepts a SignedPermissions type object, and the definition of SignedPermissions object does not have these properties as optional.

export type SignedPermissions = {
  permitGenericPolicy: boolean
  permitAdminAccess: boolean
  ignoreSecurityAttestations: boolean
  permitERC4337Paymaster: boolean
  userOpPolicies: PolicyData[]
  erc7739Policies: ERC7739Data
  actions: ActionData[]
}

    const chainSessions: ChainSession[] = [
      {
        chainId: BigInt(sepolia.id),
        session: {
          ...session,
          permissions: {
            permitGenericPolicy: false,
            permitAdminAccess: false,
            ignoreSecurityAttestations: false,
            permitERC4337Paymaster: session.permitERC4337Paymaster,
            userOpPolicies: session.userOpPolicies,
            erc7739Policies: session.erc7739Policies,
            actions: session.actions,
          },
          account: account.address,
          smartSession: SMART_SESSIONS_ADDRESS,
          nonce: sessionNonce,
        },
      },
    ]

[kopy-kat]

hmm I thought you were just going to do the permitERC4337Paymaster / actions one

I think userOpPolicies and erc7739Policies have the same issue as salt where we'd need to pass [] if they're undefined - we can either do this or leave them as well - wdyt?

[Dhruv-Mishra]

hmm I thought you were just going to do the permitERC4337Paymaster / actions one

I think userOpPolicies and erc7739Policies have the same issue as salt where we'd need to pass [] if they're undefined - we can either do this or leave them as well - wdyt?

Do we have to enforce the condition for length on actions or on userOpPolicies?

[Dhruv-Mishra]

hmm I thought you were just going to do the permitERC4337Paymaster / actions one

I think userOpPolicies and erc7739Policies have the same issue as salt where we'd need to pass [] if they're undefined - we can either do this or leave them as well - wdyt?

Yes, for keeping 'userOpPolicies' and 'erc7739Policies' as optional within the Sessions type, we will either have to pass [] wherever it is undefined or we set them as optional across all types which use these

[kopy-kat]

sorry yes the length is on userOpPolicies - actually that reminds me that actions always needs to be length >= 1 so could you add that too please

[kopy-kat]

we will either have to pass [] wherever it is undefined or we set them as optional across all types which use these

the latter only works in ts but when we do encodings for interacting with smart contracts they will have to be defined ie [] - so maybe we just do the former

[Dhruv-Mishra]

sorry yes the length is on userOpPolicies - actually that reminds me that actions always needs to be length >= 1 so could you add that too please

Sure

[Dhruv-Mishra]

we will either have to pass [] wherever it is undefined or we set them as optional across all types which use these

the latter only works in ts but when we do encodings for interacting with smart contracts they will have to be defined ie [] - so maybe we just do the former

I see, will pass [] wherever necessary

[Dhruv-Mishra]

hmm I thought you were just going to do the permitERC4337Paymaster / actions one

I think userOpPolicies and erc7739Policies have the same issue as salt where we'd need to pass [] if they're undefined - we can either do this or leave them as well - wdyt?

You're right about this being the same issue as with salt, I think it would be better to leave this for now and make a unanimous decision on how this is handled for all the fields within a type, which require a default argument.

[Dhruv-Mishra]

Raised PR: #114 with the changes