Skip to content

Refactor/types namings tsdoc redundant code#880

Merged
tolgahan-arikan merged 15 commits intomasterfrom
refactor/types_namings_tsdoc_redundant_code
Sep 23, 2025
Merged

Refactor/types namings tsdoc redundant code#880
tolgahan-arikan merged 15 commits intomasterfrom
refactor/types_namings_tsdoc_redundant_code

Conversation

@VGabriel45
Copy link
Contributor

This PR introduces the following changes:

  • refactor "permissions" to refer to Session Permissions
  • refactor "session" to refer to Session (implicit or explicit)
  • refactor "explicitSession" to refer to explicit session
  • improved RequestActionType types to cover more explicit scenarios
  • added "getAllImplicitSessions()" and getAllExplicitSessions()"
  • refactor "permissions" type in wallet/primitives SessionPermissions to be Permission[], [Permission, Permission[]] was causing too many type issues on client side and required to be casted

@@ -121,7 +136,7 @@ export interface SessionsInterface {
modifyExplicitSession(
walletAddress: Address.Address,
sessionAddress: Address.Address,
Copy link
Contributor

@corbanbrook corbanbrook Sep 19, 2025

Choose a reason for hiding this comment

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

sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.

chainId: number
valueLimit: bigint
deadline: bigint // uint64
permissions: [Permission, ...Permission[]]
Copy link
Contributor

Choose a reason for hiding this comment

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

This typing [Permission, ...Permission[]] is cute typing trick that uses a tuple type to specify that there must be at least 1 permission given in the array. where as Permission[] allows an empty array. Was this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I noticed with this sugar is that you need to cast "permissions" to [Permission, ...Permission[]] all the time otherwise typescript will complain, this looks like a workaround and is not very intuitive for the developer that is integrating, I would prefer if the sdk simply checks the permissions array to see if it's empty or not.

action: (typeof RequestActionType)['ADD_EXPLICIT_SESSION' | 'MODIFY_EXPLICIT_SESSION']
response?: AddExplicitSessionSuccessResponsePayload | ModifySessionSuccessResponsePayload
error?: any
export type DappClientExplicitSessionEventListener = ExplicitSessionEventListener & {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is strange to me ... we are tagging the event listener with a chain id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. We're adding a chainId property to the listener function itself because the DappClient needs it, so it's an ExplicitSessionEventListener with an extra property "chainId" which makes the DappClientExplicitSessionEventListener.

permissions: Permission.Permission[]
}

export type ImplicitSession = Omit<Session, 'isImplicit'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not sure why the ImplicitSession type is omitting isImplicit. It might be better to use a discriminating type union here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically if you had something like:

type Session = ImplicitSession | ExplicitSession

interface ImplicitSession  {
  type: 'implicit',
  ...
}

interface ExplicitSession  {
  type: 'explicit',
   ...
}

These could extend a base interface with common properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep agree, I refactored this one.

Copy link
Contributor

@corbanbrook corbanbrook left a comment

Choose a reason for hiding this comment

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

Looking good. just a couple questions about the Session, ExplicitSession, ImplicitSession typings. and a couple other small things.

@VGabriel45 VGabriel45 marked this pull request as ready for review September 23, 2025 15:11
@VGabriel45 VGabriel45 requested review from a team as code owners September 23, 2025 15:11
@tolgahan-arikan tolgahan-arikan merged commit 429237f into master Sep 23, 2025
3 checks passed
@tolgahan-arikan tolgahan-arikan deleted the refactor/types_namings_tsdoc_redundant_code branch September 23, 2025 15:13
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.

3 participants