bitcoin-sv / ts-sdk

Other
51 stars 12 forks source link

[FEATURE REQUEST] #99

Closed rob-tkn closed 4 months ago

rob-tkn commented 4 months ago

Summary

Some details are lost when ARC returns an error, and ts-sdk generalises it down to a code:500, description:'unknown error'.

Motivation

Primarily, we want a small, known, finite set of possible error codes, so that clients can programmatically respond to all the failure modes as simply as possible (e.g. distinguish between temporary and permanent failures). The library helps to keep clients simpler by hiding variations between backends, by categorising failures into one of the small set.

However, during development it's helpful to have access to all details about a failure, for human consumption, to help to work out why the response was failure rather than success. It doesn't matter if the set of details available varies from one backend to another.

Description

For example, If we broadcast a duff TX (it happens...), ARC returns a detailed response such as:

{
    "detail": "Transaction is malformed and cannot be processed",
    "extraInfo": "arc error 461: invalid script",
    "instance": null,
    "status": 461,
    "title": "Malformed transaction",
    "txid": "ea2924da32c47b9942cda5ad30d3c01610ca554ca3a9ca01b2ccfe72bf0667be",
    "type": "https://bitcoin-sv.github.io/arc/#/errors?id=_461"
}

However, the ts-sdk broadcast() method only returns this to the caller:

{
  "status": "error", 
  "code": "461", 
  "description": "Unknown error"
}

At least we have code: 461 there, but you have to know what that means.

Suggestion

All the details returned from the Broadcaster backend could be added into a more property in the response from broadcast(), with no guarantees about the structure of the content – it would just be there for human consumption while diagnosing. All programmatic response should be based on status and code.

ty-everett commented 4 months ago

@tonesnotes This is a valuable enhancement for debugging failures when using Arc with the library. Please extend the Broadcaster interface as described, with an optional more property. Then, it should be implemented and tested with Arc.

tonesnotes commented 4 months ago

Resolved by PR to merge branch tone-featReq99

rob-tkn commented 4 months ago

Thanks for the quick response. The PR brings the txid and detail (as description) through, but there is also extraInfo, instance (whatever that is?), title and type (which contains a link to the description of the error code number) – is there value in including those as well?

I understand the desire to have a common set of fields, to isolate clients from differences in different backends so that they don't have to "duck type" the response to look for different possible fields, but there could be useful extra info available there during development. That's why I suggested to bundle everything from the backend response into a single property in the ts-sdk response, to be considered as an opaque type as far as the ts-sdk API is concerned, for human consumption only, and only to be included in failure cases, so as not to weigh down the system in normal usage. WDYT?

tonesnotes commented 4 months ago

Done. Missed @ty-everett original suggestion to add optional more. Leaving the new optional txid as it seems to be a useful clearly defined property that should be expected from most broadcasters.

ty-everett commented 4 months ago

Implemented in #100