Add unit tests for eth address

[tcoratger]

Description

This pull request adds unit tests for Ethereum address conversion in the starknet-core crate.

Changes

• Added new unit tests.
• The new unit test reads addresses from a JSON file and tests the conversion of addresses to EthAddress types.

[xJonathanLEI]

While having more tests is generally better, I'm not sure if I get the point of adding these. The tests added are more of testing hex::decode than our own method it seems. I've decided to not merge this, as I feel like it adds unnecessary complexity to the test suite.

[tcoratger]

While having more tests is generally better, I'm not sure if I get the point of adding these. The tests added are more of testing hex::decode than our own method it seems. I've decided to not merge this, as I feel like it adds unnecessary complexity to the test suite.

@xJonathanLEI Here there were several objectives:

• Carry out tests with more than a single address to eliminate any possibility of bugs and omissions in the test suite (with the list proposed normally this risk is eliminated). The address formats taken into account are for example:
  • With prefix 0x
  • Without prefix
  • Null address
  • ...
• I don't think we test the hex decode method more than the repo functions. Indeed, there is a path which can be more or less long to generically transform the json addresses which can have different formats but this expected is always compared directly to the repo method itself without transformation. This is the case for example when we do assert_eq!(EthAddress::from_hex(address).unwrap(), eth_address); where we obtain a strict test between a method of the repo (from_hex) and a manual transformation to arrive at a bytes array. This ensures our method meets what we expect for it.
• There are also the following additions:
  • Obtaining Eth addresses from Felt (which was not previously tested)
  • Test of the error for an address which is beyond the acceptable range (overflow which was not tested before)

[xJonathanLEI]

Right. I guess what I meant is that it's better to target specific scenarios directly instead of feeding a list of random addresses. The JSON list thingy feels like you're fuzzing it, but it's just a very simple decoding method that I think we can simply test the edge cases directly.

[tcoratger]

Right. I guess what I meant is that it's better to target specific scenarios directly instead of feeding a list of random addresses. The JSON list thingy feels like you're fuzzing it, but it's just a very simple decoding method that I think we can simply test the edge cases directly.

@xJonathanLEI No problem, let me know if you still want to close it or I can just replace the "fuzzing" by 2-3 edge cases. Both are fine to me (I've no problem with closing a PR, always a way to learn something).

[xJonathanLEI]

Specific edge cases are surely desirable. Thanks!

[tcoratger]

Specific edge cases are surely desirable. Thanks!

@xJonathanLEI Fixed

[xJonathanLEI]

Rebased on master for CI and fixed missing WASM test annotation.

[xJonathanLEI]

LGTM