paulmillr/micro-eth-signer

Thoughts on API changes

Closed this issue · 3 comments

mahnunchik commented

I've faced with the several strange API changes when upgrading micro-eth-signer from version 0.6.4 to 0.9.1 CoinSpace/cs-evm-wallet@7f0e6c7

Thoughts on API changes:

  1. It is much easier to use and maintain if the library uses values ​​in a single scale, such as cryptocurrency atoms (wei for Ethereum). In version 0.9.1 the library returns wei and humanized formats in calcAmounts() method. It complicates the library internally, but in the application it is most likely required to format number again (for example if trailing zeros needed).
  2. The reason for adding calcAmounts() method is unclear. It seems much clearer for the end user to use getters such .fee, .amount.
  3. Precalculation of amountWithFee may be easy calculated (fee + amount) and may not be required by the consumer at all.

The rest of the changes between version in the library seem quite logical.

paulmillr commented

yeah, humanized is not ideal and can likely be dropped because >the application it is most likely required to format number again

It seems much clearer for the end user to use getters

I've dropped all getters except for hash, sender and v, because it had too many getters and it was hard to reason about the transaction: which fields are auto-calculated; which fields are not. After the update:

  • The transaction can now be easily inspected in console.log where it always has humanized representation, instead of JSON-stringified crap like before
  • v is rarely used, so doesn't matter
  • sender is also rarely used - it's just an alias for recoverSender().address
    • if you need a public key, then it doesn't work: do recoverSender().publicKey
    • so just a small convenience function
  • hash is more common. It can't be made pre-computed, because it would consume too much resources
    • getHash() is weird name
    • toHash() is also not great but I guess we can use it

Precalculation of amountWithFee may be easy calculated (fee + amount)

It's maximum amount that the transaction would consume. All fee estimations are "miscalculated", because gas price can't be known in advance — unless you set priorityFee = fee.

may not be required by the consumer at all

You're sending 0.5 eth with 20 gwei fee. Then you're actually spending 0.500042 eth (MAX) and not 0.5. That seems like a useful thing to know. Wallet can also present a UI where a user would enter they're wanting to spend exactly 0.5 eth. Then the wallet would re-calc the amount/fee.

  • 👍1
mahnunchik commented

As multi currency wallet app developer I can say that almost all open source wallet applications has almost the same architectecture:

  • Underlying cryptocurrency library
  • Wallet abstraction
  • UI application

For example edge-currency-accountbased and cs-evm-wallet

It is better if underlying library keep the balance between full featured and simplicity. It is easy to use, easy to audit and of course much easy to find a bugs.

As I already mentioned, we don't know exactly the use case, for example

  • The application can show 5 transactions and that's it. Then there is no sense for caching and precomputation.
  • In opposition, the application can juggle thousands of transactions. Then, in my opinion, caching and precomputation can be implemented in the wallet abstraction, not it underlying library to keep it simple.
  • Displaying the amount is the responsibility of the UI application. amount, fee, and amountWithFee can be shown in wei and humanized form. Or even gasLimit can be shown. The need for a specific UI application is unknown for us.

My opinion is that it was more convenient to have a separate getter .fee (or getter function .getFee()) for each value without caching and precomputation.

For pretty console.log inspection toString() method can be implemented: https://github.com/CoinSpace/cs-common/blob/master/lib/Transaction.js#L58-L65

Of course it is better to have helpers like ethDecimal and gweiDecimal.