Refactor/types namings tsdoc redundant code#880
Conversation
…to refactor/types_namings_tsdoc_redundant_code
| @@ -121,7 +136,7 @@ export interface SessionsInterface { | |||
| modifyExplicitSession( | |||
| walletAddress: Address.Address, | |||
| sessionAddress: Address.Address, | |||
There was a problem hiding this comment.
sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.
| chainId: number | ||
| valueLimit: bigint | ||
| deadline: bigint // uint64 | ||
| permissions: [Permission, ...Permission[]] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 & { |
There was a problem hiding this comment.
This is strange to me ... we are tagging the event listener with a chain id?
There was a problem hiding this comment.
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'> |
There was a problem hiding this comment.
Im not sure why the ImplicitSession type is omitting isImplicit. It might be better to use a discriminating type union here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep agree, I refactored this one.
corbanbrook
left a comment
There was a problem hiding this comment.
Looking good. just a couple questions about the Session, ExplicitSession, ImplicitSession typings. and a couple other small things.
This PR introduces the following changes: