search

f-o-a-m / purescript-web3

a purescript library for the web3 api
Apache License 2.0
127 stars 24 forks source link

Parsing of mixed case hexstrings fails #45

Closed kejace closed 6 years ago

kejace commented 6 years ago

We used to be able to parse hex-strings such as 0xAbCd, now we're failing on this. My suspicion is that this was maybe introduced with #36 but I haven't verified that.

Why do we want to be able to parse mixed case strings? See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-55.md

Of course, we should likely implement this feature in the in the Address constructor and throw there instead.

kejace commented 6 years ago

I can verify that this happens from fromJust $ mkHexString "0xAbBa":

/Users/kejace/devel/purescript-web3/output/Data.Maybe/index.js:127
            throw new Error("Failed pattern match at Data.Maybe line 270, column 1 - line 270, column 46: " + [ v.constructor.name ]);
            ^
kejace commented 6 years ago

I think it is important we allow mixed-case strings, because often users copy-paste addresses from websites that follow the EIP55.

I propose we

  1. Fix the hex string parser to allow mixed case
  2. Create an alternate constructor for mkAddress that follows EIP55 that fails on wrong check-sum implied by the mixed-case encoded hexstring.
martyall commented 6 years ago

@kejace

there actually isn't a hex string parser anymore because i still had problems with stack overflow, we're just basically running through the text input and making sure that it "is a valid hex string"

so for (1) you can just supplement the character set in this function https://github.com/f-o-a-m/purescript-web3/blob/master/src/Network/Ethereum/Web3/Types/Types.purs#L139

for (2) i don't know if i understand exactly what you are proposing, can you just write the pr?

cmditch commented 6 years ago

@kejace

Regarding (2) @XertroV actually helped implement this in elm-web3, this might help for reference https://github.com/cmditch/elm-web3/pull/44/files#diff-5dcf14483310ac06f8844a6458d78110R133

XertroV commented 6 years ago

@kejace - I think it was introduced here: https://github.com/f-o-a-m/purescript-web3/commit/328a47b6c20943a604e87406618db1c3108d6966#diff-ededbfc84072116829037439fdc1c254L136

(You can see that only lower case chars are used), sounds like the soln is just to include ABCDEF in the list..

We should probably check the length % 2 == 0 also