feat: add 'Data' field to 'respError' struct #123

virajbhartiya · 2024-10-03T13:24:21Z

This pull request includes a small change to the handler.go file. The change adds an optional Data field to the respError struct. This is done so as to implement changes mentioned in filecoin-project/lotus#10311

handler.go: Added Data field to the respError struct to include additional error information.

codecov-commenter · 2024-10-03T13:25:26Z

Codecov Report

Attention: Patch coverage is 70.42254% with 21 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@c185272). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
handler.go	35.29%	10 Missing and 1 partial ⚠️
response.go	84.00%	6 Missing and 2 partials ⚠️
websocket.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #123   +/-   ##
=========================================
  Coverage          ?   71.38%           
=========================================
  Files             ?       12           
  Lines             ?     1957           
  Branches          ?        0           
=========================================
  Hits              ?     1397           
  Misses            ?      459           
  Partials          ?      101

Files with missing lines	Coverage Δ
client.go	`79.67% <ø> (ø)`
errors.go	`65.21% <ø> (ø)`
server.go	`75.75% <100.00%> (ø)`
websocket.go	`74.41% <33.33%> (ø)`
response.go	`84.00% <84.00%> (ø)`
handler.go	`71.17% <35.29%> (ø)`

aarshkshah1992 · 2024-10-03T13:27:09Z

I think we will need more changes here for the library to determine if the error has data and to read it from the error but we can worry about it when we get blocked by it. We'll find out more when we do the Lotus work. Update Lotus to use this go-jsonrpc commit locally while you are working on the Lotus PR.

akaladarshi · 2024-10-09T15:15:04Z

@aarshkshah1992 As per the @Stebalien comments : https://filecoinproject.slack.com/archives/CP50PPW2X/p1728439741305709?thread_ts=1727699414.185139&cid=CP50PPW2X

We tried to add json.RawMessage, but I was getting json body invalid in response, so I went back to using interface,
I personally haven't gone into the detail why it was happening but if you have any thought about this let us know.

aarshkshah1992 · 2024-10-09T15:22:31Z

@akaladarshi Before I review this -> can you guys post a snippet of the errors you see for contract reverts when calling via JSON RPC on a Calibnet node ?

akaladarshi · 2024-10-09T15:25:02Z

@akaladarshi Before I review this -> can you guys post a snippet of the errors you see for contract reverts when calling via JSON RPC on a Calibnet node ?

@aarshkshah1992 so we hardcoded the message ourselves but the data is fetched from the error returned after applying the message.

akaladarshi · 2024-10-09T15:35:06Z

and for same function call ethereum returns this

aarshkshah1992 · 2024-10-15T05:15:35Z

errors.go

