Introduce Address type

[s1na]

As per the discussion in #182, this PR introduces a new class Address. It's experimental, and so I haven't yet exposed it. The API covers the most essential, but I think can probably be extended and improved, I'm open to ideas here. For the tests, I copied mostly from test/index.js. I'm also thinking about what other types we can introduce, apart from Address.

[coveralls]

Coverage increased (+0.03%) to 99.736% when pulling 37a74c1 on types into ae6515d on master.

[holgerd77]

Great start, thanks! 😀

Does this already encompass everything from account.ts?

[s1na]

Does this already encompass everything from account.ts?

No not yet, there are some functions for working with public and private keys there which didn't fit in here. There's another function which is somewhat related, which is toChecksumAddress. Should I include it?

[holgerd77]

Hmm, maybe we wait a bit until we have a bit of a broader picture how we relate these new structures to the existing code, how we can role out and this generally fit together.

[alcuadrado]

I was just reading about the TS migration and saw this PR.

There's another function which is somewhat related, which is toChecksumAddress. Should I include it?

IMO Address.prototype.toString should return a checksummed address. Not checksumming them can cause errors when working with libraries like ethers.js and web3 1.x.

In my experience, non-checksummed addresses are especially frustrating for new smart contract/dapp developers, as the way the checksum is encoded is super weird.

[s1na]

Not checksumming them can cause errors when working with libraries like ethers.js and web3 1.x.

Couldn't they opt-in specifically to use toChecksumAddress?

In the internal libraries using a checksummed-address is very rare afaik, and I'm not sure if dapp devs would be using ethereumjs-util directly?

[alcuadrado]

Not checksumming them can cause errors when working with libraries like ethers.js and web3 1.x.

Couldn't they opt-in specifically to use toChecksumAddress?

That's the current situation, you can call toChecksumAddress. The problem is that realizing that you have to opt-in is a frustrating process. Checksummed and non-checksummed addresses look exactly the same if you don't know what you are dealing with.

In the internal libraries using a checksummed-address is very rare afaik, and I'm not sure if dapp devs would be using ethereumjs-util directly?

I believe that's super common, at least for testing smart contracts. I use it all the time, and have seen it in lots of projects I audited.

To clarify, I don't think this class should store a checksummed representation of the address. A Buffer seems right. I'd just return checksummed addresses in toString().

[holgerd77]

@alcuadrado @s1na Hmm, I would also tend to not making stuff like this implicit but let the users (developers) decide what to choose. We can rather do some hint in the code comments?

But also haven't completely made up my mind.

[alcuadrado]

After implementing EIP-1191, I reconsidered. As there are more than one checksum algorithms, Address.prototype.toString should return a lowercase string.

[ryanio]

This would be useful for me in ethereumjs/ethereumjs-monorepo#812 where I created my own Address class, but would prefer to use one available globally in this lib.

I rebased this branch on master, updated the test file to ts, generated docs, and added a line in the readme. Would love to get this merged in and released.

[holgerd77]

Just some reassurances before we proceed here:

1. This could be released as a patch release, right?

2. Might there be negative performance implications on some scenarios if we wrap addresses into a class? //cc @alcuadrado @jochem-brouwer

[ryanio]

1. This could be released as a patch release, right?

yep! it's an additional class that doesn't affect anything else.

2. Might there be negative performance implications on some scenarios if we wrap addresses into a class?

i don't anticipate so as the class is simply one reference to a buffer, however i would be interested in hearing others' opinions.

[holgerd77]

Ok, I am through here with a review. Code + tests look good, I also like the API.

[holgerd77]

Side note: there were some issues raised on the library like #268 lately. Before we do another release we should first have a look here.

[alcuadrado]

2. Might there be negative performance implications on some scenarios if we wrap addresses into a class? //cc @alcuadrado @jochem-brouwer

TBH I'd be lying if I'd say anything about the performance impact based on intuition.