-
Notifications
You must be signed in to change notification settings - Fork 32
Improve Test Coverage for packages/web3-wallet/src #464
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
h0ngcha0
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.
Thanks for the contribution. Only minor comments.
| const wallet = PrivateKeyWallet.Random() | ||
| expect(wallet.address).toBeDefined() | ||
| expect(wallet.publicKey).toBeDefined() | ||
| expect(wallet.address.length).toBeGreaterThan(0) |
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.
Lets verify the exact length of address and publicKey here.
|
|
||
| expect(signature).toBeDefined() | ||
| expect(typeof signature).toBe('string') | ||
| // Optionally add more checks to validate the signature |
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.
Please verify if the signature is valid, we can use the verifySignature function.
h0ngcha0
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.
LGTM 👍
| }) | ||
|
|
||
| it('should handle large messages for encryption and decryption', () => { | ||
| const largeMessage = 'A'.repeat(1_000_000) // 1 MB of data |
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.
I'd be happy with 500_000 since this single test takes 700ms (on CI it could be twice as much) and it would add up a lot in the big picture, I guess that supporting 0.5MB messages is enough
|
changed @pragmaxim |
pragmaxim
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.
LGTM 👍
|
okay ser will it be merged? @pragmaxim |
closes #458