Introduce the jsonrpc.Error interface

[joshklop]

Allows for custom error types.

[codecov]

Codecov Report

Patch coverage: 96.00% and project coverage change: +0.03% 🎉

Comparison is base (babbbac) 73.71% compared to head (2001459) 73.75%.

❗ Current head 2001459 differs from pull request most recent head 0e60474. Consider uploading reports for the commit 0e60474 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
+ Coverage   73.71%   73.75%   +0.03%     
==========================================
  Files          70       70              
  Lines        7530     7541      +11     
==========================================
+ Hits         5551     5562      +11     
  Misses       1523     1523              
  Partials      456      456              

Files Changed	Coverage Δ
jsonrpc/server.go	93.11% <95.00%> (+0.23%)	⬆️
rpc/handlers.go	77.04% <96.66%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[omerfirmak]

I will read thru the changes tomorrow but;

1. How do we make sure that the error marshals to spec complaint json?
2. What is the benefit, why do we need this?

[joshklop]

How do we make sure that the error marshals to spec complaint json?

We convert everything to a *jsonrpc.StaticError (which has the desired struct tags) in jsonrpc.*Server.handleRequest.

What is the benefit, why do we need this?

So we don't need to modify global error variables, and can create custom types where necessary.

[omerfirmak]

and can create custom types where necessary.

When would that be necessary?

[joshklop]

When would that be necessary?

When we need dynamic error messages (see second commit for examples).

[omerfirmak]

When would that be necessary?

When we need dynamic error messages (see second commit for examples).

Error messages are not dynamically created by the interface tho, they are set at the time of instantiation. You are essentially using it the interface as simple getters.

Such an interface only makes sense to me if the caller (ie jsonrpc.Server) benefits from being able to cast the interface to original concrete error type and makes use of its internal fields and the functions of the interface is generating value (error messages or data) from those fields. I am not sure I see a case where this would be required and be immediately useful for us.

I would love to hear what others think as well. cc: @kirugan @IronGauntlets

[joshklop]

Happy to hear what others think as well.

I am not sure I see a case where this would be required and be immediately useful for us.

Right now, we instantiate global error variables, which precludes us from mutating messages when the spec requires it.

[omerfirmak]

Happy to hear what others think as well.

I am not sure I see a case where this would be required and be immediately useful for us.

Right now, we instantiate global error variables, which precludes us from mutating messages when the spec requires it.

We create local copies of them to be mutated when needed. Is there anything obviously wrong with this that I am missing?

[joshklop]

We create local copies of them to be mutated when needed.

We copy the pointer and mutate the underlying value. Even if we copied the value, it would be sub-optimal since errors.Is would fail.

[omerfirmak]

We create local copies of them to be mutated when needed.

We copy the pointer and mutate the underlying value. Even if we copied the value, it would be sub-optimal since errors.Is would fail.

We copy the value here, no?

https://github.com/NethermindEth/juno/pull/1236/files#diff-1f724c5e433be46bfd8dc0962d1796f0dce2a5e22253fe82f69ab6a1eb8df278L1029-L1031

Even if we copied the value, it would be sub-optimal since errors.Is would fail.

They are not generic error types tho, so errors.Is wont work on them anyways.

[joshklop]

We copy the value here, no?

Yes we do copy it there. Here we do not. Even if we decide the interface isn't necessary, I can fix that in this PR :)

They are not generic error types tho, so errors.Is wont work on them anyways.

Ah yes you're right. Maybe they should be though? For client-facing projects like starknet.go, being able to use errors.Is as usual would be nice.