Zondax / filecoin-solidity

Filecoin Solidity API Library
Apache License 2.0
93 stars 43 forks source link

Reconsider the authenticateMessage API #345

Closed Stebalien closed 1 year ago

Stebalien commented 1 year ago

With respect to https://github.com/Zondax/filecoin-solidity/issues/338.

We shouldn't match the behavior of the account actor here, we should either:

  1. Always return boolean success/fail (and document it). This includes catching any errors from the underlying send.
  2. Always raise an error on failure (and succeed with no value).

Right now, this function may return a boolean and/or may raise an error, but doesn't do so consistently (and the documentation is out of date).

For some context, the account actor returns a boolean true to let us distinguish between, e.g., a "placeholder" (that simply returns true by default) and an actor that actually implements this API. However, there's no reason to reason to replicate that behavior here.


Personally, I'd recommend raising an error.

  1. Changing this library from reverting to returning a boolean is dangerous (could cause significant security issues in contracts that don't carefully check the changelog and simply rely on compile-time type checks).
  2. This will allow authenticateMessage to propagate useful error messages on failure beyond a simple "false".

:link: zboto Link