-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add comprehensive best practices and recommendations documentation #401
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: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review completed. Found 4 issues related to API accuracy in the documentation:
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| ```typescript | ||
| test.beforeEach(async ({ page, wallet }) => { | ||
| // Reset wallet state | ||
| await wallet.reset(); |
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.
The wallet.reset() method is used in multiple code examples throughout this documentation but doesn't appear to exist in the BaseWallet interface. Based on the source code, wallet operations use the handleAction() method pattern. This example may confuse users who try to implement it.
Fix it with Roo Code or mention @roomote and request a fix.
| await wallet.sendTransaction(...); | ||
|
|
||
| // Clean up | ||
| await wallet.reset(); |
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.
Same issue here - the wallet.reset() method doesn't exist in the BaseWallet API. This will cause confusion for users trying to implement the cleanup pattern shown in this example.
Fix it with Roo Code or mention @roomote and request a fix.
| test.describe('Critical Wallet Flows', () => { | ||
| test('should connect wallet successfully', async ({ page, wallet }) => { | ||
| await wallet.connect(); | ||
| expect(await wallet.isConnected()).toBe(true); | ||
| }); | ||
|
|
||
| test('should sign transactions', async ({ page, wallet }) => { | ||
| const tx = await wallet.sendTransaction({...}); | ||
| expect(tx.hash).toBeDefined(); | ||
| }); | ||
|
|
||
| test('should handle rejected transactions', async ({ page, wallet }) => { | ||
| await expect(wallet.rejectTransaction()).resolves.toBeTruthy(); | ||
| }); | ||
|
|
||
| test('should switch networks', async ({ page, wallet }) => { | ||
| await wallet.switchNetwork(baseSepolia.id); | ||
| expect(await wallet.getCurrentChainId()).toBe(baseSepolia.id); | ||
| }); | ||
| }); |
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.
These test examples use wallet methods like wallet.connect(), wallet.isConnected(), wallet.sendTransaction(), wallet.rejectTransaction(), wallet.switchNetwork(), and wallet.getCurrentChainId() that don't match the actual OnChainTestKit API. Based on the source code, wallet operations use the handleAction(action, options) pattern (e.g., wallet.handleAction('connect', {...})). These examples should be updated to reflect the actual API to avoid confusion.
Fix it with Roo Code or mention @roomote and request a fix.
| - [Playwright Best Practices](https://playwright.dev/docs/best-practices) | ||
| - [Web3 Testing Guide](https://ethereum.org/en/developers/docs/testing/) | ||
| - [OnChainTestKit Examples](/examples) | ||
| - [GitHub Repository](https://github.com/MunyayLLC/https-github.com-joe10832-onchaintestkit) |
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.
The GitHub repository URL uses the unusual repository name https-github.com-joe10832-onchaintestkit which appears to be the internal directory structure rather than the canonical repository name. This URL will work but looks unprofessional in user-facing documentation. Consider using a cleaner repository URL if one exists.
Fix it with Roo Code or mention @roomote and request a fix.
joe10832
left a comment
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.
Added a new documentation page covering best practices and recommendations for using OnChainTestKit, including:
The documentation has been added to both the navigation bar and sidebar under a new "Resources" section.
View task on Roo Code Cloud