Skip to content
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

feat: Added support for handling lowercase addresses #233

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

alexrisch
Copy link
Contributor

Will now get checksum addresses from provided addresses Added some helper libraries to avoid adding additional dependency on ethers

Simulator Screenshot - iPhone 15 Pro - 2024-02-01 at 14 10 37

Will now get checksum addresses from provided addresses
Added some helper libraries to avoid adding additional dependency on ethers
@alexrisch alexrisch requested a review from a team as a code owner February 1, 2024 21:26
@@ -0,0 +1,50 @@
import { keccak_256 } from '@noble/hashes/sha3'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Port from viem getAddress

@cameronvoell
Copy link
Contributor

this change looks good, sounds like it is addressing a specific issue with adding new conversations.

I wonder if we should add the new getAddress in a few more locations. For example if you pass in a lowercase address to client.contacts.deny in the test linked below, I think we will fail to block the address. Perhaps we should merge this change, but keep your issue open until we feel we have a comprehensive list of places where checksums need to added. WDYT?

await bo.contacts.deny([alixConversation.peerAddress])

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with the recommendation we keep #230 open for now

@alexrisch
Copy link
Contributor Author

this change looks good, sounds like it is addressing a specific issue with adding new conversations.

I wonder if we should add the new getAddress in a few more locations. For example if you pass in a lowercase address to client.contacts.deny in the test linked below, I think we will fail to block the address. Perhaps we should merge this change, but keep your issue open until we feel we have a comprehensive list of places where checksums need to added. WDYT?

await bo.contacts.deny([alixConversation.peerAddress])

Good catch, let me figure out LOE and determine

Added handling to ensure addresses are checksums
@alexrisch
Copy link
Contributor Author

Approved with the recommendation we keep #230 open for now

Updated handling for contacts methods

@cameronvoell cameronvoell self-requested a review February 2, 2024 01:04
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, think this PR should fully close #230 now, nice!

@alexrisch alexrisch merged commit 9b08182 into main Feb 2, 2024
4 of 5 checks passed
@alexrisch alexrisch deleted the ar/address-lowercase branch February 2, 2024 02:44
Copy link
Contributor

github-actions bot commented Feb 2, 2024

🎉 This PR is included in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@alexrisch alexrisch restored the ar/address-lowercase branch February 7, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants