xmtp / xmtp-js

XMTP client SDKs, content types, and other packages written in TypeScript
https://xmtp.org/docs
211 stars 40 forks source link

ENS domain capitalization error when sending messages #211

Closed edisonqu closed 1 year ago

edisonqu commented 1 year ago

When getting ENS names, even when they are verified on the XMTP chain. Etherscan gives out two different contracts, with different lowercase and uppercase.

image

When putting it through XMTP, it would not recognize that it was a valid account even though it was from the same ENS domain on etherscan.

@jazzz helped us identify this issue and hopefully this can be fixed for future updates OR @agxmbhir and I can help you with that

jazzz commented 1 year ago

Hey @edisonqu thanks for posting this! I think we can make this easier for developers such as yourself who are getting started with XMTP.

Adding some context to the above post, currently it appears XMTP requires case-senstive addresses where ethereum addresses are case-insensitive. Thought addresses can optionally be eip55 encoded to provide a checksum.

Current Behavior: Attempting to message a valid lowercase address leads to an error incorrectly indicating the address is 'not on the network.'

Expected behavior:

example test case:

 const wallet = Wallet.fromMnemonic(
      'exchange crash march aspect home blind bench electric deputy language physical gentle'
    )
    const client = await Client.create(wallet)

    assert.ok(
      await client.canMessage('0xCeda31E813cD606228E71E87E94717CD3C6F3bab')
    )
    assert.ok( // Lower case fails
      await client.canMessage('0xCeda31E813cD606228E71E87E94717CD3C6F3bab'.toLowerCase())
    )

@edisonqu did I get that right?

edisonqu commented 1 year ago

That is true, I believe a simple .toLowercase() or .toUppercase() can do the job when authenticating the if the wallet is on the network or not.

For example in the Client, in the canMessage functions :

   * Check if @peerAddress can be messaged, specifically it checks that a PublicKeyBundle can be
   * found for the given address
   */

**// standardizing the case for the string can make sure that all addresses can be passed through without worrying about capitalization of letters in the address**
  public async canMessage(peerAddress: string): Promise<boolean> {
    const keyBundle = await this.getUserContact(peerAddress**.toLowerCase()**)
    return keyBundle !== undefined
  }

  static async canMessage(
    peerAddress: string,
    opts?: Partial<NetworkOptions>
  ): Promise<boolean> {
    const apiUrl = opts?.apiUrl || ApiUrls[opts?.env || 'dev']
    const keyBundle = await getUserContactFromNetwork(
      new ApiClient(apiUrl, { appVersion: opts?.appVersion }),
      peerAddress**.toLowerCase()**
    )
    return keyBundle !== undefined
  }

Please let me know if this works or if it doesn't, genuinely curious about the solution.

saulmc commented 1 year ago

Hey @edisonqu thank you for #220 ! Totally agree this can be made easier.

Some context on this issue and options were discussed in #172 . What do you think of the proposal of using https://docs.ethers.io/v5/api/utils/address/#utils-getAddress to standardize on the checksum format?

edisonqu commented 1 year ago

Hi @saulmc, thank you for addressing the pull request! I appreciate the team looking into this.

It works perfectly fine, good way to standardize. Thanks for letting me know!

I tested it and it works wonderfully. Everything is now standardized for the checksum format.

const ethers = require("ethers")

console.log("0xCeda31E813cD606228E71E87E94717CD3C6F3bab".toLowerCase())` // 0xceda31e813cd606228e71e87e94717cd3c6f3bab

console.log(ethers.utils.getAddress("0xCeda31E813cD606228E71E87E94717CD3C6F3bab".toLowerCase())) // 0xCeda31E813cD606228E71E87E94717CD3C6F3bab
console.log(ethers.utils.getAddress("0xCeda31E813cD606228E71E87E94717CD3C6F3bab")) // 0xCeda31E813cD606228E71E87E94717CD3C6F3bab

console.log("0xCeda31E813cD606228E71E87E94717CD3C6F3bab".toLowerCase()) // 0xceda31e813cd606228e71e87e94717cd3c6f3bab 

For @jazzz 's code:


const wallet = Wallet.fromMnemonic(
      'exchange crash march aspect home blind bench electric deputy language physical gentle'
    )
    const client = await Client.create(wallet)

    assert.ok(
      await client.canMessage(ethers.utils.getAddress('0xCeda31E813cD606228E71E87E94717CD3C6F3bab'))
    )
    assert.ok( // Lower case no longer fails 
      await client.canMessage(ethers.utils.getAddress('0xCeda31E813cD606228E71E87E94717CD3C6F3bab'.toLowerCase())))

Integrating the ethers.utils.getAddress with canMessage is the most optimal move here. Let me know if I am able to help anywhere :)