clarity-lang / reference

The Clarity Reference
146 stars 34 forks source link

Error types support #33

Open agraebe opened 3 years ago

agraebe commented 3 years ago

We discussed improvements for Stacks tooling lately and identified a pain point inside Clarity related to error handling. It seems that error types are usually created as constants, with the name representing the error type. The constant would get an integer value and returned whenever an error occurs.

Developers would need to take the value, open the contract, and read the constant name to understand the error type. @kantai suggested we could consider error enums to support better error handling.

@kantai - I'm creating this issue as a starting point for the conversation and I'll let you chime in on the proposal and details

diwakergupta commented 3 years ago

@agraebe this is potentially a nice-to-have feature. There's a spectrum, at least within the PBC blockchain-team, about the utility (relative to implementation cost) this would provide in practice.

Implementing error types as described above would be a fairly large undertaking, so at the moment it isn't in consideration for Stacks 2.0 mainnet.

njordhov commented 3 years ago

Developers would need to take the value, open the contract, and read the constant name to understand the error type.

@agraebe Encoding errors as record structures rather than integers would be an improvement. Such errors would be unique, avoiding the collisions happening with the current error codes, where the same integers are used for different error types. For example, (err 1) is returned by built-in Clarity functions to report a variety of errors:

It can be challenging to identify the semantics of such integer error codes, which may require analyzing contract code and trace calls to identify what triggered the specific error. In contrast, errors encoded as records can include properties to make them self-documenting. Unlike enums, records (aka tuples) are already supported, so it is a feasible step forward rather than a large undertaking.

njordhov commented 3 years ago

[It] would be a fairly large undertaking, so at the moment it isn't in consideration for Stacks 2.0 mainnet.

@diwakergupta The choice of error representation is contagious: Developers will have good reason to use the same error representation as in the core functions, as mixing error types is more complicated due to the strict type system and the pervasive use of abnormal function returns.

Abnormal returns (like from unwrap!) makes custom functions have a response type with the error variant of the unwrapped value. If the unwrapped response is from a call to a core function, the custom function will by inference have a response type with an (err int) variant. Developers will then have to use (err int) response types in custom functions that are unwrapped in the same block, likely making it the de facto standard choice.

This will make it hard to upgrade from (err int) after mainnet launch. Choose wisely.

friedger commented 3 years ago

The description by @njordhov is exactly what I am experience when writing and debug Clarity code. Contract code analysis to understand error codes and using the same error type for my functions as the core functions do due to unwrap!

diwakergupta commented 3 years ago

[It] would be a fairly large undertaking, so at the moment it isn't in consideration for Stacks 2.0 mainnet.

@diwakergupta The choice of error representation is contagious: Developers will have good reason to use the same error representation as in the core functions, as mixing error types is more complicated due to the strict type system and the pervasive use of abnormal function returns.

Abnormal returns (like from unwrap!) makes custom functions have a response type with the error variant of the unwrapped value. If the unwrapped response is from a call to a core function, the custom function will by inference have a response type with an (err int) variant. Developers will then have to use (err int) response types in custom functions that are unwrapped in the same block, likely making it the de facto standard choice.

This will make it hard to upgrade from (err int) after mainnet launch. Choose wisely.

@njordhov @friedger thanks, these are valid points. The primary constraint (at least for PBC) is bandwidth: if either of you would like to help out with this, I'm happy to explore options. Please reach out!

friedger commented 3 years ago

What should the tuple looks like?

{error: "asset not found", reason: "nft with id was not minted", reason-data: "1"}

njordhov commented 3 years ago

What should the tuple looks like?

  1. A unique property identifying the kind of error, primarily for programmatic dispatch (whether in Clarity, or from a contract call from other languages like Javascript).
  2. A message describing the error, for human consumption and reporting.