Boilertalk / Web3.swift

A pure swift Ethereum Web3 library
MIT License
636 stars 187 forks source link

Update cryptoswift 1.5.1 and fix EthereumPublicKey initializer #124

Closed DmitryBespalov closed 1 year ago

DmitryBespalov commented 2 years ago

Updated dependency and fixed bug in the public key initializer

wainglaister commented 2 years ago

It would be good if there was a test to prove the public key recovery fix (the fix is good, just missing the test) :)

koraykoska commented 1 year ago

What does not work with the public key recovery process right now? Why did you change it?

DmitryBespalov commented 1 year ago

@wainglaister ok, will add a test.

@koraykoska it recovers to a wrong address because it uses hash of a wrong preimage: it was hashing a signature instead of a message, which is nonsense to me 😊

I think it's a typo, but without the fix the method produces wrong values

wainglaister commented 1 year ago

Relates to this issue: https://github.com/Boilertalk/Web3.swift/issues/12

koraykoska commented 1 year ago

@DmitryBespalov Can you please add test cases from official specifications so we avoid future regressions

DmitryBespalov commented 1 year ago

@koraykoska sorry, I'm overloaded with the day job tasks at the moment, I won't have time to look at it till the middle of next week

proggeramlug commented 1 year ago

I need this in production, could someone release this please?

koraykoska commented 1 year ago

@proggeramlug CryptoSwift was updated and released. I still fail to find official examples about the recovery of the public key and I think the implementation in this PR is wrong. If you can, please add test cases and link to official docs so I can get it merged.

podkovyrin commented 1 year ago

I would suggest this as a trustworthy source: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/util/src/signature.ts#L53

Tests: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/util/test/signature.spec.ts#L101

Reference: Yellow Paper, see page 26, Appendix F, line (314)