Plutonomicon / cardano-transaction-lib

A Purescript library for building smart contract transactions on Cardano
https://plutonomicon.github.io/cardano-transaction-lib/
MIT License
93 stars 50 forks source link

Add error reporting to FromData #288

Open bladyjoker opened 2 years ago

bladyjoker commented 2 years ago

There's plenty of things that can go wrong during PlutusData decoding and having error reporting would make life downstream a lot easier.

This, however, entails changing the FromData class to:

class FromData :: Type -> Constraint
class FromData (a :: Type) where
  fromData :: PlutusData -> Either FromDataError a

This breaks the API used in the code base, so I'd rather tackle this separately from #209

klntsky commented 2 years ago

Shouldn't our API match Plutus API?

Also, who will be consuming the reported errors and for what purpose?

Also2 I find it hard to provide meaningful messages when it comes to monadic decoders that support alternatives. I.e. which error should be reported from unsuccessful decoding of some a <|> b <|> c? Usually it's the last one, but for the end user it may be counterintuitive, because the ordering is an implementation detail.

klntsky commented 2 years ago

If it's just for our debugging, we can just make two type classes, one of which is not exposed, and another just reuses the former and hushes the result of fromData. The users will write instances for the latter class, and we will be able to debug and (probably) run some automated tests for our instances.

bladyjoker commented 2 years ago

Shouldn't our API match Plutus API?

I don't know tbh. Personally, I don't expect CTL to match the Plutus APIs, and I kind of like that CTL diverges wherever it finds an opportunity to improve the existing status. Either way, works for me.

Also, who will be consuming the reported errors and for what purpose?

Good question. In the ideal world, no one, but the migration to CTL is going to be plagued by plenty of issues. It's already difficult to diagnose the root cause and I fear that without error reporting and extensive logging this is going to be even more difficult.

Also2 I find it hard to provide meaningful messages when it comes to monadic decoders that support alternatives. I.e. which error should be reported from unsuccessful decoding of some a <|> b <|> c? Usually it's the last one, but for the end user it may be counterintuitive, because the ordering is an implementation detail.

Agreed, that's why sometimes one can name alternatives (I think attoparsec does that). I'm suggesting something fairly trivial, which is not ideal for sure, but good enough maybe?

If it's just for our debugging, we can just make two type classes, one of which is not exposed, and another just reuses the former and hushes the result of fromData. The users will write instances for the latter class, and we will be able to debug and (probably) run some automated tests for our instances.

That sounds good. One nit tho, users shouldn't be writing instances manually, but generically deriving them and at most specifying the constructor indices using HasConstrIndices.

I was kind of hoping that the call sites for toData and fromData in QueryM (<- guessing here) could log errors. For instance https://github.com/Plutonomicon/cardano-browser-tx/blob/183388045433b9d9c92a67e74bc105578505a456/src/Types/TypedTxOut.purs#L173 perhaps we can make toData and fromData QueryM convenience actions

fromData :: FromData a => PlutusData -> QueryM a
toData :: ToData a => a -> QueryM PlutusData

That would fail on errors and log an error. Wdyt?

klntsky commented 2 years ago

That would fail on errors and log an error. Wdyt?

QueryM is too powerful for logs. It's like using IO where one needs a Writer.

bladyjoker commented 2 years ago

QueryM is too powerful for logs. It's like using IO where one needs a Writer.

Sure, I thought QueryM was also a Writer as is the case in Plutus Contracts, very useful for tracking contract execution. My point was mostly, we need error detection and reporting, and we need to eventually report that to whatever is running a contract. We have errors when decoding JSON, CBOR, (any processing that can fail) no reason not to have the same capabilities with FromData especially as it can fail in very subtle ways.

Specifically for FromData, I'm trying hard to guarantee compatible PlutusData representation between Haskell Plutus and Purescript CTL data types. But it's a long way from Haskell to Purescript and many things can go wrong in the underlying chain of events.

To summarize, if we detect an error we need to be able to report it, that's a dear principle that saves countless engineering hours and days. I don't hold any preference as to how to do this, and having any err reporting abilities is a win imo.

GeorgeFlerovsky commented 2 years ago

I spent several hours this week in debugging mob sessions. Better error messages would have made the process so much smoother.

The status quo is an obstacle to productivity.

ngua commented 2 years ago

Moving this to stage 5 as we want to keep CTL and purescript-bridge more stable