Breaking API changes (stricter type requirements)

[holgerd77]

Currently some overhaul of the API with breaking API changes with stricter type requirements is discussed within the TypeScript transition PR, will open this issue here for transparency, everyone interested feel free to join the discussion!

See API guidelines and change proposals in the thread.

[holgerd77]

@alcuadrado @s1na I think it might be more targeted if we just come up with some comprise set of general API guidelines and rules (like "A string representing a hex value has always to be prefixed with 0x", similar rules, error case definitions) and we then go along the methods and modify so that these comply with the rules.

I will try to come up with a suggestion during the next 1-2 days, feel free to also already contribute, eventually we use a shared comment preceding this one and edit until we are satisfied.

We can eventually even add this to the organizational docs at the end and have some common guidelines for API creation, we could also point to on PRs and also use for self-guidance on refactoring and general library work.

[holgerd77]

API Guidelines and Rules DRAFT (Shared Post)

1. Hex Values and Strings

1.1 Hex Value Format

A string representing a hex value always has to be prefixed with 0x, on passing a non-prefixed string the library should throw.

Reasoning

This ensures (to a larger extend) that work is really be done within the hex value set and no invalid values are passed leading to undefined/unexpected behavior. Throwing allows for clean handling of invalid value passing on the user side.

1.2 Hex Value Naming

A hex value parameter should always be prefixed with "hex" on parameter naming so that it can be clearly identified as a hex string, most broad naming in this sense would be "hexString".

Reasoning

This gives more transparency for people looking into/changing the inner logic of the code and prevents errors on this side. It also is an additional hint for users of the method (since TypeScript can not play out its benefits on this low-level hex/string differentiation solution.

1.3 Hex Value / String Uniqueness

A string parameter should either represent a common string or a hex value, never both.

Reasoning

Value sets are otherwise ambiguous, for strings like "0x1234" it can't be decided if being a hex string or a random string with a preceding "0x". Generally this type of mixing up values has turned out dangerous in the past since the same input can have a very different interpretation depending on regarded as hex or string which is a significant entrypoint for erroneous behavior.

2. Buffer-centric Methods

2.1 No implicit Buffer Conversion

There should be no buffer conversion for methods ultimately needing a buffer for execute its intended logic. Instead buffer conversion should be left to the user.

Reasoning

This forces users into explicitly do the buffer conversion and think about eventual implications and potential errors, which will lead to more robust and secure code on the user side.

2.2 Additional Convenience Methods

If there is a strong need for more convenient ways of passing non-buffer values to buffer functionality, this should be realized by additional type-unambiguous functions with an explicit name prefix, e.g. keccakFromString().

Reasoning

This is done as a compromise on not allowing type laziness but at the same time provide some extra convenience where there is stronger reasoning for it. Keeping the API unambiguous nevertheless greatly reduces unexpected behavior due to not-thought-of type mixups (e.g. passing "1", 1 to a hash function).

---

Note: feel free to directly rewrite if you are confident on the update.

[holgerd77]

@s1na @alcuadrado Ok, have done some first write-up of mainly hex and buffer-related API rules, which should be enough for a start and some discussion. Let me know what you think, is this too strict? You can also directly edit in the comment and change whatever you like.

Below I will paste the API change proposals from the TypeScript PR (linked above). Will try to adopt a bit to the latest state of discussion. Same here, feel free to edit directly.

[holgerd77]

Breaking API Changes

List with suggestions for breaking API changes

---

account.ts

zeroAddress

Should also return HexString type if we agree on this.

isValidAddress / isZeroAddress / toChecksumAddress / isValidChecksumAddress (#241)

Should follow hex guidelines from above.

generateAddress2 (#243)

Disallow string, buffer guidelines.

isPrecompiled (#242)

This is a very error-prone and implicitly-HF-dependent method anyhow, which I have once removed from the VM and which currently has no other usages with EthereumJS. We should consider to remove.

bytes.ts

setLengthLeft / setLength

Also just did some organization-wide search, this is in (EthereumJS) practice also mostly used on Buffer anyhow (doesn't mean there are other usages out there).

Would suggest that we drop Array support here and just pass and return Buffer here.

The setLength alias can be removed on this occasion (see here.

unpad

Suggestion: three distinct methods unpadBuffer (with buffer guidelines), unpadArray (how does this array version work anyhow?) and unpadHexString (with hex guidelines).

Here is a search on unpad usage.

addHexPrefix

Should return HexString if we agree.

bufferToHex

Should be renamed to bufferToHexString and should also return HexString type if we agree on this.

baToJSON

This is insanely named anyhow and should be renamed to bufferToJSON, this array thing has to be examined more closely.

hash.ts

keccak and similar

Suggestion: in plain version only allow buffer, limited set of convenience methods (e.g. keccakFromString()) after some code search of current method usage in practice (and then support the most used types with additional methods).

object.ts

defineProperties

I think after some first replacement trials we can realistically say that we likely have to live with this method some more time, so I think it would be worth to give this some additional analysis if we can at least tighten the types for the parameters and get eventually rid of the any type for the time being. I would suggest to nevertheless officially deprecate this now in the TSDocs and release notes.

signature.ts

hashPersonalMessage

Suggestion: only allow buffer, buffer guidelines and rewrite the method a bit. Eventually 1-2 often used convenience methods.

toRpcSig / fromRpcSig

Not sure, maybe not for this round (maybe yes 😄), could/should we transform sig into a dedicated string type with stricter checks?

---

TODO: Other methods, please read for latest version on GitHub and not via mail

NOTE: Everyone feel free to replace suggestions from above with the latest version as discussion progresses.

[holgerd77]

Ok, also through with a first round on bringing the list of API change suggestions up-to-date, this should now be enough to have some further discussion basis.

This is the result of some rough/broad bring-everything-up-to-date process and definitely needs some closer look on the changes proposed as well as a check for completeness (all methods which needs an update covered?).

[holgerd77]

Closed by #249