+}
+
+// DataError contains extra data to explain the error
+type DataError interface {


ErrorWithData

aarshkshah1992 · 2024-10-15T05:15:46Z

errors.go

@@ -58,3 +58,15 @@ type marshalable interface {
 	json.Marshaler
 	json.Unmarshaler
 }
+
+// Error wraps RPC errors, which contain an error code in addition to the message.
+type Error interface {


Why do we need this interface ?

Added so we can have custom error codes.

Why do we need custom error codes ? Can you write a bit explaining what made you add this ? Was it something you saw during your testing ?

Also this needs a better name. This can be ErrorWithCode and the below can be ErrorWithData

@aarshkshah1992 I think this will give more idea towards the error codes: filecoin-project/lotus#12553

aarshkshah1992 · 2024-10-15T05:17:10Z

handler.go

@@ -69,6 +70,7 @@ type respError struct {
 	Code    ErrorCode       `json:"code"`
 	Message string          `json:"message"`
 	Meta    json.RawMessage `json:"meta,omitempty"`
+	Data    interface{}     `json:"data,omitempty"`


Why is this interface{} ?

@aarshkshah1992 I posted a comment above in current PR regarding this: #123 (comment)

Can we make this just a string ? Why does it have to be json.RawMessage or interface{}

@rvagg -> Any thoughts ?

https://www.jsonrpc.org/specification#error_object

A Primitive or Structured value that contains additional information about the error.

🤷 interface{} would seem more accurate, as long as it's serialisable

aarshkshah1992 · 2024-10-15T06:27:44Z

handler.go

 		}
 	}

+	var err2 Error
+	if errors.As(err, &err2) {
+		out.Code = ErrorCode(err2.ErrorCode())


Why are we setting the code here again ? Don't we already have it on line 355 above ?

@aarshkshah1992
So in line 355 we are setting the registered error codes, but here we are setting the custom error code.

I think we can directly register the code for ErrWithExecutionReverted, but then we need to finalise on which code to use, as error code 3 is already registered with the ActorNotFound

aarshkshah1992 · 2024-10-15T06:31:42Z

handler.go

@@ -504,10 +524,17 @@ func (s *handler) handle(ctx context.Context, req request, w func(func(io.Writer

 			log.Warnf("failed to setup channel in RPC call to '%s': %+v", req.Method, err)
 			stats.Record(ctx, metrics.RPCResponseError.M(1))
-			resp.Error = &respError{
+			respErr := &respError{


Don't we need to do the same thing in the rpcError function in server.go ?

Probably not as rpcErr is only used for RPC processing errors and not errors returned by the invocation of the function.

I am not sure about this because rpcError function is returning errors specific to RPCs (method not found etc), but the actual execution error is inside the handle function after doCall here:

go-jsonrpc/handler.go

Line 450 in c185272

callResult, err := doCall(req.Method, handler.handlerFunc, callParams)

aarshkshah1992 · 2024-10-15T06:43:49Z

@rvagg Please can you 👁️ this ?

rvagg · 2024-10-16T05:22:08Z

some tests for this in here would be good

rvagg · 2024-10-23T01:56:34Z

response.go

+var (
+	_               error = (*JSONRPCError)(nil)
+	marshalableRT         = reflect.TypeOf(new(marshalable)).Elem()
+	unmarshalableRT       = reflect.TypeOf(new(RPCErrorCodec)).Elem()


Suggested change

unmarshalableRT = reflect.TypeOf(new(RPCErrorCodec)).Elem()

errorCodecRT = reflect.TypeOf(new(RPCErrorCodec)).Elem()

rvagg · 2024-10-23T01:56:45Z

response.go

+				v = reflect.New(t)
+			}
+
+			if v.Type().Implements(unmarshalableRT) {


Suggested change

if v.Type().Implements(unmarshalableRT) {

if v.Type().Implements(errorCodecRT) {

rvagg · 2024-10-23T01:59:07Z

response.go

+			}
+
+			if v.Type().Implements(unmarshalableRT) {
+				_ = v.Interface().(RPCErrorCodec).FromJSONRPCError(*e)


if we get an error here (and in the block below for UnmarshalJSON), we should:

log.Warnf

return reflect.ValueOf(e) because we can't know that we have a properly instantiated converted error type

rvagg · 2024-10-24T02:56:58Z

response.go

+
+			if len(e.Meta) > 0 && v.Type().Implements(marshalableRT) {
+				if err := v.Interface().(marshalable).UnmarshalJSON(e.Meta); err != nil {
+					log.Errorf("Error unmarshaling metadata for error type '%s': %w", t.String(), err)


Suggested change

log.Errorf("Error unmarshaling metadata for error type '%s': %w", t.String(), err)

log.Errorf("Error unmarshalling error metadata to custom error type '%s' (code %d): %w", t.String(), e.Code, err)

rvagg · 2024-10-24T02:57:14Z

response.go

+
+			if v.Type().Implements(errorCodecRT) {
+				if err := v.Interface().(RPCErrorCodec).FromJSONRPCError(*e); err != nil {
+					log.Errorf("Error converting JSONRPCError (code %d) to custom error type '%s': %w", e.Code, t.String(), err)


Suggested change

log.Errorf("Error converting JSONRPCError (code %d) to custom error type '%s': %w", e.Code, t.String(), err)

log.Errorf("Error converting JSONRPCError to custom error type '%s' (code %d): %w", t.String(), e.Code, err)

rvagg · 2024-10-24T02:58:33Z

response.go

+			}
+
+			if len(e.Meta) > 0 && v.Type().Implements(marshalableRT) {


Suggested change

}

if len(e.Meta) > 0 && v.Type().Implements(marshalableRT) {

} else if len(e.Meta) > 0 && v.Type().Implements(marshalableRT) {

let's disallow dual behaviour for now, we could enable this at some point if there was ever a case for it (I can't think what it would be) but best avoid surprises if we're not testing for it

rvagg · 2024-10-24T03:08:58Z

handler.go

-			resp.Error = &respError{
+
+			respErr := &JSONRPCError{
 				Code:    1,
 				Message: err.Error(),
 			}
+
+			if m, ok := err.(RPCErrorCodec); ok {
+				rpcErr, err := m.ToJSONRPCError()
+				if err != nil {
+					log.Warnf("Failed to convert error to JSONRPCError: %v", err)
+				} else {
+					respErr.Data = rpcErr.Data
+				}
+			}
+
+			resp.Error = respErr


This is unnecessary, we only ever get back an internal error at this point which is why code 1 is being used as the internal error code (custom codes start at 2). You can follow this case down in to websocket.go handleChanOut. So all you need to do here is s/respError/JSONRPCError/ and leave the rest alone.

I will remove this from here, wdym by when you say You can follow this case down in to websocket.go handleChanOut. I don't see any error in the response there.

rvagg · 2024-10-24T03:09:48Z

handler.go

+	case RPCErrorCodec:
+		o, err := m.ToJSONRPCError()
+		if err != nil {
+			log.Warnf("Failed to convert error to JSONRPCError: %v", err)


let's Errorf these two as well, if they're not converting then it's either a programmer error or a system error (like OOM)

rvagg

looks good, just a few minor changes and this is good to ship I think

rvagg · 2024-10-24T11:56:17Z

well done @virajbhartiya & @akaladarshi, nice work on this one

refactor: add 'Data' field to 'respError' struct

d56a7d5

virajbhartiya added 2 commits October 5, 2024 11:53

feat: return Data in createError

8493164

feat: Add 'Data' field to 'response' struct

dfc83a0

BigLep assigned virajbhartiya Oct 7, 2024

BigLep requested a review from aarshkshah1992 October 7, 2024 23:45

virajbhartiya and others added 2 commits October 9, 2024 00:47

feat: Update error handling in handler.go

514c1af

refactor: add data field in response error

a0806e5

add error code in interface to get custom codes

53eab64

aarshkshah1992 reviewed Oct 15, 2024

View reviewed changes

errors.go Outdated

}

// DataError contains extra data to explain the error

type DataError interface {

Copy link

aarshkshah1992 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorWithData

aarshkshah1992 reviewed Oct 15, 2024

View reviewed changes

aarshkshah1992 requested a review from rvagg October 15, 2024 06:43

address comments

4358258

akaladarshi added 5 commits October 16, 2024 18:32

remove the error with code interface

4e33c9f

add message and data in process response

9592989

add test for response error and refactor

65441ad

refactor: json rpc error and error codec interface

ef506b1

rename the interface

e8dc77a

akaladarshi force-pushed the master branch from a32b50f to e8dc77a Compare October 22, 2024 14:12

rvagg reviewed Oct 23, 2024

View reviewed changes

rvagg mentioned this pull request Oct 23, 2024

fix: handle error and revert data in EthEstimateGas and EthCall filecoin-project/lotus#12553

Merged

8 tasks

address comments

90c8605

rvagg reviewed Oct 24, 2024

View reviewed changes

rvagg requested changes Oct 24, 2024

View reviewed changes

address comments

9ef484a

rvagg approved these changes Oct 24, 2024

View reviewed changes

rvagg merged commit a9fac20 into filecoin-project:master Oct 24, 2024
7 checks passed

	unmarshalableRT = reflect.TypeOf(new(RPCErrorCodec)).Elem()
	errorCodecRT = reflect.TypeOf(new(RPCErrorCodec)).Elem()

	if v.Type().Implements(unmarshalableRT) {
	if v.Type().Implements(errorCodecRT) {

	log.Errorf("Error unmarshaling metadata for error type '%s': %w", t.String(), err)
	log.Errorf("Error unmarshalling error metadata to custom error type '%s' (code %d): %w", t.String(), e.Code, err)

	log.Errorf("Error converting JSONRPCError (code %d) to custom error type '%s': %w", e.Code, t.String(), err)
	log.Errorf("Error converting JSONRPCError to custom error type '%s' (code %d): %w", t.String(), e.Code, err)

feat: add 'Data' field to 'respError' struct #123

feat: add 'Data' field to 'respError' struct #123

Conversation

virajbhartiya commented Oct 3, 2024

codecov-commenter commented Oct 3, 2024 • edited Loading

Codecov Report

aarshkshah1992 commented Oct 3, 2024

akaladarshi commented Oct 9, 2024

aarshkshah1992 commented Oct 9, 2024

akaladarshi commented Oct 9, 2024 • edited Loading

akaladarshi commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 15, 2024

rvagg commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg left a comment

Choose a reason for hiding this comment

rvagg commented Oct 24, 2024

codecov-commenter commented Oct 3, 2024 •

edited

Loading

akaladarshi commented Oct 9, 2024 •

edited

Loading