divergencetech / ethier

Golang and Solidity SDK to make Ethereum development ethier
MIT License
217 stars 23 forks source link

SignatureChecker signed message generation #11

Closed cxkoda closed 2 years ago

cxkoda commented 2 years ago

Currently, SignatureChecker handles two responsibilities:

The former should be not be hardcoded into SignatureChecker following the SRP and should be left to the caller. This will make the system more flexible - allowing different message generation procedures such as required by EIP-191 to be easily integrated if needed.

divergencetech commented 2 years ago

My concern is that this takes a fairly simple file and splits it for limited benefit, thus adding complexity instead of reducing it. The single responsibility of the library can be "checking that signatures are valid for incoming data" and the SR for the functions are split as you suggest so each is responsible for generating the message and the one central one checks it.

The added complexity has 2 downsides IMO: (a) more to hold in one's head while reading / reviewing code and therefore greater chance of missed bugs; and (b) confuses the public API because an end user has to know what to use and how to pipe the data between them.

One of the exposed functions should allow for bytes32 so that advanced users (or us adding new features) can pass arbitrarily generated messages for checking.

cxkoda commented 2 years ago

In the old PR #1, I split the responsibilities into separate files/libraries. After reconsidering, I totally agree that the added complexity outweighs the benefits. Therefore, I opted in #12 to keep everything in a single library and already exposed an additional function for byte32 messages as you suggested.

ARR4N commented 2 years ago

Gotcha :) I've closed PR #1 and marked it as superseded by #12