CU-86dt27bzj - Create a WcSdk method to wipe all requests from a dapp #113
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
e2e/tests/DappMethods.spec.ts
Outdated
| const dappPage = DAPP_REACT | ||
| const walletPage = WALLET_REACT | ||
| await connectReactDappToNewReactAccount(context, dappPage, walletPage) | ||
| await dappPage.page.waitForLoadState('networkidle') // Wait to load request |
There was a problem hiding this comment.
Instead of using waitForLoadState('networkidle') and awaitSeconds(5), consider using waitForSelector or waitForFunction to ensure the specific element or condition is ready before proceeding.
There was a problem hiding this comment.
It seems like a good suggestion to use this waitForSelector to avoid the loop.
e2e/tests/DappMethods.spec.ts
Outdated
| await connectReactDappToNewReactAccount(context, dappPage, walletPage) | ||
| await dappPage.page.waitForLoadState('networkidle') // Wait to load request | ||
| await dappPage.awaitSeconds(5) // Wait for 5 seconds | ||
| for (let i = 0; i <= 3; i++) { |
There was a problem hiding this comment.
Avoid using a loop without a clear exit condition. If the 'hello-world__sign-transaction' button is not available, this could result in an infinite loop.
e2e/tests/DappMethods.spec.ts
Outdated
| for (let i = 0; i <= 3; i++) { | ||
| // Click on Sign Transaction button 3 times | ||
| await dappPage.page.getByTestId('hello-world__sign-transaction').click() | ||
| await dappPage.awaitSeconds(0.5) |
There was a problem hiding this comment.
Avoid using arbitrary wait times like awaitSeconds(0.5). It's better to wait for specific elements or conditions to be met.
e2e/tests/DappMethods.spec.ts
Outdated
| await dappPage.awaitSeconds(0.5) | ||
| } | ||
| await dappPage.page.getByTestId('hello-world__wipe-methods').click() // Click on Wipe Methods button | ||
| await dappPage.awaitSeconds(2) // Wait for 2 seconds |
There was a problem hiding this comment.
Avoid using arbitrary wait times like awaitSeconds(2). It's better to wait for specific elements or conditions to be met.
e2e/tests/DappMethods.spec.ts
Outdated
| await dappPage.page.getByTestId('hello-world__wipe-methods').click() // Click on Wipe Methods button | ||
| await dappPage.awaitSeconds(2) // Wait for 2 seconds | ||
| const pendingRequests = await walletPage.page.getByTestId('default-card__pending-request').all() | ||
| expect(pendingRequests.length).toBe(0) // Verify if the response had a return |
There was a problem hiding this comment.
The comment // Verify if the response had a return is not clear. Please provide a more descriptive comment.
| @@ -377,4 +387,27 @@ export class WcWalletSDK { | |||
| this.requests = filteredRequests | |||
| } | |||
| } | |||
|
|
|||
| private async wipeRequests({ session, request }: TAdapterMethodParam): Promise<string[]> { | |||
There was a problem hiding this comment.
Consider renaming the method wipeRequests to something more descriptive, like wipeAllDappRequests, to clearly indicate its purpose.
| @@ -377,4 +387,27 @@ export class WcWalletSDK { | |||
| this.requests = filteredRequests | |||
| } | |||
| } | |||
|
|
|||
| private async wipeRequests({ session, request }: TAdapterMethodParam): Promise<string[]> { | |||
| const result: string[] = [] | |||
There was a problem hiding this comment.
The result variable could be named more descriptively. Consider renaming it to wipedRequests or clearedRequests.
| (dappRequest: TSessionRequest) => dappRequest.topic === session.topic && dappRequest.id !== request.id, | ||
| ) ?? []) as TSessionRequest[] | ||
|
|
||
| for (const dappRequest of dappRequests) { |
There was a problem hiding this comment.
Consider using Promise.all with Array.map for handling multiple promises. This could potentially improve the performance by executing the promises concurrently.
| topic: dappRequest.topic, | ||
| response: formatJsonRpcError(dappRequest.id, { | ||
| code: 1, | ||
| message: 'rejected by the dapp', |
There was a problem hiding this comment.
The error code 1 is not descriptive. Consider using a constant with a descriptive name or a standard error code if available.
| }), | ||
| }) | ||
| } finally { | ||
| this.requests = this.requests.filter(({ id }) => id !== dappRequest.id) |
There was a problem hiding this comment.
The requests array is being updated inside a loop. This might lead to performance issues for large arrays. Consider collecting all the ids to be removed in an array and then use Array.filter once after the loop.
e2e/tests/DappMethods.spec.ts
Outdated
| const dappPage = DAPP_REACT | ||
| const walletPage = WALLET_REACT | ||
| await connectReactDappToNewReactAccount(context, dappPage, walletPage) | ||
| await dappPage.page.waitForLoadState('networkidle') // Wait to load request |
There was a problem hiding this comment.
It seems like a good suggestion to use this waitForSelector to avoid the loop.
| try { | ||
| await wcSdk.wipeRequests() | ||
| } catch (error) { | ||
| console.log(error) |
There was a problem hiding this comment.
I think you can remove this try-catch with the console.log.
It's not supposed to fail here, so allow the app to crash when it fails.
| @@ -43,6 +43,7 @@ export class WcWalletSDK { | |||
| private _proposals: TSessionProposal[] = [] | |||
| private _requests: TSessionRequest[] = [] | |||
| private _status: EStatus = EStatus.NOT_STARTED | |||
| private _permissibleWalletRequestMethods: Record<string, (args: TAdapterMethodParam) => Promise<any>> | |||
There was a problem hiding this comment.
I think we can call it nonAdapterMethods, it's not pretty, but it's very clear.
LeonardoDizConde
force-pushed
the
CU-86dt27bzj
branch
from
804f2d2
to
dc98ab1
Compare
| @@ -203,26 +205,26 @@ packages: | |||
| typescript: | |||
| optional: true | |||
There was a problem hiding this comment.
Consider using the latest version of '@eslint-community/regexpp' for better performance and security.
| '@typescript-eslint/utils': 6.7.4(eslint@8.50.0)(typescript@5.2.2) | ||
| '@typescript-eslint/visitor-keys': 6.7.4 | ||
| '@eslint-community/regexpp': 4.10.0 | ||
| '@typescript-eslint/parser': 6.21.0(eslint@8.57.0)(typescript@5.4.5) |
There was a problem hiding this comment.
Update '@typescript-eslint/parser' to the latest version to leverage new features and improvements.
| '@typescript-eslint/visitor-keys': 6.7.4 | ||
| '@eslint-community/regexpp': 4.10.0 | ||
| '@typescript-eslint/parser': 6.21.0(eslint@8.57.0)(typescript@5.4.5) | ||
| '@typescript-eslint/scope-manager': 6.21.0 |
There was a problem hiding this comment.
Consider updating '@typescript-eslint/scope-manager' to the latest version for better performance and stability.
| '@eslint-community/regexpp': 4.10.0 | ||
| '@typescript-eslint/parser': 6.21.0(eslint@8.57.0)(typescript@5.4.5) | ||
| '@typescript-eslint/scope-manager': 6.21.0 | ||
| '@typescript-eslint/type-utils': 6.21.0(eslint@8.57.0)(typescript@5.4.5) |
There was a problem hiding this comment.
Update '@typescript-eslint/type-utils' to the latest version to leverage new features and improvements.
| '@typescript-eslint/parser': 6.21.0(eslint@8.57.0)(typescript@5.4.5) | ||
| '@typescript-eslint/scope-manager': 6.21.0 | ||
| '@typescript-eslint/type-utils': 6.21.0(eslint@8.57.0)(typescript@5.4.5) | ||
| '@typescript-eslint/utils': 6.21.0(eslint@8.57.0)(typescript@5.4.5) |
There was a problem hiding this comment.
Consider updating '@typescript-eslint/utils' to the latest version for better performance and stability.
|
|
||
| const executionTime = performance.now() - startExecutionTime | ||
| result = await adapterMethod.apply(blockchainOptions.adapter, [{ request, session }]) | ||
| } else throw new Error('Invalid request method') |
There was a problem hiding this comment.
The error message 'Invalid request method' is not informative enough. Consider providing more details about the error.
| result = await adapterMethod.apply(blockchainOptions.adapter, [{ request, session }]) | ||
| } else throw new Error('Invalid request method') | ||
|
|
||
| const executionTime = Math.abs(performance.now() - startExecutionTime) |
There was a problem hiding this comment.
Consider using performance.now() directly in the subtraction to avoid the use of Math.abs(). This will make the code cleaner and easier to read.
| }), | ||
| ) | ||
|
|
||
| this.requests = this.requests.filter((dappRequest: TSessionRequest) => dappRequest.topic !== session.topic) |
There was a problem hiding this comment.
The this.requests filter operation is outside the Promise.all block. This might lead to a race condition where the requests are not properly wiped if the promises have not yet resolved. Consider moving this line inside the finally block of the Promise.all block.
| @@ -63,4 +63,9 @@ export type TAdapterMethodParam = { | |||
| session: TSession | |||
| } | |||
|
|
|||
| export enum ResponseErrorCode { | |||
| REJECT = 1, | |||
There was a problem hiding this comment.
Consider adding a brief description for each enum member to clarify their purpose and usage.
| @@ -63,4 +63,9 @@ export type TAdapterMethodParam = { | |||
| session: TSession | |||
| } | |||
|
|
|||
| export enum ResponseErrorCode { | |||
| REJECT = 1, | |||
| DISCONNECT = 5900, | |||
There was a problem hiding this comment.
The error code DISCONNECT seems to be arbitrary. If there's a specific reason for choosing 5900, it would be better to document it somewhere outside the code.
No description provided.