-
Notifications
You must be signed in to change notification settings - Fork 63
Refactor/types namings tsdoc redundant code #880
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
base: master
Are you sure you want to change the base?
Conversation
…to refactor/types_namings_tsdoc_redundant_code
@@ -99,7 +114,7 @@ export interface SessionsInterface { | |||
addExplicitSession( | |||
walletAddress: Address.Address, | |||
sessionAddress: Address.Address, |
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.
sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.
@@ -121,7 +136,7 @@ export interface SessionsInterface { | |||
modifyExplicitSession( | |||
walletAddress: Address.Address, | |||
sessionAddress: Address.Address, |
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.
sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.
@@ -327,12 +342,12 @@ export class Sessions implements SessionsInterface { | |||
async addExplicitSession( | |||
walletAddress: Address.Address, | |||
sessionAddress: Address.Address, |
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.
sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.
@@ -341,7 +356,7 @@ export class Sessions implements SessionsInterface { | |||
async modifyExplicitSession( | |||
walletAddress: Address.Address, | |||
sessionAddress: Address.Address, |
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.
sessionAddress is an argument for addExplicitSession but its also a property of the explicitSession.
@@ -25,7 +25,7 @@ export type SessionPermissions = { | |||
chainId: number | |||
valueLimit: bigint | |||
deadline: bigint // uint64 | |||
permissions: [Permission, ...Permission[]] |
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 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?
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.
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?
permissions: Permission.Permission[] | ||
} | ||
|
||
export type ImplicitSession = Omit<Session, 'isImplicit'> |
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.
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.
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.
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.
Looking good. just a couple questions about the Session, ExplicitSession, ImplicitSession typings. and a couple other small things.
This PR introduces the following changes: