-
Notifications
You must be signed in to change notification settings - Fork 29
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
Improve Test Coverage for packages/web3-wallet/src #464
base: master
Are you sure you want to change the base?
Conversation
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.
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 |
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 |
resolves #458