search

faker-js / faker

Generate massive amounts of fake data in the browser and node.js
https://fakerjs.dev
Other
12.98k stars 919 forks source link

Adding checksummed ethereum addresses #1567

Closed RareBodhi closed 1 year ago

RareBodhi commented 1 year ago

Clear and concise description of the problem

When developing crypto-related applications, wallet addresses are of great importance and they actually exist in a number of different formats. One is a non-checksummed address and one is checksummed.

Non Checksum ( lowercase ): 0x4bbeeb066ed09b7aed07bf39eee0460dfa261520 Checksum ( caps letters ): 0x4bbeEB066eD09B7AEd07bF39EEe0460DFa261520

We have a use case in our e2e tests for testing checksummed and non-checksummed addresses.

Suggested solution

I would propose a solution to implement a new finance method which simply is a checksummed version of the string.

Current: faker.finance.ethereumAddress()

New:

  1. faker.finance.ethereumAddress()
  2. faker.finance.ethereumAddressChecksum()

Alternative

No response

Additional context

No response

import-brain commented 1 year ago

Perhaps we could have checksum as an optional boolean parameter for finance.ethereumAddress()?

RareBodhi commented 1 year ago

Perhaps we could have checksum as a boolean parameter for finance.ethereumAddress()?

@import-brain That is actually a much better alternative, I think that would be a useful optinon, definitely for our codebase. This seems like something that should be relatively easy to implement.

Would you like me to open a PR for this, or is it something you are able to implement yourself quite quickly?

import-brain commented 1 year ago

Perhaps we could have checksum as a boolean parameter for finance.ethereumAddress()?

@import-brain That is actually a much better alternative, I think that would be a useful optinon, definitely for our codebase. This seems like something that should be relatively easy to implement.

Would you like me to open a PR for this, or is it something you are able to implement yourself quite quickly?

I think we need some more input before proceeding with this idea.

@Shinigami92 @ST-DDT @xDivisionByZerox What do you think about this proposal?

ST-DDT commented 1 year ago

I think an optional string literal might be suited better here: type: 'the-normal-thing' | 'checksum'

usage (potentially wrapped in options: {}):

etherumAddress('checksum')

That way it is easier to read without looking up the boolean value.

xDivisionByZerox commented 1 year ago

Perhaps we could have checksum as a boolean parameter for finance.ethereumAddress()?

Just to understand it correctly. The checksum flag would just toggle the casing between 'lower' and 'mixed' correctly?

RareBodhi commented 1 year ago

Perhaps we could have checksum as a boolean parameter for finance.ethereumAddress()?

Just to understand it correctly. The checksum flag would just toggle the casing between 'lower' and 'mixed' correctly?

That would be correct.

ST-DDT commented 1 year ago

Sounds good to me.

Are you willing to implement this?

Please implement it with options: { type: '???' | 'checksum' } = {} or optionally 'options: ???' | 'checksum' | options: { type: '???' | 'checksum' } = {}. Is address a good value for the ??? placeholder? Please make sure that the check-summed version returns valid results.

This page can be used to verify it: https://ethsum.netlify.app/

RareBodhi commented 1 year ago

@ST-DDT They are both technically an address so I'm not sure it would be a good placeholder for ???

I would propose simply non-checksum and checksum as the two options since checksumming is the preferred practice. Feel it's aptly descriptive to do that.

RareBodhi commented 1 year ago

A PR has been opened here if you have a change to review https://github.com/faker-js/faker/pull/1578

ST-DDT commented 1 year ago

FFR: This might be impossible to implement for now without adding external dependencies.